-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Remove source-map from Rollup bundle #2055
Conversation
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.
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.
src/Module.ts
Outdated
line: location.line, | ||
column: location.column | ||
}); | ||
const line = sourcemap.mappings[location.line - 1]; |
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.
Could line
be undefined here?
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.
Added more checks in a follow-up commit.
src/Module.ts
Outdated
let locationFound = false; | ||
|
||
for (const segment of line) { | ||
if (+segment[0] >= location.column) { |
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 the +
coercion really necessary?
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.
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 |
@@ -13,8 +13,7 @@ import extractNames from './ast/utils/extractNames'; | |||
import enhance from './ast/enhance'; | |||
import clone from './ast/clone'; | |||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we inline the RawSourceMap
type we could remove source-map
as a dependency entirely too.
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.
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.
src/Module.ts
Outdated
let locationFound = false; | ||
|
||
if (line !== undefined) { | ||
for (const segment of line) { |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 |
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.
LGTM
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-map
dependency, which was only used for locating errors, can be easily replaced but usingmappings
directly. This makes the final Rollup bundle ~10% smaller.