-
Notifications
You must be signed in to change notification settings - Fork 29k
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.
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
buffer: fix deprecation warning emit #20163
Conversation
Due to npm using workers on Windows which inititate processes for code within node_modules, the current way of testing is a little too strict to catch all occurrences.
CI: https://ci.nodejs.org/job/node-test-pull-request/14396/ (You can also 👍 for fast-track.) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM for lack of a better idea
(/cc @mafintosh)
If possible I would like to get some fast-track approvals so @jasnell can get this in his RC build today. Currently we only have 1. Thanks everyone! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to fast-track
+1 to fast-track if it wasn’t clear. |
Due to npm using workers on Windows which inititate processes for code within node_modules, the current way of testing is a little too strict to catch all occurrences. PR-URL: #20163 Fixes: #20160 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Landed in d718d53. Thanks everyone! |
@jasnell this needs to be pulled into 10.0.0 today. Thanks! |
Can you please make sure it's added to the 10.0.0 milestone? I can't do so myself at the moment. |
@jasnell Yep, it's on there. Along with the other regression fix. |
Due to npm using workers on Windows which inititate processes for code within node_modules, the current way of testing is a little too strict to catch all occurrences. PR-URL: #20163 Fixes: #20160 Reviewed-By: Matteo Collina <matteo.collina@gmail.com> Reviewed-By: Colin Ihrig <cjihrig@gmail.com> Reviewed-By: Minwoo Jung <minwoo@nodesource.com> Reviewed-By: Anna Henningsen <anna@addaleax.net> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Due to npm using workers on Windows which inititate processes for code within node_modules, the current way of testing is a little too strict to catch all occurrences.
I understand that the previous implementation was there for a reason but as far as I can tell it's also likely to get a lot of other false positives than just the
npm install
issue — such as running code innode_modules/.bin/
.Fixes: #20160
Refs: https://github.com/npm/npm/blob/8452a9d9b231bc6c95745bb49a4b838ce11d3b9c/lib/install/action/extract.js#L22-L34
Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passes