Skip to content
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

Remapping when there's multiple sources #159

Closed
JoostK opened this issue Feb 5, 2022 · 4 comments · Fixed by babel/babel#14246
Closed

Remapping when there's multiple sources #159

JoostK opened this issue Feb 5, 2022 · 4 comments · Fixed by babel/babel#14246

Comments

@JoostK
Copy link

JoostK commented Feb 5, 2022

First, thank you for such a high quality piece of software for source map merging.

With the adoption of this project within Babel 7.17, we're seeing build errors (angular/angular#44972) originating in this project in combination with Angular's linker transform, a Babel plugin that transforms JS source files to become fully AOT compiled for the Angular runtime to execute.

For context, the Angular linker compiles certain template string literals into Angular template functions. Angular templates can have originally been authored in external template files, and the template string literal is mapped into such an external file (typically an .html file). The template string literal in JS might contain escape sequences different to what the original external template contained, which means that source positions in the template literal string may be slightly different to their positions in the external template. To avoid these differences, the Angular linker actually attempts to read the original HTML template from the input source map and then uses that source text as template parsing source, after which it generates Babel AST with a SourceLocation.filename corresponding with that external template source. This will subsequently cause Babel's output map to contain an extra source (or any arbitrary amount of extra sources, really).

Babel used to ignore this case silently, per this logic. This condition is no longer present since babel/babel#14209, so an output source map with multiple sources now causes a hard Transformation map 0 must have exactly one source file error that fails the build. We can workaround this by disabling the Angular's linker plugin sourceMap option so it won't parse external templates using the input source map, but perhaps the condition should be reintroduced in Babel to avoid this breakage (although I realize that the linker's output map never behaved as desired as it wasn't merged).

I am opening this issue here as I'd be interested to see if/how we could 1) support multiple sources in this project, or 2) if you think our current approach is invalid and whether you have suggestions for adapting the Angular linker to deal with this differently. One alternative approach we explored was to record additional parsing offsets due to escape sequences, such that template parsing could take these into account (as the template string can't be assigned multiple source map segments, at least not if it's just a single template literal) but using the original sources from the input source map was far easier and resulted in simpler generated code.

FYI, the Angular compiler has its own source map merging logic which doesn't have the limitation of a single source, I believe.

As for a potential fix in Babel (at least for v7), please let me know if you think that re-adding the single-sources condition sounds reasonable, or if we should implement a workaround in the Angular's linker (which would currently mean to disable the linker's sourceMap option by default)

@jridgewell
Copy link
Collaborator

Thanks for the bug report! remapping actually supports multi-file sourcemaps, but the code I added to babel is using the single-source API. I wasn't aware that babel could generate a multi-file sourcemap, I thought it was always one to one so I deleted the Babel old code without really thinking about it.

I'm looking into a the repo case in the linked issue, and I'll figure out how to update Babel so that this is supported again.

jridgewell added a commit to jridgewell/babel that referenced this issue Feb 6, 2022
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.
jridgewell added a commit to jridgewell/babel that referenced this issue Feb 6, 2022
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.
jridgewell added a commit to jridgewell/babel that referenced this issue Feb 6, 2022
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.
@jridgewell
Copy link
Collaborator

Bug fix in babel at babel/babel#14246. This actually adds support for merging multi-source output maps, instead of silently failing.

@JoostK
Copy link
Author

JoostK commented Feb 6, 2022

Thanks, it works as expected.

nicolo-ribaudo pushed a commit to babel/babel that referenced this issue Feb 6, 2022
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.
@jridgewell
Copy link
Collaborator

Closing with babel/babel#14246

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants