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

Question: Building react with sourcemaps #14361

Closed
nprasath002 opened this issue Nov 30, 2018 · 22 comments
Closed

Question: Building react with sourcemaps #14361

nprasath002 opened this issue Nov 30, 2018 · 22 comments

Comments

@nprasath002
Copy link

How do I build react with source maps?

I am in master branch with the latest code. I tried setting rollup config to sourcemap: true and setting "sourceMaps": true in .babalrc. I am getting the following error.

Sourcemap is likely to be incorrect: a plugin was used to transform files, but didn't generate a sourcemap for the transformation. Consult the plugin documentation for help

I want to generate source maps so that I can set breakpoints in the actual es6 package code and step through it.

@nprasath002 nprasath002 changed the title Building react with sourcemaps Question: Building react with sourcemaps Nov 30, 2018
@markerikson
Copy link
Contributor

Per discussion in Reactiflux, this shouldn't require any special builds of React. The app build setup is generally responsible for generating sourcemaps of any libraries that are being used at build time. I just tried starting up a CRA app in dev mode, and I can both see the original-ish source of react.development.js in the DevTools and set breakpoints there.

@karmadotcom
Copy link

What is the real issue here

@nprasath002
Copy link
Author

To add more info to the question when adding a breakpoint here
https://github.com/facebook/react/blob/master/fixtures/packaging/globals/dev.html#L7

I expected to step through the original source here
https://github.com/facebook/react/blob/master/packages/react-dom/src/client/ReactDOM.js#L665
rather than the development build.

@nprasath002
Copy link
Author

nprasath002 commented Nov 30, 2018

What is the real issue here

I was just curious to step through code and see how it works.

@gaearon
Copy link
Collaborator

gaearon commented Feb 8, 2019

I don't know how to do it but you should be able to step through the code in development mode without sourcemaps.

@gaearon gaearon closed this as completed Feb 8, 2019
@justingrant
Copy link
Contributor

you should be able to step through the code in development mode without sourcemaps.

Hi @gaearon - With today's React you can't do what the OP is asking, which is to be able to set breakpoints in and step through the original React source (not the transpiled code) when debugging a React app using a debugger like Chrome DevTools or VS Code. You can step-through/set-breakpoints-in the transpiled source which is less clear than the original source.

How "less clear" it is depends on the amount of transpilation. Many other libraries that make heavy use of ES6+ features (esp. async/await) are almost unreadable when transpiled to ES5. React doesn't seem to use much ES6 so it's transpiled code is more readable, but it's still less developer-friendly than doing what many other libraries do which is:

  1. Include their original source in packages that they publish to npm
  2. Include sourcemaps pointing to that original source, ideally using separate .map files instead of inline maps because inline sourcemaps can introduce performance and usability issues in some cases when debugging.

If React starts using more ES6+ features, I'd strongly recommend adding sourcemaps and shipping original source to npm.

@tsangint
Copy link

tsangint commented Jan 2, 2020

https://github.com/rollup/rollup/wiki/Troubleshooting#sourcemap-is-likely-to-be-incorrect
Some plugins does not return sourcemap, and I think it should be fixed by react build script.I‘ll try to fix it later

@orloffv
Copy link

orloffv commented Mar 27, 2020

@tsangint any updates?

@pws019
Copy link

pws019 commented Mar 31, 2020

@tsangint 什么时候修复好呢?

@sma2lbao
Copy link

sma2lbao commented Oct 19, 2020

mark

@maomy1992
Copy link

mark

@jasonwilliams
Copy link
Contributor

https://github.com/rollup/rollup/wiki/Troubleshooting#sourcemap-is-likely-to-be-incorrect
Some plugins does not return sourcemap, and I think it should be fixed by react build script.I‘ll try to fix it later

@tsangint did you ever figure out how to get around this issue?

@justingrant
Copy link
Contributor

https://github.com/rollup/rollup/wiki/Troubleshooting#sourcemap-is-likely-to-be-incorrect
Some plugins does not return sourcemap, and I think it should be fixed by react build script.I‘ll try to fix it later

@tsangint did you ever figure out how to get around this issue?

Hi Jase! BTW, the URL above has been moved. New URL is this: https://rollupjs.org/guide/en/#warning-sourcemap-is-likely-to-be-incorrect

@jasonwilliams
Copy link
Contributor

jasonwilliams commented Jul 15, 2021

Hey @justingrant thanks for the link.

I got this working somewhat, I updated Rollup and all of the plugins, set sourcemaps to true and SourceMaps: true in the babel config. Then worked through the plugins.

