-
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Switch to @ampproject/remapping
to merge source maps
#14209
Conversation
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/50972/ |
// but eachMapping returns mappings with absolute paths. To avoid that | ||
// incompatibility, we leave the sourceRoot out here and add it to the | ||
// final map at the end instead. | ||
// This is a bit hack. Remapping will create absolute sources in our |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it true for @ampproject/remapping
too? In the readme, the helloworld.js
doesn't seem to be transformed to absolute.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately so: https://github.com/ampproject/remapping/blob/fcc9f442f43249389dc946cbe8bb3736a8033d6a/src/source-map-tree.ts#L115.
In the readme, the helloworld.js doesn't seem to be transformed to absolute.
The readme example doesn't have a sourceRoot
. If one were included, say src
, then the output would partially match { sourceRoot: undefined, sources: ['src/helloworld.js'] }
.
Could you verify if the failing tests are correct? |
I actually get 0 failures on my local, so I'm still trying to debug what's happening. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The changed sourcemaps here all refer to the same var foo = () => 4;
input, which only has 2 mappings:
var foo = () => 4;
^^^^^^^^^
^^^^^^^^^
The old code would create new mappings based on the transformed map, but these don't actually point to a sane spot in the original file. See how when you hover over foo
on the left, it hightlights var foo =
on the right? This is because the foo
mapping is pointing to line 1 column 0, not column 4 (where foo
exists in the original code). If it did, it would correctly highlight the foo
identifier on the right side.
The new output doesn't generate a useless mapping for foo
. But, if the input sourcemap were higher fidelity (with mappings at identifiers, syntax, etc), it would.
@@ -540,7 +540,7 @@ describe("api", function () { | |||
column: 4, | |||
}), | |||
).toEqual({ | |||
name: null, | |||
name: "Foo", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "Foo" name doesn't exist in the input sourcemap, but does in the transformed sourcemap. Remapping will use the input's if it existed, but falls back to the transformed one if not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@ampproject/remapping
to merge source maps
What is happing here is that when using the linker on `@angular/material` the sourcemap will contain multiple sources example. Babel now uses [remapping](https://github.com/ampproject/remapping/) to merge sourcemaps, see: babel/babel#14209. This doesn't support multiple sources which the linker produduces. Previously, Babel failed silently and didn't generate a sourcemap angular/angular#42769 at all. Linker produced sourcemap ```js [ '/packages/web-app-edit/Users/cli-reproductions/monorepo-new/node_modules/@angular/material/fesm2020/card.mjs', '/packages/web-app-edit/Users/src/material/card/card.html', '/packages/web-app-edit/Users/src/material/card/card-header.html', '/packages/web-app-edit/Users/src/material/card/card-title-group.html' ] ``` Will cause the below error during merging ``` Transformation map 0 must have exactly one source file. ``` Related to - angular/angular#42769 - angular/angular#44972 Addresses CI failure - https://app.circleci.com/pipelines/github/angular/angular/42281/workflows/e060088b-5963-43b0-b6fc-b4ddd8855bee/jobs/1116551
When using the linker on `@angular/material` or packages which contain external sourcemap. The sourcemap will contain multiple sources example. Babel now uses [remapping](https://github.com/ampproject/remapping/) to merge sourcemaps, see: babel/babel#14209. This doesn't support multiple sources which the linker produduces. Previously, Babel failed silently and didn't generate a sourcemap angular/angular#42769 at all. Linker produced sourcemap ```js [ '/packages/web-app-edit/Users/cli-reproductions/monorepo-new/node_modules/@angular/material/fesm2020/card.mjs', '/packages/web-app-edit/Users/src/material/card/card.html', '/packages/web-app-edit/Users/src/material/card/card-header.html', '/packages/web-app-edit/Users/src/material/card/card-title-group.html' ] ``` Will cause the below error during merging ``` Transformation map 0 must have exactly one source file. ``` Related to - angular/angular#42769 - angular/angular#44972 Addresses CI failure - https://app.circleci.com/pipelines/github/angular/angular/42281/workflows/e060088b-5963-43b0-b6fc-b4ddd8855bee/jobs/1116551
When using the linker on `@angular/material` or packages which contain external sourcemap. The sourcemap will contain multiple sources example. Babel now uses [remapping](https://github.com/ampproject/remapping/) to merge sourcemaps, see: babel/babel#14209. This doesn't support multiple sources which the linker produduces. Previously, Babel failed silently and didn't generate a sourcemap angular/angular#42769 at all. Linker produced sourcemap ```js [ '/packages/web-app-edit/Users/cli-reproductions/monorepo-new/node_modules/@angular/material/fesm2020/card.mjs', '/packages/web-app-edit/Users/src/material/card/card.html', '/packages/web-app-edit/Users/src/material/card/card-header.html', '/packages/web-app-edit/Users/src/material/card/card-title-group.html' ] ``` Will cause the below error during merging ``` Transformation map 0 must have exactly one source file. ``` Related to - angular/angular#42769 - angular/angular#44972 Addresses CI failure - https://app.circleci.com/pipelines/github/angular/angular/42281/workflows/e060088b-5963-43b0-b6fc-b4ddd8855bee/jobs/1116551
Surprisingly, Babel allows a transformer to mark the source file of a node to allow it to be sourced from any file. When this happens, the output sourcemap will contain multiple `sources`. I didn't realize this when I created babel#14209, and this `remapping` will throw an error if the output map has multiple sources. This can be fixed by using `remapping`'s graph building API (don't pass an array). This allows us to return an input map for _any_ source file, and we just need some special handling to figure out which source is our transformed file. Fixes ampproject/remapping#159.
Surprisingly, Babel allows a transformer to mark the source file of a node to allow it to be sourced from any file. When this happens, the output sourcemap will contain multiple `sources`. I didn't realize this when I created babel#14209, and this `remapping` will throw an error if the output map has multiple sources. This can be fixed by using `remapping`'s graph building API (don't pass an array). This allows us to return an input map for _any_ source file, and we just need some special handling to figure out which source is our transformed file. This actually adds a new feature, allowing us to remap these multi-source outputs. Previously, the merging would silently fail and generate a blank (no `mappings`) sourcemap. That's not great. The new behavior will properly merge the maps, provided we can figure out which source is the transformed file (which should always work, I can't think of a case it wouldn't). Fixes ampproject/remapping#159.
Surprisingly, Babel allows a transformer to mark the source file of a node to allow it to be sourced from any file. When this happens, the output sourcemap will contain multiple `sources`. I didn't realize this when I created babel#14209, and this `remapping` will throw an error if the output map has multiple sources. This can be fixed by using `remapping`'s graph building API (don't pass an array). This allows us to return an input map for _any_ source file, and we just need some special handling to figure out which source is our transformed file. This actually adds a new feature, allowing us to remap these multi-source outputs. Previously, the merging would silently fail and generate a blank (no `mappings`) sourcemap. That's not great. The new behavior will properly merge the maps, provided we can figure out which source is the transformed file (which should always work, I can't think of a case it wouldn't). Fixes ampproject/remapping#159.
Surprisingly, Babel allows a transformer to mark the source file of a node to allow it to be sourced from any file. When this happens, the output sourcemap will contain multiple `sources`. I didn't realize this when I created #14209, and this `remapping` will throw an error if the output map has multiple sources. This can be fixed by using `remapping`'s graph building API (don't pass an array). This allows us to return an input map for _any_ source file, and we just need some special handling to figure out which source is our transformed file. This actually adds a new feature, allowing us to remap these multi-source outputs. Previously, the merging would silently fail and generate a blank (no `mappings`) sourcemap. That's not great. The new behavior will properly merge the maps, provided we can figure out which source is the transformed file (which should always work, I can't think of a case it wouldn't). Fixes ampproject/remapping#159.
…ty and performance of babel sourcemaps With this change we remove the workaround for fidelity and performance of babel sourcemaps as this is no longer needed as Babel now uses `@ampproject/remapping` to merge sourcemaps. See babel/babel#14209 for more context.
Fixes #1, Fixes #2
This switches us from the custom-build source map merging to
@ampproject/remapping
. This should result in a considerably faster merges with reduced memory usage. Eg, angular/angular-cli#21779 found that using the old method caused OOM errors when the input/output maps were large enough.