Conversation
|
For maintainers only:
|
da341a2 to
552f53a
Compare
| it("should create require", () => { | ||
| const require = _createRequire(import.meta.url); | ||
| expect(require("./a")).toBe(1); | ||
| expect(_createRequire(import.meta.url)("./c")).toBe(3); |
There was a problem hiding this comment.
TODO
add support of context. maybe contextify request?
| } | ||
| }; | ||
|
|
||
| parser.hooks.import.tap( |
There was a problem hiding this comment.
should we add parser option to enable such behaviour? (and enable it by default in target=node)
There was a problem hiding this comment.
Yes, we don't want to enable that by default in web target since it's a "node.js polyfill" which we have chosen to not provide by default.
93f44bf to
16f53cb
Compare
16f53cb to
34c4edc
Compare
sokra
left a comment
There was a problem hiding this comment.
Looks good, but I can't see where context is used when passing a string to createRequire.
Also it might make sense to rename context to issuer since context usually points to a directory in webpack context, while issuer would be the requesting module.
| if ( | ||
| arg.type === "MemberExpression" && | ||
| arg.object.type === "MetaProperty" | ||
| ) { | ||
| if ( | ||
| arg.object.meta.type === "Identifier" && | ||
| arg.object.meta.name === "import" && | ||
| arg.object.property.type === "Identifier" && | ||
| arg.object.property.name === "meta" && | ||
| arg.property.type === "Identifier" && | ||
| arg.property.name === "url" | ||
| ) { | ||
| // same module context | ||
| return false; |
There was a problem hiding this comment.
I think we evaluate import.meta to an identifier in evaluateExpression, so this special case isn't needed in my opinion.
You can use evaluated.isIdentifier() && evaluated.identifier === "import.meta.url" for that case in the code below
| } | ||
| }; | ||
|
|
||
| parser.hooks.import.tap( |
There was a problem hiding this comment.
Yes, we don't want to enable that by default in web target since it's a "node.js polyfill" which we have chosen to not provide by default.
| expect(require.cache).toBe(_require.cache); | ||
| expect(require.cache).toBe(_createRequire(import.meta.url).cache); | ||
| }); | ||
|
|
There was a problem hiding this comment.
Needs a test case when a string is passed to createRequire. e. g. createRequire(import.meta.url + "/../somewhere-else/file.js")
There was a problem hiding this comment.
import.meta.url + "/../somewhere-else/file.js" will not produce valid context. I have added support to new URL(...)
5d967db to
64778f0
Compare
64778f0 to
8df5134
Compare
# Conflicts: # declarations/WebpackOptions.d.ts # lib/config/defaults.js # schemas/WebpackOptions.check.js # schemas/WebpackOptions.json # test/Defaults.unittest.js # test/__snapshots__/Cli.basictest.js.snap # types.d.ts
|
Thanks |
What kind of change does this PR introduce?
closes #14228
Did you add tests for your changes?
yes
Does this PR introduce a breaking change?
no
What needs to be documented once your changes are merged?
nothing