lib: disbanden circular dependency#14885
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.
|
@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. |
|
The buffer module is lazily required in the deepEqual checks. That is why there is the circular dependency. |
|
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. |
|
@refack I like the idea of using |
|
Lets see if it works - https://ci.nodejs.org/job/node-test-commit-linuxone/8136/ |
| } | ||
| return true; | ||
| } | ||
| return compare(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.
|
Still LGTM. |
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>
|
Landed in 9aa7093 |
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>
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>
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>
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>
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>
|
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 |
|
This is going to be much easier after the backport for #13862 has landed. I open a PR for that in a few minutes. |
|
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