Some I had to comment out though as i don't know how to add mapping support to some of these internal plugins I think closure and strip-unused-imports can be replaced by the closure plugin.

I've had to give up for now though, if anyone else was looking into it that's what i've done

Comparison: https://github.com/facebook/react/compare/main...jasonwilliams:addSourcMaps?expand=1
Branch (build file): https://github.com/jasonwilliams/react/blob/addSourcMaps/scripts/rollup/build.js

Clone it then run node --trace-warnings .\scripts\rollup\build.js react/index,react-dom/index

@0xdevalias
Copy link
Contributor

I was deep diving into this the other week as well.. was writing up notes/observations as I went.. but then got sidetracked and never completed it. Will include the notes I do have in an expandable below in case it helps anyone else:

Click to expand

In case it's of use for anyone else that stumbles across this issue:

This is where I changed the rollup build script to try and enabled sourcemaps:

diff --git a/scripts/rollup/build.js b/scripts/rollup/build.js
index 41b068509..aad155b45 100644
--- a/scripts/rollup/build.js
+++ b/scripts/rollup/build.js
@@ -235,7 +235,9 @@ function getRollupOutputOptions(
     freeze: !isProduction,
     interop: false,
     name: globalName,
-    sourcemap: false,
+    // TODO: if we change sourcemap to true here will we get sourcemaps output?
+    // sourcemap: false,
+    sourcemap: true,
     esModule: false,
   };
 }

And this is how I attempted to build the code, which resulted in the same error as the OP hit:

⇒  yarn build react/index,react-dom/index --type=UMD
yarn run v1.22.10
$ node ./scripts/rollup/build.js react/index,react-dom/index --type=UMD
 BUILDING  react.development.js (umd_dev)

Sourcemap is likely to be incorrect: a plugin (at position 6) was used to transform files, but didn't generate a sourcemap for the transformation. Consult the plugin documentation for help

error Command failed with exit code 1.

The two main sources that come up when googling that are:

Neither of which seemed particularly helpful on their own.

Modifying the build script a bit more:

