lib: disbanden circular dependency#14885
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added assert 
lpinca left a comment
lpinca left a comment There was a problem hiding this comment.
LGTM but how was the circular dependency created? It's not clear to me.
BridgeAR commented Aug 17, 2017
@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 commented Aug 21, 2017
The buffer module is lazily required in the deepEqual checks. That is why there is the circular dependency. |
refack commented Aug 21, 2017
Since Line 120 in bf18fe1 be replaced with a simple Buffer.from?Or maybe replace the Lines 133 to 134 in bf18fe1 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 commented Aug 21, 2017
@refack I like the idea of using |
refack commented Aug 21, 2017
Lets see if it works - https://ci.nodejs.org/job/node-test-commit-linuxone/8136/ |
BridgeAR commented Aug 23, 2017
| } | ||
| returntrue; | ||
| } | ||
| returncompare(from(a.buffer,a.byteOffset,len), |
There was a problem hiding this comment.
I would just make this Buffer.from() to make it clearer what is happening
There was a problem hiding this comment.
I am no sure I can follow. The Buffer.from part was removed intentionally. Should I add a comment to describe the Uint8Array part?
There was a problem hiding this comment.
IMHO a comment explaining just that would be great.
There was a problem hiding this comment.
I edited the comment above the function. I am not really sure what to write besides what I now changed.
jasnell commented Aug 23, 2017
lpinca commented Aug 24, 2017
Still LGTM. |
PR-URL: nodejs#14885 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
BridgeAR commented Aug 25, 2017
Landed in 9aa7093 |
PR-URL: nodejs/node#14885 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: nodejs/node#14885 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #14885 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #14885 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
PR-URL: #14885 Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Refael Ackermann <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins commented Sep 20, 2017
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 commented Sep 21, 2017
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 commented Dec 11, 2017
After looking at this again, I do not see the necessity to backport this. So I changed the labels accordingly. |
6 participants
The circular dependency can be disbanded by directly using the binding functions in assert.
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
util, assert