Skip to content
This repository was archived by the owner on Apr 3, 2024. It is now read-only.

fix: use stableObjectId field for object equality in Node 10+ #524

Merged
merged 10 commits into from
Mar 14, 2019
Merged

fix: use stableObjectId field for object equality in Node 10+ #524

merged 10 commits into from
Mar 14, 2019

Conversation

kjin
Copy link
Contributor

@kjin kjin commented Sep 11, 2018 •
edited
Loading

This allows us to properly represent objects with circular references in Node 10+, unless capture.maxDataSize=0, which will only be fixed once stableObjectId is introduced in Node 10. See #516

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Sep 11, 2018
@kjin kjin mentioned this pull request Sep 11, 2018
Closed
@kjin kjin changed the title [WIP] fix: use [stable]ObjectId field for object equality in Node 10+ fix: use [stable]ObjectId field for object equality in Node 10+ Sep 12, 2018
@kjin kjin requested a review from a team September 12, 2018 17:34
DominicKramer
DominicKramer previously approved these changes Sep 12, 2018
View reviewed changes
Copy link
Contributor

@DominicKramer DominicKramer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I would suggest adding a test to ensure the warning message you added is displayed under and only under the conditions in which it should be displayed.

@kjin
Copy link
Contributor Author

kjin commented Sep 17, 2018 •
edited
Loading

Just a note, we should not land this yet since the method for getting stable object ID will change. Once the functionality is committed to V8, I will provide an update.

@ghost ghost assigned kjin Sep 17, 2018
@kjin kjin changed the title fix: use [stable]ObjectId field for object equality in Node 10+ [do not land] fix: use [stable]ObjectId field for object equality in Node 10+ Sep 17, 2018
@kjin kjin force-pushed the n10-soi branch 2 times, most recently from d17da40 to ca9c7f3 Compare October 26, 2018 18:21
@kjin
Copy link
Contributor Author

kjin commented Oct 26, 2018

This fix should work once this is landed on v10/v11: nodejs/node#23886

@DominicKramer
Copy link
Contributor

@kjin Is this PR ready to land?

@kjin
Copy link
Contributor Author

kjin commented Nov 29, 2018

@DominicKramer Not yet, see my previous comment. The PR has to get merged first, then backported to v10/v11, and then released.

@DominicKramer
Copy link
Contributor

@kjin What is the status on this?

@kjin
Copy link
Contributor Author

kjin commented Jan 31, 2019

nodejs/node#25330

@kjin kjin force-pushed the n10-soi branch 2 times, most recently from 696e1b7 to d6600e6 Compare February 5, 2019 22:41
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Feb 7, 2019
@ofrobots ofrobots added the do not merge Indicates a pull request not ready for merge, due to either quality or timing. label Feb 8, 2019
@kjin kjin closed this Feb 8, 2019
@ofrobots ofrobots reopened this Feb 11, 2019
@ofrobots ofrobots removed the 🚨 This issue needs some love. label Feb 11, 2019
@DominicKramer DominicKramer added the status: blocked Resolving the issue is dependent on other work. label Feb 11, 2019
@kjin
Copy link
Contributor Author

kjin commented Feb 11, 2019 •
edited
Loading

This is blocked on release of Node 10.15.2 10.15.3, followed by Kokoro image updates for Node 10/11

@codecov
Copy link

codecov bot commented Mar 7, 2019 •
edited
Loading

Codecov Report

Merging #524 into master will decrease coverage by 0.02%.
The diff coverage is 92.1%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #524      +/-   ##
==========================================
- Coverage   92.91%   92.88%   -0.03%     
==========================================
  Files          38       38              
  Lines        3556     3570      +14     
  Branches      106      108       +2     
==========================================
+ Hits         3304     3316      +12     
- Misses        229      230       +1     
- Partials       23       24       +1
Impacted Files Coverage Δ
test/test-debuglet.ts 98.31% <ø> (ø) ⬆️
src/agent/debuglet.ts 83.6% <33.33%> (-0.49%) ⬇️
test/test-circular.ts 95.16% <97.14%> (+1.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1cc9619...cb1b2fc. Read the comment docs.

@kjin kjin changed the title [do not land] fix: use [stable]ObjectId field for object equality in Node 10+ fix: use [stable]ObjectId field for object equality in Node 10+ Mar 13, 2019
@kjin kjin removed do not merge Indicates a pull request not ready for merge, due to either quality or timing. status: blocked Resolving the issue is dependent on other work. labels Mar 13, 2019
@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Mar 13, 2019
@kjin kjin requested a review from a team March 13, 2019 20:49
@kjin
Copy link
Contributor Author

kjin commented Mar 13, 2019 •
edited
Loading

@googleapis/node-team This is finally ready to re-review and land, now that 10.15.3 is out. PTAL.

@kjin kjin dismissed DominicKramer’s stale review March 13, 2019 20:50

Content changed significantly

@kjin kjin changed the title fix: use [stable]ObjectId field for object equality in Node 10+ fix: use stableObjectId field for object equality in Node 10+ Mar 13, 2019
@DominicKramer DominicKramer added status: blocked Resolving the issue is dependent on other work. and removed 🚨 This issue needs some love. labels Mar 14, 2019
@JustinBeckwith
Copy link
Contributor

Is this really still blocked?

@DominicKramer DominicKramer removed the status: blocked Resolving the issue is dependent on other work. label Mar 14, 2019
@DominicKramer
Copy link
Contributor

@JustinBeckwith I guess it is not now. When I set the blocked label I didn't see Kelvin's last comment.

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Mar 14, 2019
DominicKramer
DominicKramer approved these changes Mar 14, 2019
View reviewed changes
@kjin
Copy link
Contributor Author

kjin commented Mar 14, 2019 •
edited
Loading

There will be a slight code coverage decrease because we can't pin old versions of Node to test that a warning message appears on those versions. I think this is fine as we know quite clearly which change introduced the fix we needed: 10.x, 11.x.

@kjin kjin merged commit 91f4dbb into googleapis:master Mar 14, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@DominicKramer DominicKramer DominicKramer approved these changes

Assignees

@kjin kjin

Labels
cla: yes This human has signed the Contributor License Agreement. 🚨 This issue needs some love.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@kjin @DominicKramer @JustinBeckwith @ofrobots @googlebot @yoshi-automation