Add applySourceMap #3
Conversation
src/gen-mapping.ts
Outdated
| if (sourceRoot != null) { | ||
| sourceFile = relative(sourceRoot, sourceFile); | ||
| } |
There was a problem hiding this comment.
So the expectation is that the sourceFile is always fully resolved, and we need to make it back into an unresolved path?
There was a problem hiding this comment.
My understanding is that the new map needs to be resolved relative to the existing sourcemap.
Removing this block will break all the tests with relative paths.
I've also checked locally with my pending PR to postcss and that's the same behaviour as source-map-js
|
|
||
| const sourceIndex = get(sources, source); | ||
| if (sourceIndex !== undefined) { | ||
| sourcesContent[sourceIndex] = content; | ||
| } |
There was a problem hiding this comment.
While not directly related to applySourceMap I've seen while preparing my PR to postcss that setSourceContent accepts to add any source content even for unknown sources.
However this would break if the setSourceContent is called before adding mappings and I don't know if that's needed or not.
source-map-js has a different approach where they store all source contents by Map<path, content> and create the sourcesContent when transforming to an encoded map, effectively dropping the unneeded sources.
What do you think is the best approach?
There was a problem hiding this comment.
However this would break if the setSourceContent is called before adding mappings and I don't know if that's needed or not.
Can you describe what you mean? setSourceContent should create a new index, and if a mapping is ever added with that source filename, that index will be used. Sourcemaps mappings don't need to be monotonic to the source locations.
source-map-js has a different approach where they store all source contents by Map<path, content> and create the sourcesContent when transforming to an encoded map, effectively dropping the unneeded sources.
I think this depends on the answer to the previous question. For the general case, I think it's fine to add source content for sources that never receive a mapping (and it keeps the library fast). But I'm not sure of a situation that your code will do that.
There was a problem hiding this comment.
But I'm not sure of a situation that your code will do that.
Yeah sorry I could have elaborated from the start. I have a test case in postcss that has a sourcemap for /root/dir/a.css.
Then we receive a call for setSourceContent('file:///root/dir/a.css' which essentially is the same file, but with a different path.
On one side, there is the fact that the call itself is incorrect, since the path we know is /root/... receiving file:///root won't work. The current code however happily accepts to add the sourceContent for it even if there is no mapping for it.
In the case of source-map-js it accepts the source content, but doesn't output it if there is no mapping, and thus no entry in .sources.
setSourceContentshould create a new index, and if a mapping is ever added with that source filename, that index will be used.
I agree with this, but on the other hand, I'm not sure that the map should have a sourceContent for something that's never mapped
For the general case, I think it's fine to add source content for sources that never receive a mapping (and it keeps the library fast).
Agreed, but this could be done in a map, and resolved when doing the toEncodedMap, this would reduce the number of lookups and do it only once when exporting the sourcemap
|
The changes to switch to |
| // If the traced mapping points to a sourceless segment, we need to truncate the | ||
| // original to also be sourceless. | ||
| (seg as number[]).length = 1; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This is the last block that isn't covered by tests.
Since this was introduced by your PR to my PR, I have to admit I'm not sure what case it covers and what input would cover this test. do you have an example ?
Hello,
as discussed in jridgewell/source-map#2
This PR adds the
applySourceMapbased on the implementation you proposed.I will add some comments on this PR on the places where I'm not sure my implementation is optimal.
Also, I'm not a big expert in licenses, some pieces of this implementation (the
utils.tsfile and the tests) come fromsource-map-js. its License seems to say that we need to add the copyright notice to the original work.I would have preferred to not copy this code, but the tests are very valuable by themselves and the utilities are useful as well. What's the best course of action here?