Remove source-map from Rollup bundle - #2055
Conversation
guybedford
left a comment
There was a problem hiding this comment.
This looks great!
With the high performance work going on in the source maps library, I do wonder if we'd be better off trying to utilize this project more though in Rollup, but haven't investigated the source maps fully enough to be able to judge if there are other places it might be beneficial to utilize. Worth thinking about though.
| line: location.line, | ||
| column: location.column | ||
| }); | ||
| const line = sourcemap.mappings[location.line - 1]; |
There was a problem hiding this comment.
Could line be undefined here?
There was a problem hiding this comment.
Added more checks in a follow-up commit.
| let locationFound = false; | ||
|
|
||
| for (const segment of line) { | ||
| if (+segment[0] >= location.column) { |
There was a problem hiding this comment.
Is the + coercion really necessary?
There was a problem hiding this comment.
Yes, it complains about values being strings. The original type issue here is that source map objects are typed as RawSourceMap, but mappings is actually decoded into arrays (while TypeScript assumes it's a single string). I'd suggest fixing the typing here in a separate type-cleaning PR because a proper fix might be quite involved.
Note that the new version of Since |
| import ModuleScope from './ast/scopes/ModuleScope'; | ||
| import { encode } from 'sourcemap-codec'; | ||
| import { RawSourceMap, SourceMapConsumer } from 'source-map'; | ||
| import { RawSourceMap } from 'source-map'; |
There was a problem hiding this comment.
If we inline the RawSourceMap type we could remove source-map as a dependency entirely too.
There was a problem hiding this comment.
It's still used in tests, and it might be better to keep it there because it's a "reference" implementation, which is good for verifying source maps.
| let locationFound = false; | ||
|
|
||
| if (line !== undefined) { | ||
| for (const segment of line) { |
There was a problem hiding this comment.
Could we make this for (const segment of <[number, number, number, number][]>line) to get the types to work out without the coersions then?
There was a problem hiding this comment.
I tried something like this but TypeScript errored with "can't coerce a string into an array of numbers".
There was a problem hiding this comment.
Then defining line above as const line: any = ... could work along with the for typing. Then we can further look at fixing this up in due course.
|
Sure, sounds like a good start to me. Do we have any performance numbers for this change? |
I didn't measure because this only happens on bundle errors, which is rare. It should be fast enough. |
Sure! |
|
@guybedford removed the coercions with |
|
Looks good. Curious finding: When I measure bundling performance now, the first tree-shaking runs seem to be significantly and consistently slower while graph analysis and other earlier steps seem to be faster by at least the same amount. No idea what could be behind this as the changed code is not even used. |
Perhaps the first, reference perf.json was an outlier (faster than usual)? I think this happened to me once too. |
|
I would not think so as I was doing 10 runs and discarding 5 outliers in both cases (using |
While digging through the source map generation, I noticed that the
source-mapdependency, which was only used for locating errors, can be easily replaced but usingmappingsdirectly. This makes the final Rollup bundle ~10% smaller.