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

use sourcemaps to get a more accurate bundle-report #4463

Merged
merged 23 commits into from
Apr 30, 2020

Conversation

DeMoorJasper
Copy link
Member

@DeMoorJasper DeMoorJasper commented Apr 11, 2020

↪️ Pull Request

Closes #4368

This Pull Request is a work in progress to use sourcemaps to get a more accurate build report after minification and treeshaking.

It is not entirely accurate as our sourcemaps aren't entirely accurate or complete, but it's a lot better than before.

Before:
Screenshot 2020-04-11 at 13 19 32

After:
Screenshot 2020-04-11 at 13 18 47

@height
Copy link

height bot commented Apr 11, 2020

Link Height tasks by mentioning a task ID in the pull request title or description, commit messages, or comments.

You can also use "Close T-X" to automatically close a task when the pull request is merged.

@devongovett
Copy link
Member

Nice! Have you seen https://github.com/danvk/source-map-explorer? I believe the underlying library can be used standalone as well.

@DeMoorJasper
Copy link
Member Author

DeMoorJasper commented Apr 11, 2020

@devongovett wow I didn't know that already existed. Looks pretty cool.

However I don't think we should include an additional library for this as we've already done all the heavy lifting by writing so many utilities around handling sourcemaps, so this PR is basically just a simple loop through all the mappings.

Might be nice to look through how it's implemented though, will look through it. (just looked through and it's apparently super simple with the sourcemapconsumer from Mozilla, might be interesting to add something similar to eachMapping to @parcel/source-map at some point)

@wbinnssmith
Copy link
Contributor

In the "after" screenshot, kitchen-sink.[hash].js has a couple 0 byte assets — is that accurate?

@DeMoorJasper
Copy link
Member Author

@wbinnssmith it should be relatively accurate, it probably means there's no mappings with the origin to that asset.

@DeMoorJasper
Copy link
Member Author

@wbinnssmith just double checked and it's a bug with the sourcemap. When throwing it in the sourcemap visualiser it shows up as not found. But the size is incorrect and should be correct once the sourcemap is correct.

Screenshot 2020-04-14 at 21 06 16

Copy link
Member

@devongovett devongovett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should update the bundle analyzer with this too.

One question: what kind of performance cost does this add to large builds? Do we need to bring back the --detailed-report flag?

packages/core/utils/src/generateBuildMetrics.js Outdated Show resolved Hide resolved
packages/core/utils/src/generateBuildMetrics.js Outdated Show resolved Hide resolved
@DeMoorJasper
Copy link
Member Author

DeMoorJasper commented Apr 24, 2020

@devongovett I'm not sure about any performance cost, I doubt it would be a very large impact, it's just a relatively simple loop. Will check with the threejs benchmark.

Nvm apparently the threejs example is broken by scope-hoisting @parcel/packager-js: src/copy1/Three.js does not export '*' @mischnic is this a known issue?

@mischnic
Copy link
Member

is this a known issue?

Not really, but I was aware of how the threejs benchmark is failing. Investigating now...

@mischnic
Copy link
Member

If you replace export {copy1}; with console.log(copy1); as a workaround, the error should go away.

@DeMoorJasper
Copy link
Member Author

DeMoorJasper commented Apr 24, 2020

@devongovett I ran the threejs benchmark and it takes about 1-2 seconds on my machine to show the build report, so maybe it's a good idea to add the flag?

Or I could just log Generating bundle report... before the generation starts so nobody thinks Parcel froze or anything

@wbinnssmith
Copy link
Contributor

What should we do if someone wants scope hoisting but doesn't want source maps? Should we generate sourcemaps so we can create a meaningful report, but never write them as output?

cc @mischnic re: #4528

@mischnic
Copy link
Member

mischnic commented Apr 24, 2020

What should we do if someone wants scope hoisting but doesn't want source maps? Should we generate sourcemaps so we can create a meaningful report, but never write them as output?

I thought about that as well. I guess the question is if disabling sourcemaps completely (e.g. also for babel-parser) has a significant performance benefit.

we can create a meaningful report,

The same goes for some scope hoisting missing export diagnostics

@DeMoorJasper
Copy link
Member Author

DeMoorJasper commented Apr 25, 2020

@wbinnssmith It depends on how much anyone cares about performance. I think it saves us quite some time to not generate any sourcemaps. Maybe it should generate sourcemaps every time the --detailed-report flag is used. So in case anyone wants the detailed and accurate report they can just opt-in to it.

However as nobody probably wants this to be put into dist when using --no-source-maps we probably need to figure out a way to keep it in memory or cache in that case.

This report should still work without a sourcemap though, but it would be identical to before this PR...

Probably best to implement the --detailed-report flag in a seperate PR though as it'll probably be quite some work in case it would affect sourcemap generation

@devongovett devongovett merged commit 0fddcb1 into v2 Apr 30, 2020
@devongovett devongovett deleted the sourcemap-bundle-report branch April 30, 2020 03:41
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 this pull request may close these issues.

Use sourcemaps to report true asset sizes in bundles
4 participants