Fix node_modules sourcemap config (which will fix VSCode debugging of CRA apps)

[justingrant]

This PR fixes #6044 and fixes #6296, which are sourcemap-related problems that make the VSCode debugger unable to debug code in node_modules of CRA apps. "Unable to debug" means:

• breakpoints set on node_modules code trigger on different code lines than where the breakpoint was set
• stepping into node_modules code will highlight the wrong line, and sometimes even the wrong module!
• selecting a particular frame in a call stack will highlight the wrong line in a node_modules file

The cause of this problem was a Babel config change in #5096 from Sept 2018:

create-react-app/packages/react-scripts/config/webpack.config.js

Lines 432 to 436 in 57ef103

	// If an error happens in a package, it's possible to be
	// because it was compiled. Thus, we don't want the browser
	// debugger to show the original code. Instead, the code
	// being evaluated would be much more helpful.
	sourceMaps: false,

The goal of that change was to force debuggers to show transpiled code instead of original source. Unfortunately, that's not what it did. Webpack still generates a sourcemap for node_modules (to see it, open https://localhost:3000/static/js/0.chunk.js.map in any CRA app in dev mode), but this sourcemap doesn't match the code that Babel generated for each module. Notably there are differences in whitespace (extra blank lines) and comments between the sourcemap code in sourcesContent and the original source. When VSCode tries to set a breakpoint or step into node_modules code, the sourcemap points to the wrong line in the webpack node_modules chunk.

BTW, this explains why the problem doesn't show up in the Chrome debugger, because the Chrome debugger only uses the inline sourcesContent code in the sourcemap-- it ignores the actual source files on disk. VSCode, however, will try to load the original source files from disk using the sources array in the sourcemap. This provides a better debugging experience-- for example, if you quit your debug session the same source files are still loaded, and you can set breakpoints before your app is running to debug startup behavior.

Anyway, this PR changes Babel config to ensure that Babel is correctly generating sourcemaps for node_modules code. Heads up: it will likely make build times longer, because the original change in #5096 made build times shorter.

To validate that the PR is working, use the following steps:

1. git clone https://github.com/justingrant/cra-sourcemap.git - this repo is unaltered CRA boilerplate code. The only changes I made was to add VSCode config files to make it easier for VSCode newbies to get a debug session going to repro the problem and validate the fix.
2. Open VSCode
3. Choose File->Add Folder to Workspace and choose the cra-sourcemap folder you cloned above
4. Open src/index.js in the IDE editor
5. Set a breakpoint on the call to ReactDOM.Render. If you don't know how to set a breakpoint in VSCode, one way to do it is to set the cursor on that line of code and press F9.
6. yarn start
7. Press F5 to start a debug session. It should start debugging immediately, but if you're asked, choose "Chrome" from the dropdown menu and then it will start.
8. Now refresh the browser. This should trigger the breakpoint you set above.
9. Click "Step Into", which is the down arrow on the debug toolbar. Or press F11.
   
10. You'll now be in some webpack boostrap module-loading code that you'll need to get out of, so press Step Into (F11) a few more times until the editor returns to index.js. Now it's actually ready to call into React code.
11. Now click Step Into one more time.

Expected (after PR is applied):
IDE highlights the first line of createElementWithValidation(), which is what ReactDOM.Render resolves to inside React's original source code.

Actual (current behavior):
A line of code is highlighted in the middle of a method-- it's clearly the wrong line.

BTW, to validate this PR I wrote a webpack plugin that compared the code in sourcesContent of 0.chunk.js.map to the actual code sitting on disk. I ran it on my main project and got 870 of 1029 source files don't match the source map static/js/0.chunk.js.map. Then I ran it against the PR configs and zero mismatches were reported.

Here's an excerpt from the plugin's output for current CRA configs, omitting the first 867 (!!!) mismatched files:

/Users/justingrant/Documents/hdev/h3/web/node_modules/webpack/buildin/global.js doesn't match sourcemap, starting at line 1, col 7
Original Line 1: var g;
     Map Line 1: var g; // This works in non-strict mode
Original Line 2: 
     Map Line 2: 

