Record relative dynamic-require paths #7350
Conversation
|
|
|
For maintainers only:
|
|
Thank you for your pull request! The most important CI builds succeeded, we’ll review the pull request soon. |
| var fs = require("fs"); | ||
| var path = require("path"); | ||
| var content = fs.readFileSync(path.join(__dirname, "records.json"), "utf-8"); | ||
| expect(content).toEqual(`{ |
There was a problem hiding this comment.
I could do... you're sure about that, though? None of the other configTestCases use snapshots AFAICT - I was trying to mimic, eg, https://github.com/webpack/webpack//blob/master/test/configCases/records/issue-2991/test.js#L2.
There was a problem hiding this comment.
I would also say a snapshot would be appropriate here. The other test was written before we used jest and we did have access to snapshot tests these days. If you want, you can change the other test to a snapshot too.
lib/util/identifier.js
Outdated
| @@ -12,6 +12,11 @@ const path = require("path"); | |||
| * @returns {boolean} returns true if path is "Absolute Path"-like | |||
| */ | |||
| const looksLikeAbsolutePath = maybeAbsolutePath => { | |||
| if (maybeAbsolutePath === "/^\\.\\/.*$/") { | |||
There was a problem hiding this comment.
Is this the only possible value?
There was a problem hiding this comment.
Actually, no. We could also get stringified regexps here from something like:
require.context("./dependencies", false, /\.js$/);Maybe I should just check if it starts and ends with a /, to determine if it's regexp-y. I couldn't find any cases where an absolute filepath would be passed in here with a trailing slash (eg "/src/") - what do you think?
There was a problem hiding this comment.
Threading strings starting and ending with / as RegExp, seems to be safe in my opinion. Every other absolute path is a resolved path and doesn't end with /.
lib/util/identifier.js
Outdated
| @@ -12,6 +12,11 @@ const path = require("path"); | |||
| * @returns {boolean} returns true if path is "Absolute Path"-like | |||
| */ | |||
| const looksLikeAbsolutePath = maybeAbsolutePath => { | |||
| if (maybeAbsolutePath === "/^\\.\\/.*$/") { | |||
There was a problem hiding this comment.
Threading strings starting and ending with / as RegExp, seems to be safe in my opinion. Every other absolute path is a resolved path and doesn't end with /.
| var fs = require("fs"); | ||
| var path = require("path"); | ||
| var content = fs.readFileSync(path.join(__dirname, "records.json"), "utf-8"); | ||
| expect(content).toEqual(`{ |
There was a problem hiding this comment.
I would also say a snapshot would be appropriate here. The other test was written before we used jest and we did have access to snapshot tests these days. If you want, you can change the other test to a snapshot too.
This avoids records like: "src/dependencies sync ../../../../^/.//.*$": "./src/dependencies sync recursive ^\\.\\/.*$" with the number of "../"s varying according to the depth of the folder you're building from. Fixes webpack#7339
jdelStrother
force-pushed
the
dynamic-requires
branch
from
219098d
to
6bf9479
Compare
webpack-bot
added
PR: review-outdated
and removed
PR: reviewed-changes-requested
labels
|
@jdelStrother Thanks for your update. I labeled the Pull Request so reviewers will review it again. @sokra Please review the new changes. |
|
Thanks |
|
I've created an issue to document this in webpack/webpack.js.org. |
What kind of change does this PR introduce?
This avoids records like:
"src/dependencies sync ../../../../^/.//.*$": "./src/dependencies sync recursive ^\\.\\/.*$"when using dynamic requires (eg
require("./foo/" + file)), with the number of "../"s varying according to the depth of the folder you're building from.Fixes #7339
Did you add tests for your changes?
Yes
Does this PR introduce a breaking change?
Don't believe so.
What needs to be documented once your changes are merged?
Nothing(?)
WDYT to this?