Add builtin modules with node: prefix #13311
Conversation
|
For maintainers only:
|
webpack-bot
added
PR: CI-ok
PR: reviewed-changes-requested
and removed
PR: unreviewed
labels
|
@sokra Ok thanks for the hint. Can you provide some guidance as to what a test case would look like? And where I would add this test case? |
|
@karlhorky You can put this here https://github.com/webpack/webpack/tree/master/test/configCases/target and add |
|
@karlhorky Thanks for your update. I labeled the Pull Request so reviewers will review it again. @sokra Please review the new changes. |
|
@alexander-akait ok I spent 10 minutes looking at the things in that folder, but I don't know what you mean. The structure of the test directory structures is hard for me to understand. I ended up just guessing that you mean doing something like this: |
| @@ -1,7 +1,7 @@ | |||
| it("should run", function() {}); | |||
|
|
|||
| it("should name define", function() { | |||
| var fs = require("fs"); | |||
| var fs = require("node:fs"); | |||
There was a problem hiding this comment.
Please add new tests, don't modify exists
There was a problem hiding this comment.
As I wrote above, I don't understand how to add new tests here - I tried :)
|
@karlhorky The most important CI builds failed. This way your PR can't be merged. Please take a look at the CI results from azure (1 errors / 1 warnings) and appveyor (failure) and fix these issues. |
|
So this feature is about to get more usage: the https://nodejs.org/api/esm.html -> link "node: Imports" (links with anchors don't scroll properly right now)
|
|
An alternative to the approaches in #13311 and #12693 would be to use |
|
Done here #12693, sorry for delay |
|
No worries, thanks for this! |
Closes #13290
What kind of change does this PR introduce?
Support
node:prefix for builtin modulesDid you add tests for your changes?
@alexander-akait what tests would you suggest here?
Does this PR introduce a breaking change?
I guess not, since this is now a feature of Node.js?
What needs to be documented once your changes are merged?
Not sure this needs to be documented, but happy to be corrected here.