Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

lib: disbanden circular dependency #14885

Closed
wants to merge 3 commits into from
Closed

lib: disbanden circular dependency #14885

wants to merge 3 commits into from

Conversation

BridgeAR
Copy link
Member

The circular dependency can be disbanded by directly using the binding functions in assert.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines
Affected core subsystem(s)

util, assert

@nodejs-github-bot nodejs-github-bot added assert Issues and PRs related to the assert subsystem. util Issues and PRs related to the built-in util module. labels Aug 17, 2017
lpinca
lpinca approved these changes Aug 17, 2017
View reviewed changes
Copy link
Member

@lpinca lpinca 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 how was the circular dependency created? It's not clear to me.

@BridgeAR
Copy link
Member Author

@Ipinca honestly speaking: I am not absolutely sure as I did not check further but this was my first guess when looking at it and it worked.

@BridgeAR
Copy link
Member Author

The buffer module is lazily required in the deepEqual checks. That is why there is the circular dependency.

refack
refack approved these changes Aug 21, 2017
View reviewed changes
Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

Yay 🙌

@refack
Copy link
Contributor

refack commented Aug 21, 2017

Since Buffer is bound to the global scope should this

node/lib/assert.js

Line 120 in bf18fe1

const { from } = require('buffer').Buffer;

be replaced with a simple Buffer.from?

Or maybe replace the from in

node/lib/assert.js

Lines 133 to 134 in bf18fe1

return compare(from(a.buffer, a.byteOffset, len),
from(b.buffer, b.byteOffset, b.byteLength)) === 0;

with new Uint8Array (since that is actually the underlying operation)

I'm thinking out loud since I'm not sure if it will just mask the circular dependency.

@BridgeAR
Copy link
Member Author

@refack I like the idea of using new Uint8Array as the first call would otherwise be bad with the require.

@refack
Copy link
Contributor

refack commented Aug 21, 2017

Lets see if it works - https://ci.nodejs.org/job/node-test-commit-linuxone/8136/

@refack refack added the buffer Issues and PRs related to the buffer subsystem. label Aug 21, 2017
@BridgeAR
Copy link
Member Author

@refack @lpinca would you be so kind and have another look and give your OKs since I changed to use Uint8Arrays directly? I would like to land this soon.

jasnell
jasnell reviewed Aug 23, 2017
View reviewed changes
lib/assert.js
@@ -131,8 +129,8 @@ function areSimilarTypedArrays(a, b) {
}
return true;
}
return compare(from(a.buffer, a.byteOffset, len),
Copy link
Member

Choose a reason for hiding this comment

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

I would just make this Buffer.from() to make it clearer what is happening

Copy link
Member Author

Choose a reason for hiding this comment

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

I am no sure I can follow. The Buffer.from part was removed intentionally. Should I add a comment to describe the Uint8Array part?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMHO a comment explaining just that would be great.

Copy link
Member Author

Choose a reason for hiding this comment

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

I edited the comment above the function. I am not really sure what to write besides what I now changed.

@jasnell
Copy link
Member

jasnell commented Aug 23, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/9812/

jasnell
jasnell approved these changes Aug 23, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green

@lpinca
Copy link
Member

lpinca commented Aug 24, 2017

Still LGTM.

BridgeAR added a commit to BridgeAR/node that referenced this pull request Aug 25, 2017
@BridgeAR
lib: remove circular reference
9aa7093 
PR-URL: nodejs#14885
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@BridgeAR
Copy link
Member Author

Landed in 9aa7093

@BridgeAR BridgeAR closed this Aug 25, 2017
ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
@BridgeAR
lib: remove circular reference
18a35ae 
PR-URL: nodejs/node#14885
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
ghost pushed a commit to ayojs/ayo that referenced this pull request Aug 30, 2017
@BridgeAR
lib: remove circular reference
c841263 
PR-URL: nodejs/node#14885
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@BridgeAR @MylesBorins
lib: remove circular reference
d538b25 
PR-URL: #14885
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 11, 2017
@BridgeAR @MylesBorins
lib: remove circular reference
0a22d0d 
PR-URL: #14885
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@BridgeAR @MylesBorins
lib: remove circular reference
d5a376a 
PR-URL: #14885
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
@MylesBorins
Copy link
Contributor

This does not land cleanly in LTS. Please feel free to manually backport by following the guide. Please also feel free to replace do-not-land if it is being backported

@BridgeAR
Copy link
Member Author

This is going to be much easier after the backport for #13862 has landed. I open a PR for that in a few minutes.

@BridgeAR
Copy link
Member Author

After looking at this again, I do not see the necessity to backport this. So I changed the labels accordingly.

@BridgeAR BridgeAR deleted the remove-circular-ref branch April 1, 2019 23:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@lpinca lpinca lpinca approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
assert Issues and PRs related to the assert subsystem. buffer Issues and PRs related to the buffer subsystem. util Issues and PRs related to the built-in util module.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@BridgeAR @refack @jasnell @lpinca @MylesBorins @nodejs-github-bot