buffer: move setupBufferJS to internal - #16391
Conversation
Sorry, something went wrong.
|
CI: https://ci.nodejs.org/job/node-test-pull-request/10911/ And since this modifies process.binding('buffer'), CITGM: https://ci.nodejs.org/view/Node.js-citgm/job/citgm-smoker/1025/ |
Sorry, something went wrong.
There was a problem hiding this comment.
Not blocking, and up for debate:
Since this changes global state, maybe move this to node_bootstap, or explicitly require with a comment. Otherwise AFAICT it will run as a side effect of
node/lib/internal/bootstrap_node.js
Line 295 in 02a5267
Sorry, something went wrong.
There was a problem hiding this comment.
Hmm, I'd like to keep the caching and removing of setupBufferJS in the same place. Would a comment in bootstrap_node.js clarifying that requiring buffer also removes the setupBufferJS from the binding be sufficient to clarify? Something like:
// Note that this requires `lib/internal/buffer.js`, which removes `setupBufferJS`
// from the buffer binding.
global.Buffer = NativeModule.require('buffer').Buffer;
Sorry, something went wrong.
There was a problem hiding this comment.
Another viable option is to add this to bootstrap_node.js
// This require as side effect removes `setupBufferJS` from the buffer binding.
NativeModule.require('internal/buffer.js');
Sorry, something went wrong.
There was a problem hiding this comment.
Cool, I'll throw a comment and explicit require in. Stay tuned.
Sorry, something went wrong.
|
I've just realized the commit message is wrong. Fixing it now. |
Sorry, something went wrong.
Stashing it away in internal/buffer so that it can't be used in userland, but can still be used in internals.
jasnell
left a comment
There was a problem hiding this comment.
LGTM with green CI
Sorry, something went wrong.
|
Alright here's another round of CI and CITGM, since this has changed slightly as per @refack's review. CI: https://ci.nodejs.org/job/node-test-pull-request/10955/ |
Sorry, something went wrong.
|
CI failures are unrelated |
Sorry, something went wrong.
Stashing it away in internal/buffer so that it can't be used in userland, but can still be used in internals. PR-URL: #16391 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Stashing it away in internal/buffer so that it can't be used in userland, but can still be used in internals. PR-URL: nodejs/node#16391 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
|
Should this be backported to v8.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label. |
Sorry, something went wrong.
|
@gibfahn I think in order to avoid the circular dependency issue (https://github.com/nodejs/node/blob/v8.x-staging/lib/buffer.js#L1501-L1503) (not an issue on master/9.x) it may not make sense to backport this commit as it's implemented. That is, putting setupBufferJS into internal/buffer might not make sense on 8.x. |
Sorry, something went wrong.
Stashing it away in internal/buffer so that it can't be used in userland, but can still be used in internals. PR-URL: nodejs/node#16391 Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Timothy Gu <timothygu99@gmail.com> Reviewed-By: Refael Ackermann <refack@gmail.com> Reviewed-By: James M Snell <jasnell@gmail.com>
Stashing it away in internal/buffer so that it can't be used in userland, but can still be used in internals.
/cc @Fishrock123 @addaleax since we've previously talked about doing this.
Checklist
Affected core subsystem(s)
buffer