ampproject / remapping

Remap sequential sourcemaps through transformations to point at the original source code
Apache License 2.0
109 stars 26 forks source link

Remapping when there's multiple sources #159

Closed JoostK closed 2 years ago

JoostK commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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

JoostK commented 2 years ago

Thanks, it works as expected.

jridgewell commented 2 years ago

Closing with https://github.com/babel/babel/pull/14246