/Users/justingrant/Documents/hdev/h3/web/node_modules/webpack/buildin/module.js doesn't match sourcemap, starting at line 1, col 26
Original Line 1: module.exports = function(module) {
     Map Line 1: module.exports = function (module) {
Original Line 2:        if (!module.webpackPolyfill) {
     Map Line 2:   if (!module.webpackPolyfill) {

/Users/justingrant/Documents/hdev/h3/web/node_modules/zen-observable/lib/Observable.js doesn't match sourcemap, starting at line 7, col 33
Original Line 6: 
     Map Line 6: 
Original Line 7: var _createClass = function () { function defineProperties(target, props) { for (var i = 0; i < props.length; i++) { var descriptor = props[i]; descriptor.enumerable = descriptor.enumerable || false; descriptor.configurable = true; if ("value" in descriptor) descriptor.writable = true; Object.defineProperty(target, descriptor.key, descriptor); } } return function (Constructor, protoProps, staticProps) { if (protoProps) defineProperties(Constructor.prototype, protoProps); if (staticProps) defineProperties(Constructor, staticProps); return Constructor; }; }();
     Map Line 7: var _createClass = function () {
Original Line 8: 
     Map Line 8:   function defineProperties(target, props) {

WARNING: 870 of 1029 source files don't match the source map static/js/0.chunk.js.map

[justingrant]

I just noticed that @gordonmleigh pointed me back in March to exactly the solution implemented in this PR. But I didn't notice his comment. Arrrgh. Anyway, kudos to Gordon for figuring out the solution. I could have saved a lot of time by paying closer attention!

Gordon also had a related suggestion which is to add source-map-loader to webpack config. Doing this will make it possible to "drill down" while debugging into the original source of node_modules libraries which have themselves been transpiled and bundled. Unfortunately, source-map-loader has a bad problem currently which is that parent-relative links (e.g. ../src/foo.js) are not resolved properly. There's a pending PR from @Volune that fixes this problem (and improves performance and error messaging too) so I'm planning to wait until that PR lands in source-map-loader, and then I'll do a follow-up PR to see if we can get source-map-loader into CRA to make debugging even better.

BTW, if any CRA maintainers are reading this, could you unlock #6296 so I can close the loop for people who come to that issue looking for answers? I'd like to refer them to this PR. Done, thanks!

[morgs32]

Until this gets merged, should we be on an older version of react-scripts?
Right now this breaks error logging in bugsnag (maybe Sentry? I haven't checked)

[gaearon]

@justingrant Thanks for the PR. Sorry we haven't looked yet, it's been very busy few months. Do you have a sense of how much slower this makes the build?

[justingrant]

@gaearon - no worries, thanks for taking a look! In totally unscientific testing with my largest project on my dev machine (2015 MBP), I'm seeing about 10% slower npm start (measured from starting the command to the URLs being displayed in the terminal) with this PR enabled. Without the PR it's 11 seconds, with the PR it averaged a little over 12 seconds. For npm run build, the times were 21s with the PR, 17s without the PR, so about 24% more. You guys have access to much larger CRA projects than I do, so YMMV.

In a minimal project like the repro repo in this PR, the difference was also too small to accurately measure-- a few hundred msecs or less. Hot reloads on my biggest project were also too fast to reliably tell the difference between with and without the PR.

Given that performance-sensitive users already have a command-line option to disable sourcemaps completely, it seems like folks who are concerned about build times already have a way to speed things up much more than 10%. But newbies who want to fix bugs or to understand react or other libraries by debugging into them with VSCode (or any IDE whose debugger matches sourcemaps to files like VSCode does) have no easy options today regardless of perf impact. Given CRAs prime directive as the on-ramp to the React ecosystem, I'd argue for "it just works" being the default. Folks with big apps or who are obsessive about initial build time can (like they already can today) opt into faster builds by turning off sourcemaps. What do you think?

[justingrant]

I rebased to catch up with the last few months of commits. No changes on my end. Hope that this can make it into 3.2!

[ZKruse]

Looks like 3.2 shipped and this is in the 3.3 list. It would be awesome to see this in 3.2.1 to get it sooner though!

Myself and colleagues have been using 2.1.4 which seems to be the last version with fully working VSCode debugging (as noted here: #6074 (comment))

[mrmckeb]

Thanks a lot, this looks great!