diff --git a/scripts/rollup/build.js b/scripts/rollup/build.js
index 41b068509..928c7610d 100644
--- a/scripts/rollup/build.js
+++ b/scripts/rollup/build.js
@@ -159,6 +159,7 @@ function getBabelConfig(
     configFile: false,
     presets: [],
     plugins: [...babelPlugins],
+    sourceMaps: true, // Ref: https://babeljs.io/docs/en/options#sourcemaps
   };
   if (isDevelopment) {
     options.plugins.push(
@@ -235,7 +236,9 @@ function getRollupOutputOptions(
     freeze: !isProduction,
     interop: false,
     name: globalName,
-    sourcemap: false,
+    // TODO: if we change sourcemap to true here will we get sourcemaps output?
+    // sourcemap: false,
+    sourcemap: true,
     esModule: false,
   };
 }
@@ -363,7 +366,8 @@ function getPlugins(
     bundleType === RN_FB_PROD ||
     bundleType === RN_FB_PROFILING;
   const shouldStayReadable = isFBWWWBundle || isRNBundle || forcePrettyOutput;
-  return [
+
+  const thePlugins = [
     // Extract error codes from invariant() messages into a file.
     shouldExtractErrors && {
       transform(source) {
@@ -371,18 +375,23 @@ function getPlugins(
         return source;
       },
     },
+
     // Shim any modules that need forking in this environment.
     useForks(forks),
+
     // Ensure we don't try to bundle any fbjs modules.
     forbidFBJSImports(),
+
     // Use Node resolution mechanism.
     resolve({
       skip: externals,
     }),
+
     // Remove license headers from individual modules
     stripBanner({
       exclude: 'node_modules/**/*',
     }),
+
     // Compile to ES2015.
     babel(
       getBabelConfig(
@@ -393,12 +402,14 @@ function getPlugins(
         !isProduction
       )
     ),
+
     // Remove 'use strict' from individual source files.
     {
       transform(source) {
         return source.replace(/['"]use strict["']/g, '');
       },
     },
+
     // Turn __DEV__ and process.env checks into constants.
     replace({
       __DEV__: isProduction ? 'false' : 'true',
@@ -410,10 +421,12 @@ function getPlugins(
       // NOTE: I did not put much thought into how to configure this.
       __VARIANT__: bundle.enableNewReconciler === true,
     }),
+
     // The CommonJS plugin *only* exists to pull "art" into "react-art".
     // I'm going to port "art" to ES modules to avoid this problem.
     // Please don't enable this for anything else!
     isUMDBundle && entry === 'react-art' && commonjs(),
+
     // Apply dead code elimination and/or minification.
     isProduction &&
       closure(
@@ -424,9 +437,11 @@ function getPlugins(
           renaming: !shouldStayReadable,
         })
       ),
+
     // HACK to work around the fact that Rollup isn't removing unused, pure-module imports.
     // Note that this plugin must be called after closure applies DCE.
     isProduction && stripUnusedImports(pureExternalModules),
+
     // Add the whitespace back if necessary.
     shouldStayReadable &&
       prettier({
@@ -435,6 +450,7 @@ function getPlugins(
         trailingComma: 'none',
         bracketSpacing: true,
       }),
+
     // License and haste headers, top-level `if` blocks.
     {
       renderChunk(source) {
@@ -447,6 +463,7 @@ function getPlugins(
         );
       },
     },
+
     // Record bundle size.
     sizes({
       getSize: (size, gzip) => {
@@ -465,7 +482,14 @@ function getPlugins(
         };
       },
     }),
-  ].filter(Boolean);
+  ]
+
+  const thePluginsFiltered = thePlugins.filter(Boolean);
+
+  console.log(thePlugins);
+  console.log(thePlugins.length);
+
+  return thePluginsFiltered;
 }
 
 function shouldSkipBundle(bundle, bundleType) {

I get the following output:

⇒  yarn build react/index,react-dom/index --type=UMD
yarn run v1.22.10
$ node ./scripts/rollup/build.js react/index,react-dom/index --type=UMD
[
  undefined,
  {
    name: 'scripts/rollup/plugins/use-forks-plugin',
    resolveId: [Function: resolveId]
  },
  { name: 'forbidFBJSImports', resolveId: [Function: resolveId] },
  { name: 'node-resolve', resolveId: [Function: resolveId$1] },
  { transform: [Function: transform] },
  {
    name: 'babel',
    resolveId: [Function: resolveId],
    load: [Function: load],
    transform: [Function: transform]
  },
  { transform: [Function: transform] },
  { name: 'replace', transform: [Function: transform] },
  false,
  false,
  false,
  undefined,
  { renderChunk: [Function: renderChunk] },
  {
    name: 'scripts/rollup/plugins/sizes-plugin',
    generateBundle: [Function: generateBundle]
  }
]
14
 BUILDING  react.development.js (umd_dev)

Sourcemap is likely to be incorrect: a plugin (at position 6) was used to transform files, but didn't generate a sourcemap for the transformation. Consult the plugin documentation for help

error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Since the array of plugins is filtered to only use the 'truthy' plugins, the 'position 6' referred to in the error corresponds with:

// Remove 'use strict' from individual source files.
{
  transform(source) {
    return source.replace(/['"]use strict["']/g, '');
  },
},

If we comment that out then run it again, we get the following error:

Sourcemap is likely to be incorrect: a plugin (at position 7) was used to transform files, but didn't generate a sourcemap for the transformation. Consult the plugin documentation for help

This seems to correspond with the following:

// License and haste headers, top-level `if` blocks.
{
  renderChunk(source) {
    return Wrappers.wrapBundle(
      source,
      bundleType,
      globalName,
      filename,
      moduleType
    );
  },
},

If we comment that out then run it again, we get the following error:

Sourcemap is likely to be incorrect: a plugin (scripts/rollup/plugins/closure-plugin) was used to transform files, but didn't generate a sourcemap for the transformation. Consult the plugin documentation for help

This seems to correspond with the following:

// Apply dead code elimination and/or minification.
isProduction &&
  closure(
    Object.assign({}, closureOptions, {
      // Don't let it create global variables in the browser.
      // https://github.com/facebook/react/issues/10909
      assume_function_wrapper: !isUMDBundle,
      renaming: !shouldStayReadable,
    })
  ),

If we look at scripts/rollup/plugins/closure-plugin.js we can see that it uses require('google-closure-compiler')

@0xdevalias
Copy link
Contributor

I think closure and strip-unused-imports can be replaced by the terser plugin.

My notes/main area of focus in #14361 (comment) was on the closure compiler. It has the ability to generate sourcemaps already, just needs to be configured to do so.

Here's the diff I have locally, I had this working the other day for this part, but haven't tested again since if I modified this diff further.. so it might not be perfectly working right now.. Hopefully points people in the right direction but!

It also seemed like if we switched from the java closure compiler to the JS one, it might have a nicer API to use. Unsure if/how much impact that would have on running speed/etc for it though.

Click to expand/see diff
diff --git a/scripts/rollup/plugins/closure-plugin.js b/scripts/rollup/plugins/closure-plugin.js
index 258f3b6be..37c7ec52a 100644
--- a/scripts/rollup/plugins/closure-plugin.js
+++ b/scripts/rollup/plugins/closure-plugin.js
@@ -5,11 +5,21 @@ const {promisify} = require('util');
 const fs = require('fs');
 const tmp = require('tmp');
 const writeFileAsync = promisify(fs.writeFile);
+const readFileAsync = promisify(fs.readFile);
 
 function compile(flags) {
   return new Promise((resolve, reject) => {
     const closureCompiler = new ClosureCompiler(flags);
+
+    console.log(flags);
+    console.log(closureCompiler);
+    console.log(Object.keys(closureCompiler));
+
     closureCompiler.run(function(exitCode, stdOut, stdErr) {
+      // console.log(stdOut);
+      // console.log(stdErr);
+      // console.log(stdErr);
+
       if (!stdErr) {
         resolve(stdOut);
       } else {
@@ -25,11 +35,25 @@ module.exports = function closure(flags = {}) {
     async renderChunk(code) {
       const inputFile = tmp.fileSync();
       const tempPath = inputFile.name;
-      flags = Object.assign({}, flags, {js: tempPath});
+      flags = Object.assign({}, flags, {js: tempPath}, { create_source_map: tempPath + ".map"});
       await writeFileAsync(tempPath, code, 'utf8');
+
+      console.log(flags);
+
       const compiledCode = await compile(flags);
+
+      const compiledSourceMap = await readFileAsync(flags.create_source_map, { encoding: "utf8"});
+
+      console.log(typeof compiledCode);
+
+
+      console.log(tempPath)
+      console.log(flags.create_source_map)
+      // console.log(Object.keys(compiledCode));
+      // console.log(compiledSourceMap);
+
       inputFile.removeCallback();
-      return {code: compiledCode};
+      return {code: compiledCode, map: compiledSourceMap};
     },
   };
 };

Originally posted by @0xdevalias in #20186 (comment)

@jasonwilliams
Copy link
Contributor

jasonwilliams commented Jul 17, 2021

You can give each plugin a name, then instead of saying “position 6” it will say “use strict” for example. In my branch I did this

@Terry-Su
Copy link

@nprasath002 @gaearon @others-who-meet-same-problem I struggled with this question for long time before, finally approached debugging splitted files in a different way: https://github.com/Terry-Su/debug-react-source-code. Hope this helps.

@0xdevalias
Copy link
Contributor

as i don't know how to add mapping support to some of these internal plugins

I haven't looked too deeply into this, so not sure if they are covering the same use cases, but for the "Remove 'use strict' from individual source files" I came across the following when googling:


I think closure and strip-unused-imports can be replaced by the terser plugin.

I just stumbled upon this benchmark repo that includes benchmarks for google-closure-compiler and terser (plus a bunch more such as esbuild), which might be useful here:

esbuild tends to win hands down as far as speed is concerned. terser generally seems to come in near the top for minified sizes, and is a lot quicker than uglify-js (which tends to slightly win in size, but for a much longer run time). google-closure-compiler often tends to have less good minified sizes, and take longer to run. But it bounces around a bit depending on the library it was tested against, so hard to draw any 'universal conclusions' about it.

@LitileXueZha
Copy link

The error means one of plugins (rollup-plugin-babel) not returns source map.

I tried build with source map myself, and then wrote a gist for record: debug react source code. Hope it can you.

@jasonwilliams
Copy link
Contributor

My PR is here if anyone wanted to take a look #21946

@0xdevalias
Copy link
Contributor

For anyone still following this issue that isn't also following #20186, that is where the majority of updates RE: sourcemap support are happening now.

PR added: #21946

I feel its close, but one of the plugins renderChunk needs looking at. And i don't understand why some production builds have shouldStayReadable set. So it gets minified then re-beautified by prettier. This seems odd to me but im sure there's logic to it somewhere. I don't think sourcemaps for these builds are needed if the output is already beautified.

@jasonwilliams Thanks for your work on that PR! It's taken me forever to get back to this, but I spent some time today going through it, leaving some rather in depth context/comments/etc, as well as researching solutions to what I believe should be the rest of the remaining issues.:

Originally posted by @0xdevalias in #20186 (comment)

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

No branches or pull requests