Avoid relying on Node’s internals#6398
Conversation
`process.binding()` is not a public API, and should not be used. Luckily, Node recently introduced an API that does exactly what Webpack needs: https://nodejs.org/api/modules.html#modules_module_builtinmodules So use that instead and keep the old path as a fallback.
|
|
|
I am not sure why that Windows build failed, on Node 8 there really shouldn’t be any difference here, and certainly not any OS-dependent one. (I can still try to take a look on Windows later, but it might be nice to know whether this is a known issue or not.) |
|
I can reproduce the CI failures locally, but don't understand why this is happening... It's pretty weird. It gets even weirder, because adding a console.log actually fixes the problem: class NodeTargetPlugin {
apply(compiler) {
+ console.log();
new ExternalsPlugin("commonjs", builtins).apply(compiler);
}
} |
|
Just tried it on Windows and got the same issue - this is really weird. Also, this only seems to happen with Node >= 8.0.0, so there's a decent chance this is a bug in Node. A quick bisect using nightlies shows that the offending commit in Node would be somewhere in nodejs/node@54d3431...0b5191f I'll try to investigate a bit more! |
|
Oh, fun; this makes the test pass too: class NodeTargetPlugin {
apply(compiler) {
+ const d = new Date().getTime()
+ while (new Date().getTime() - d < 100);
new ExternalsPlugin("commonjs", builtins).apply(compiler);
}
}So it's probably an existing race condition in webpack or its tests. Okay, this is at least partly a Node bug: nodejs/node#18463 |
|
Great work on investigating this bug @addaleax |
|
@addaleax Thanks for your update. I labeled the Pull Request so reviewers will review it again. @ooflorent Please review the new changes. |
|
@sokra Thanks! I think I have a proper workaround here (that would be good to include, regardless of the original patch). Let's see what CI has to say. |
|
Wow... I wasn't expected this PR to result into node's bugfix! Great job 👍 |
|
@ooflorent I'm happy with whatever you prefer. Adding a TODO seems fine to me, and I agree that the workaround can be removed once the fix has made its way into Node 8, but without any kind of workaround I don't think this would pass CI. :/ |
|
|
Work around nodejs/node#18463 by detecting the conditions under which the bug occurs and performing a simple operation that resets the error state if necessary.
|
@ooflorent Yup. I've updated with that + a TODO comment, let me know if that's fine with you |
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
|
Thanks |
6 participants
What kind of change does this PR introduce?
Future-proofing / being a good OSS citizen.
Did you add tests for your changes?
No, but Webpack doesn’t seem to test current verions of Node in CI right now anyway.
Once Node 9 or above is in there, this is going to be tested automatically.
If relevant, link to documentation update:
N/A
Summary
process.binding()is not a public API, and shouldnot be used.
Luckily, Node recently introduced an API that does
exactly what Webpack needs:
https://nodejs.org/api/modules.html#modules_module_builtinmodules
So use that instead and keep the old path as a fallback.
Does this PR introduce a breaking change?
No.
Other information
Nope. :)