FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix: use stableObjectId field for object equality in Node 10+ by kjin · Pull Request #524 · googleapis/cloud-debug-nodejs · GitHub

This repository was archived by the owner on Apr 3, 2024. It is now read-only.
/ cloud-debug-nodejs Public archive

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

Merged
kjin merged 10 commits into
googleapis:masterfrom
kjin:n10-soi
Mar 14, 2019
ConversationCommits (10)ChecksFiles changed
Merged

fix: use stableObjectId field for object equality in Node 10+#524
kjin merged 10 commits into
googleapis:masterfrom
kjin:n10-soi

Conversation

kjin commented Sep 11, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor

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 added the cla: yes This human has signed the Contributor License Agreement. label Sep 11, 2018
kjin mentioned this pull request Sep 11, 2018
Closed
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 requested a review from a team September 12, 2018 17:34
DominicKramer previously approved these changes Sep 12, 2018
View reviewed changes

DominicKramer left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 commented Sep 17, 2018 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 assigned kjin Sep 17, 2018
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 force-pushed the n10-soi branch 2 times, most recently from d17da40 to ca9c7f3 Compare October 26, 2018 18:21

kjin commented Oct 26, 2018

Copy link
Copy Markdown
Contributor Author

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

DominicKramer commented Nov 19, 2018

Copy link
Copy Markdown
Contributor

@kjin Is this PR ready to land?

kjin commented Nov 29, 2018

Copy link
Copy Markdown
Contributor Author

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

DominicKramer commented Jan 31, 2019

Copy link
Copy Markdown
Contributor

@kjin What is the status on this?

kjin commented Jan 31, 2019

Copy link
Copy Markdown
Contributor Author

nodejs/node#25330

kjin force-pushed the n10-soi branch 2 times, most recently from 696e1b7 to d6600e6 Compare February 5, 2019 22:41
yoshi-automation added the 🚨 This issue needs some love. label Feb 7, 2019
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 closed this Feb 8, 2019
ofrobots reopened this Feb 11, 2019
ofrobots removed the 🚨 This issue needs some love. label Feb 11, 2019
DominicKramer added the status: blocked Resolving the issue is dependent on other work. label Feb 11, 2019

kjin commented Feb 11, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

codecov Bot commented Mar 7, 2019 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

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

@@            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 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 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 added the 🚨 This issue needs some love. label Mar 13, 2019
kjin requested a review from a team March 13, 2019 20:49

kjin commented Mar 13, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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

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

Content changed significantly

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 added status: blocked Resolving the issue is dependent on other work. and removed 🚨 This issue needs some love. labels Mar 14, 2019

JustinBeckwith commented Mar 14, 2019

Copy link
Copy Markdown
Contributor

Is this really still blocked?

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

DominicKramer commented Mar 14, 2019

Copy link
Copy Markdown
Contributor

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

yoshi-automation added the 🚨 This issue needs some love. label Mar 14, 2019
DominicKramer approved these changes Mar 14, 2019
View reviewed changes
Comment thread src/agent/debuglet.ts Outdated

kjin commented Mar 14, 2019 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

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 merged commit 91f4dbb into googleapis:master Mar 14, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer
DominicKramer DominicKramer approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

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

Back | FazBrowse Home | New Git URL