fix: improve inputSourceMap URL handling#17999
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/61556 |
|
commit: |
There was a problem hiding this comment.
Pull request overview
Improves resolution of inputSourceMap URLs that contain .., by restricting reads to the closest ancestor package.json directory (or options.root, whichever is closer). The source-map reading logic is moved out of normalize-file.ts into a new platform-conditional module so the browser build can stub it out.
Changes:
- New helper
read-input-source-map-file.ts(with a browser variant that throws) that guards reads against escaping the package/root boundary usingfind-up-simple. normalize-file.tsnow delegates to that helper via a#transformation/read-input-source-map-filesubpath import.- New test suite and many fixtures covering downward, upward-within-root/package, and upward-escaping cases.
Reviewed changes
Copilot reviewed 15 out of 40 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/babel-core/src/transformation/read-input-source-map-file.ts | New helper that resolves and validates the source-map path against package/root boundary |
| packages/babel-core/src/transformation/read-input-source-map-file-browser.ts | Browser stub that throws |
| packages/babel-core/src/transformation/normalize-file.ts | Replaces inline fs read with helper call |
| packages/babel-core/package.json | Adds find-up-simple dep and subpath imports entry |
| packages/babel-core/test/input-source-map-resolution.js | New test suite covering boundary scenarios |
| packages/babel-core/test/fixtures/input-source-map-resolution/** | Fixture trees for each scenario |
| yarn.lock | Lockfile update for find-up-simple@^1.0.1 |
Files not reviewed (3)
- packages/babel-core/test/fixtures/input-source-map-resolution/invalid-relative-up-across-root/root/sub/input.js: Language not supported
- packages/babel-core/test/fixtures/input-source-map-resolution/relative-down-into-node-modules/input.js: Language not supported
- packages/babel-core/test/fixtures/input-source-map-resolution/relative-down/input.js: Language not supported
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "browser": "./src/transformation/read-input-source-map-file-browser.ts", | ||
| "default": "./src/transformation/read-input-source-map-file.ts" | ||
| }, | ||
| "types": "./lib/index.d.ts", |
There was a problem hiding this comment.
The types maps to ./lib/index.d.ts because the bundle-dts step bundles every declarations into lib/index.d.ts so the published file does not contain ./lib/transformation/read-input-source-map-file.d.ts.
| const inputMapPath = path.resolve(path.dirname(filename), inputMapURL); | ||
| if (inputMapURL.includes("..")) { | ||
| const inputPackageJSONPath = findUpSync("package.json", { | ||
| cwd: path.dirname(filename), | ||
| stopAt: root, | ||
| }); |
There was a problem hiding this comment.
The default value of root is cwd and generally the input filename will be within cwd, or within root for monorepo's usage. If user misconfigured root, that means they are intentionally compiling something out of the monorepo boundary, in this case I don't think we should do anything.
| function getInputMapPath( | ||
| filename: string, | ||
| root: string, | ||
| inputMapURL: string, | ||
| ): string | null { | ||
| const inputMapPath = path.resolve(path.dirname(filename), inputMapURL); | ||
| if (inputMapURL.includes("..")) { |
| relativeToInputFileDir.startsWith("..") || | ||
| path.isAbsolute(relativeToInputFileDir) | ||
| ) { | ||
| const inputPackageJSONPath = findUpSync("package.json", { |
There was a problem hiding this comment.
For Babel 7, can we manually iterate up the file system?
There was a problem hiding this comment.
I plan to start from https://github.com/sindresorhus/find-up-simple/blob/f10133c55dcbf36f84a246c6f1bbfed178dcb774/index.js#L36 and shrink functions that we don't need, so it would be something like find-up-simple-node-6. We could manually iterate, too but then we will have to add extra tests for the different implementation.
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
JLHwung
force-pushed
the
improve-input-source-map-handling
branch
from
b4bc4d6 to
05dd840
Compare
liuxingbaoyu
left a comment
liuxingbaoyu
left a comment
There was a problem hiding this comment.
The .map extension might be an exception to skip the check. (Let's see if anyone complains about this check.)
| debug( | ||
| `discarding input sourcemap "${inputMapPath}" outside of package root "${inputFileRoot}"`, | ||
| ); |
There was a problem hiding this comment.
Users seem to be unable to see this message by default, which I'm worried will cause confusion.
There was a problem hiding this comment.
The input sourcemap merging errors, such as invalid source map format, are printed to the debug as well, since this branch will only be reached when there is parent traversal in the source map url, which is a pretty rare use case, I think it is fine to leave it as-is. We can improve in the future when there is user feedback.
There was a problem hiding this comment.
I recall we previously discussed printing out invalid source map warning messages in Babel 8. :)
There was a problem hiding this comment.
OK. I think that should be addressed in another PR.
JLHwung
added
PR: Bug Fix 🐛
|
Docs PR is ready, ptal: babel/website#3204 |
In this PR we improve the handling of source map url, especially for relative URL containing
...For these URLs, Babel now will find the closest
package.jsondirectory oroptions.root, whichever is closer, and only read the source map file if it comes from that directory.I will prepare a backport PR if we agree on the current design. The backport will be non-trivial as we have to switch
find-up-simplefor[email protected]with thestopAtsupport and then manually polyfill the syntax for Node.js 6.