Skip to content
This repository has been archived by the owner on May 17, 2019. It is now read-only.

Change webpack dev sourcemap strategy to cheap-eval-source-map #758

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

derekjuber
Copy link
Contributor

@derekjuber derekjuber commented Apr 12, 2019

Addresses WPT-1864.

Our previous source map strategy was not bundling all of the individual source map files in node_modules into the final vendor.js.map file. By my rough estimations less than 25% of our fusion plugins were being added to the final generated mapping file. I cannot say why as attempting to track down the reasons didn't really lead anywhere after hours of learning about how this stuff works.

I started investigating alternative source map strategies and basically any strategy that allows source map debugging in original code would break. Anything else that uses generated or transformed code was fine though. So I picked the best one for dev based on that with fast build and rebuild speeds.

image
image
image

Our previous pick was:

image
image
image

Going by the chart it seems like our previous pick wasn't the best? Webpack doesn't recommend it for dev or production but only for special situations which I am unaware of.

An alternative approach would be to figure out why original source source maps aren't working. The documentation says this depends on loader support so something could be wrong with our loaders. I'm ok with going down that route if we don't want to settle with transpiled code source maps.

Personally it doesn't bother me that much since I can recognize what the code is doing even if it was transpiled but I can see the arguments for preserving the original code for maximum debugability.

Update: Forgot to add that this does remove the external js.map file for dev and instead inlines all of the source mappings into the generated vendor.js bundle. File size goes from about 2 MB to 5 MB and beyond. I don't see this as an issue in dev though since this is all local.

@derekjuber derekjuber requested a review from rtsao April 12, 2019 22:02
@rtsao
Copy link
Member

rtsao commented Apr 12, 2019

For what it's worth, both create-react-app and Next.js both use cheap-module-source-map.

My understanding is that source maps with webpack is still something of an unsolved problem in that no option really works perfectly with Chrome DevTools.

Our previous source map strategy was not bundling all of the individual source map files in node_modules into the final vendor.js.map file.

Are you referring to the source map files already in node_modules such as: ./fusion-plugin-rpc/dist/browser.es2017.es.js.map?

Honestly, I'm not even totally sure we'd want to consume those node_module source maps in Fusion, since the actual code bundled by webpack would be ./dist/browser.es2017.es.js and not ./src/xyz.js.

Copy link
Contributor

@KevinGrandon KevinGrandon left a comment

Choose a reason for hiding this comment

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

Not sure if @rtsao has open requests here, but I'm seeing some failures in CI that do not manifest on master. Wondering if this is causing some weirdness or we have weird tests. Requesting changes for now to clear the queue, but feel free to re-request review whenever tests are addressed.

@derekjuber
Copy link
Contributor Author

@rtsao: Hmm. The fact that the other two frameworks use cheap-module-source-map does give me some pause. Though it looks like there was an effort to change create-react-app to something else but it was reverted due to unfortunate Chrome and Safari issues.

See facebook/create-react-app#4930 and facebook/create-react-app#5059.

Do we not want to ingest node module source map files? I probably need to dig through the splitChunks source code but the way I thought this would work for the vendor bundle would be that Webpack crawls through node_modules, concats all of the dist/ files and then concats the corresponding source map file into the vendor js.map file (if it exists, I guess it would have to manually create the mappings for node modules that do not distribute the files).

I'll take another pass at this but cheap-eval-source-map worked great when I tested this but what I couldn't confirm was if this broke something else.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants