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

Switch from cheap-module-source-map eval-source-map #4930

Merged
merged 1 commit into from
Sep 20, 2018

Conversation

jasonLaster
Copy link
Contributor

@jasonLaster jasonLaster commented Aug 27, 2018

Fixes issue

Switches webpack's devtool mode from cheap-module-source-map to source-map so that the mappings include column positions.

This enables variable and scope mappings in Firefox, which makes it easier to debug applications built with webpack and babel. This should fix the this mapping issue. It will also simplify the scopes pane by showing the "original" scopes and variables. It will also enable in-line code preview and console support when evaluating "original" expressions.

The incremental build times were 25% slower for source-map vs cheap-module-source-maps (500ms vs 400ms) on a simple app. I believe that webpack's cache means that these numbers would not be that different on a larger app.

Both modes had equivalent breakpoint support on page load and while the app was running.

Chrome

screen shot 2018-08-27 at 4 50 34 pm

screen shot 2018-08-27 at 4 50 24 pm

Firefox

image

image

@@ -77,7 +77,7 @@ module.exports = {
mode: 'development',
// You may want 'eval' instead if you prefer to see the compiled output in DevTools.
// See the discussion in https://github.com/facebook/create-react-app/issues/343
devtool: 'cheap-module-source-map',
devtool: 'source-map',

Choose a reason for hiding this comment

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

It's tough. I'd be curious to get thoughts from others. In my experience, source-map gets pretty slow for large bundles because it is a lot of work to regenerate the full map of the bundle. You can't cache things very well because any change to any file will force the recalculation of mappings for any file after the one that changed in the bundle. I usually go for eval-source-map for that reason.

@Pajn
Copy link
Contributor

Pajn commented Aug 28, 2018

If such an expensive option is chosen, source maps should probably be mad optional in dev as they already are in prod https://github.com/facebook/create-react-app/blob/3a51fde1bdd1841c156d14bfc08c8046f303b9ba/packages/react-scripts/config/webpack.config.prod.js#L98

I personally always disable source maps in the browser anyway as I've yet to see a case where they are more helpful than problematic.

@jasonLaster
Copy link
Contributor Author

Good point @loganfsmyth. I can look into eval-source-map when i tested it earlier, the mappings and breakpoints worked, so it was just a question of performance.

@jasonLaster jasonLaster force-pushed the source-map branch 2 times, most recently from f8e1039 to a891114 Compare September 19, 2018 16:55
@jasonLaster jasonLaster changed the title Switch from cheap-module-source-map source-map Switch from cheap-module-source-map eval-source-map Sep 19, 2018
@jasonLaster
Copy link
Contributor Author

jasonLaster commented Sep 19, 2018

I just tested several of the webpack devtool modes with the debugger.html format:

source-map: average compile times 1853.48 over 8 runs
inline-source-map: average compile times 1657.42 over 7 runs
eval-source-map: average compile times 604.69 over 13 runs
inline-cheap-source-map: average compile times 683.41 over 7 runs
cheap-module-source-map: average compile times 712.37 over 7 runs

It looks like, there were two sets of times (1.5 sec) (600 ms). eval-source-map is the fastest option that also includes column breakpoint information, which is needed for variable and scope mapping.

Thanks @loganfsmyth for the summary above about the benefits of eval-source-maps.

@Pajn I agree it could be nice to add a new option. It looks like eval-source-map is faster than cheap-module-source-map so this change probably doesn't have to block on the new option.

@Timer Timer added this to the 2.0.0 milestone Sep 19, 2018
@Timer
Copy link
Contributor

Timer commented Sep 19, 2018

Thanks for doing this research! Can you make sure the error overlay still works please?

@gaearon
Copy link
Contributor

gaearon commented Sep 19, 2018

@Timer I'm not sure @jasonLaster is familiar with what error overlay is

@Timer
Copy link
Contributor

Timer commented Sep 19, 2018

That's fair, @jasonLaster: if you edit App.js and add some code that causes a runtime exception, e.g.:

          <img
            onClick={() => {
              document.body()
            }}
            src={logo}
            className="App-logo"
            alt="logo"
          />

Running the dev server and triggering this exception should cause an overlay to appear:

If this doesn't work, can you please provide more details on where the sourcemaps are located and how to retrieve them?
I can provide more direction on how to fix this if the overlay isn't working.

@jasonLaster
Copy link
Contributor Author

Nice catch. Looks like it is missed

@jasonLaster
Copy link
Contributor Author

The travis failure looks like a node 10 timeout. The others pass

@jasonLaster
Copy link
Contributor Author

I think we'll need to update this logic:

https://github.com/facebook/create-react-app/blob/master/packages/react-error-overlay/src/utils/getSourceMap.js#L77-L96

but it shouldn't be a big deal... we do similar things on the debugger side to get the maps :)

@Timer
Copy link
Contributor

Timer commented Sep 19, 2018

I restarted the one job. Unfortunately, we don't have tests that make sure the error overlay is working so Travis won't catch this.

This file probably needs adjusted: https://github.com/facebook/create-react-app/blob/next/packages/react-error-overlay/src/utils/getSourceMap.js

edit: you beat me to it! Think you can take care of this? 😄

@jasonLaster
Copy link
Contributor Author

I'm running into a couple of issues getting the data for the error overlay:

A bit of context, with cheap-module-source-map the error stack locations are bundle.js with eval-source-map the locations are webpack original locations [1].

screen shot 2018-09-19 at 3 51 43 pm

screen shot 2018-09-19 at 3 50 52 pm

This has a couple of interesting effects

  1. we can no longer use the same approach: fetch the bundle given the original location, find the source map url, and load the source map for the application.

  2. if we set a better source map root, we can show a call stack based on the error stack. We could not get the surrounding lines though because we would not have the source map or the original source text.

  3. we might be able to have webpack dev server host each "built" file. This means that localhost:3000/static/js/src/App.js would resolve, which would let us fetch the source text and get the map, which would give us the surrounding text.

As an aside, I wish that there were a devtools extension api that would let you from content just get this data. We have it within the devtools client and would be happy to share :)

[1] eval-source-map each module in the bundle is included in an eval, which has its own sourceURL.

"use strict";
eval("<source contents>;n//# //# sourceURL=[module]\n//# sourceMappingURL=data:application/json;charset=utf-8;base64,eyJ2ZXJzaW9uIjozLCJz\n//#sourceURL=webpack-internal:///...")

@jasonLaster
Copy link
Contributor Author

jasonLaster commented Sep 20, 2018

@Timer and I paired on a solution, which lets us use eval-based sourcemaps and continues to support the error overlay.

There are a couple of benefits with eval-based sourcemaps

  1. They're faster to re-compile because each module has its own sourcemap.
  2. The sourcemap has column level mappings, which lets firefox compute original scopes and variables

The significant change is that there is no longer a single bundle.js map file, which the error overlay can use to map the error stack. The PR introduces a new webpack dev middleware, which lets the error overlay fetch the generated source and retrieve the sourcemap as it did before.

As a side note, the error overlay will not support errors that occur during render methods as bundle.js is hard coded there. This use case is already broken when webpack has uses chunks. The new middleware, is a good way of fixing the case with chunking as well.

One potential risk with this approach is that it does rely on a couple of implementation details with webpack and the dev-server. I think eventually it would be nice if the middleware were maintained from within the webpack project as it is likely useful for other projects that use eval-sourcemap and might want sourcemap diagnostics from within the runtime.


Firefox Scope & Variable Mapping

screen shot 2018-09-20 at 3 45 28 pm

index 7e261ca..0ab8fa1 100644
--- a/packages/react-scripts/template/src/App.js
+++ b/packages/react-scripts/template/src/App.js
@@ -1,6 +1,7 @@
 import React, { Component } from 'react';
 import logo from './logo.svg';
 import './App.css';
+import { dancer } from './emojis'

 class App extends Component {
   render() {
@@ -8,13 +9,12 @@ class App extends Component {
       <div className="App">
         <header className="App-header">
           <img src={logo} className="App-logo" alt="logo" />
+          <h1>{dancer}</h1>
           <a
diff --git a/packages/react-scripts/template/src/emojis.js b/packages/react-scripts/template/src/emojis.js
new file mode 100644
index 0000000..c335a60
--- /dev/null
+++ b/packages/react-scripts/template/src/emojis.js
@@ -0,0 +1,3 @@
+const dancer = "🕺";

@jasonLaster
Copy link
Contributor Author

jasonLaster commented Sep 20, 2018

hmm, i'm not sure why simple can't find the new module. simple passes locally for me.

screen shot 2018-09-20 at 4 54 26 pm

re: probably have to export it from the package.json :)

@@ -34,7 +34,11 @@ async function map(
});
await settle(
files.map(async fileName => {
const fileSource = await fetch(fileName).then(r => r.text());
const fetchUrl = fileName.startsWith('webpack-internal:')
? `/getInternalSource?fileName=${encodeURIComponent(fileName)}`
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we export this URL from a package? That way if we change it we don't need to update two places.
See https://github.com/facebook/create-react-app/blob/next/packages/react-dev-utils/launchEditorEndpoint.js

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, we should probably make this more in line with the other url, i.e. /__get-internal-source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/__get-internal-source.

👍

Can we export this URL from a package?

I'm a bit worried about importing that module in react-error-overlay. It looks like react-dev-utils might be a new dependency.

@jasonLaster
Copy link
Contributor Author

jasonLaster commented Sep 21, 2018

It looks like chrome stable and safari fail to show the error overlay with eval-source-map. Based on that, I think we should likely switch back to cheap-module-source-map for the time being.

  • Chrome canary works and i believe they've fixed the underlying issue. I have not looked into safari, but error.stack is populated and they do not have the same cors issue.
  • It would be nice to leave the middleware in so that users can choose to use eval-source-map and still use the overlay in firefox. We could also turn on eval-source-map by default if firefox is the default browser. [2]
  • If we want to use eval-source-map sooner, one other option is to format the source url with relative paths e.g. ./template/src/App.js instead of webpack-internal://./template/src/App.js. This does not currently work in Firefox, but is fixable. It does work fairly well in chrome which is encouraging.

[2] (process.env.BROWSER || '').toUpperCase().includes('FIREFOX')

zmitry pushed a commit to zmitry/create-react-app that referenced this pull request Sep 30, 2018
@lock lock bot locked and limited conversation to collaborators Jan 19, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants