Add source maps #21946

jasonwilliams · 2021-07-23T19:33:36Z

Fixes #20186

Summary

Attempts to get sourcemaps working.

Status:

only the renderChunk is commented out for now as I had to give up with that one
Sourcemap generation is slow on the prettier plugin. Maybe sourcemaps aren't needed if we're generating "pretty" views anyway? We should disable this if shouldStayReadable is true.

stale · 2022-01-08T23:41:54Z

This pull request has been automatically marked as stale. If this pull request is still relevant, please leave any comment (for example, "bump"), and we'll keep it open. We are sorry that we haven't been able to prioritize reviewing it yet. Your contribution is very much appreciated.

justingrant · 2022-01-08T23:46:29Z

not stale

0xdevalias

A rather verbose review + deep dive research into the context of why certain changes were made here, as well as potential solutions for what I believe are the remaining issues that would block this PR from landing. At least in theory, I think all of the potential issues have solutions now. Would love to see this work able to land!

There are a few minor code cleanup 'suggestions' as well, which can all be automatically applied as a single commit as follows:

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request#applying-suggested-changes

0xdevalias · 2022-07-10T02:33:22Z

scripts/rollup/build.js

@@ -364,26 +368,38 @@ function getPlugins(
    bundleType === RN_FB_PROFILING;
  const shouldStayReadable = isFBWWWBundle || isRNBundle || forcePrettyOutput;
  return [
-    // Extract error codes from invariant() messages into a file.
+    // // Extract error codes from invariant() messages into a file.


Suggested change

// // Extract error codes from invariant() messages into a file.

// Extract error codes from invariant() messages into a file.

There seem to be a bunch of places where the comment // has been doubled up in this PR. While it's minor, it will help minimise the diff to better see where the real changes are happening.

0xdevalias · 2022-07-10T02:43:17Z

scripts/rollup/build.js

-    // Use Node resolution mechanism.
-    resolve({
-      skip: externals,
-    }),
-    // Remove license headers from individual modules
+    // // Use Node resolution mechanism.
+    resolve(),


Why was skip: externals removed from resolve here out of curiosity?

It looks like the old version of the library removed it in 3.0.0 in favour of using external:

https://github.com/rollup/rollup-plugin-node-resolve/blob/master/CHANGELOG.md#300

[BREAKING] Remove options.skip rollup/rollup-plugin-node-resolve#90

People are better off using the external option in the main Rollup config

https://github.com/rollup/rollup-plugin-node-resolve#resolving-built-ins-like-fs

Looking at the options for the new library, these are the parts that seem potentially relevant:

https://github.com/rollup/plugins/tree/master/packages/node-resolve#options

https://github.com/rollup/plugins/tree/master/packages/node-resolve#resolving-built-ins-like-fs

0xdevalias · 2022-07-10T02:52:23Z

scripts/rollup/build.js

+    // // Turn __DEV__ and process.env checks into constants.
+    replace({
+      preventAssignment: true,
+      values: {
+        __DEV__: isProduction ? 'false' : 'true',
+        __PROFILE__: isProfiling || !isProduction ? 'true' : 'false',
+        __UMD__: isUMDBundle ? 'true' : 'false',
+        'process.env.NODE_ENV': isProduction ? "'production'" : "'development'",
+        __EXPERIMENTAL__,
+        // Enable forked reconciler.
+        // NOTE: I did not put much thought into how to configure this.
+        __VARIANT__: bundle.enableNewReconciler === true,
+      },
+    }),


Curious, why was this moved earlier in the chain of things? (originally down around line 402-412)

I suspect that this may be incorrect, as now it is being run before useForks, which could result in different code being imported (and thus more potential locations of use strict.

Comparing the diff, the main changes seem to be the addition of preventAssignment, and wrapping the values (presumably based on a change in the options of the new lib version)

New lib:

https://github.com/rollup/plugins/tree/master/packages/replace#options

https://github.com/rollup/plugins/tree/master/packages/replace#preventassignment

Prevents replacing strings where they are followed by a single equals sign.

Old lib:

https://github.com/rollup/rollup-plugin-replace#options

All other options are treated as string: replacement replacers...

...unless you want to be careful about separating values from other options, in which case you can

0xdevalias · 2022-07-10T02:52:50Z

scripts/rollup/build.js

+        __VARIANT__: bundle.enableNewReconciler === true,
+      },
+    }),
+    // // Shim any modules that need forking in this environment.


Suggested change

// // Shim any modules that need forking in this environment.

// Shim any modules that need forking in this environment.

0xdevalias · 2022-07-10T02:52:59Z

scripts/rollup/build.js

    useForks(forks),
-    // Ensure we don't try to bundle any fbjs modules.
+    // // Ensure we don't try to bundle any fbjs modules.


Suggested change

// // Ensure we don't try to bundle any fbjs modules.

// Ensure we don't try to bundle any fbjs modules.

0xdevalias · 2022-07-10T06:11:20Z

scripts/rollup/build.js

    babelrc: false,
    configFile: false,
+    sourceMaps: true,


It looks like @rollup/plugin-babel generates sourcemaps by default since 2.1.0:

https://github.com/rollup/plugins/blob/master/packages/babel/CHANGELOG.md#210

So we probably don't need to add this (unless it's also going to other plugins that don't set it by default?)

Suggested change

sourceMaps: true,

0xdevalias · 2022-07-10T06:17:09Z

package.json

+    "@rollup/plugin-replace": "^2.4.2",
+    "@rollup/plugin-commonjs": "19.0.1",
+    "@rollup/plugin-node-resolve": "^13.0.1",
+    "@rollup/plugin-babel": "^5.3.0",


Latest versions:

https://www.npmjs.com/package/@rollup/plugin-replace (4.0.0)

https://www.npmjs.com/package/@rollup/plugin-commonjs (22.0.1)

https://www.npmjs.com/package/@rollup/plugin-node-resolve (13.3.0)

https://www.npmjs.com/package/@rollup/plugin-babel (5.3.1)

0xdevalias · 2022-07-10T06:19:05Z

package.json

+    "rollup": "^2.53.2",
+    "rollup-plugin-prettier": "^2.1.0",
+    "rollup-plugin-strip-banner": "^2.0.0",
+    "rollup-plugin-terser": "^7.0.2",


Latest versions:

https://www.npmjs.com/package/rollup (2.76.0)

https://www.npmjs.com/package/rollup-plugin-prettier (2.2.2)

https://www.npmjs.com/package/rollup-plugin-strip-banner (2.0.0)

https://www.npmjs.com/package/rollup-plugin-terser (7.0.2)

0xdevalias · 2022-07-10T06:22:18Z

scripts/rollup/build.js

@@ -235,7 +239,7 @@ function getRollupOutputOptions(
    freeze: !isProduction,
    interop: false,
    name: globalName,
-    sourcemap: false,
+    sourcemap: !isPretty,


As mentioned in another comment in this review, I think maybe we should be using shouldStayReadable here directly, since the only time where we (currently) wouldn't be generating sourcemaps (to my knowledge) is when prettier is called (though personally, if it doesn't change the build time dramatically, I would be potentially inclined to keep it enabled even then)

Suggested change

sourcemap: !isPretty,

sourcemap: !shouldStayReadable,

0xdevalias · 2022-07-10T06:23:48Z

scripts/rollup/build.js

-      skip: externals,
-    }),
-    // Remove license headers from individual modules
+    // // Use Node resolution mechanism.


Suggested change

// // Use Node resolution mechanism.

// Use Node resolution mechanism.

@jasonwilliams

Update Rollup and related plugins to their most recent versions + resolve any breaking changes/deprecations/etc along the way. I made each change piece by piece, so the commit history tells a pretty good story of what was changed where/how/why. fixes #24894 For the full deepdive/context, see: - #24894 The inspiration for this came from @jasonwilliams 's PR for attempting to add sourcemap output support to React's builds: - #20186 - #21946 But I figured that it would be useful to minimise the scope of changes in that PR, and to modernise the build tooling along the way. If any of these updates rely on a node version later than `10.x`, then the following PR may have to land first, otherwise things might break on AppVeyor: - #24891 - #24892 Co-authored-by: Sebastian Markbage <sebastian@calyptus.eu>

@jasonwilliams

Update Rollup and related plugins to their most recent versions + resolve any breaking changes/deprecations/etc along the way. I made each change piece by piece, so the commit history tells a pretty good story of what was changed where/how/why. fixes #24894 For the full deepdive/context, see: - #24894 The inspiration for this came from @jasonwilliams 's PR for attempting to add sourcemap output support to React's builds: - #20186 - #21946 But I figured that it would be useful to minimise the scope of changes in that PR, and to modernise the build tooling along the way. If any of these updates rely on a node version later than `10.x`, then the following PR may have to land first, otherwise things might break on AppVeyor: - #24891 - #24892 Co-authored-by: Sebastian Markbage <sebastian@calyptus.eu> DiffTrain build for [6b6d061](6b6d061) [View git log for this commit](https://github.com/facebook/react/commits/6b6d0617eff48860c5b4e3e79c74cbd3312cf45a)

0xdevalias · 2023-03-27T04:10:02Z

It looks like the work done in the following PR is likely to supercede and replace this PR:

Generate sourcemaps for production build artifacts #26446

0xdevalias · 2023-12-01T21:49:01Z

Since the above PR (#26446) has been merged, this PR is now obsolete, and can probably be closed out.

@jasonwilliams

Update Rollup and related plugins to their most recent versions + resolve any breaking changes/deprecations/etc along the way. I made each change piece by piece, so the commit history tells a pretty good story of what was changed where/how/why. fixes facebook/react#24894 For the full deepdive/context, see: - facebook/react#24894 The inspiration for this came from @jasonwilliams 's PR for attempting to add sourcemap output support to React's builds: - facebook/react#20186 - facebook/react#21946 But I figured that it would be useful to minimise the scope of changes in that PR, and to modernise the build tooling along the way. If any of these updates rely on a node version later than `10.x`, then the following PR may have to land first, otherwise things might break on AppVeyor: - facebook/react#24891 - facebook/react#24892 Co-authored-by: Sebastian Markbage <sebastian@calyptus.eu> DiffTrain build for [6b6d0617eff48860c5b4e3e79c74cbd3312cf45a](facebook/react@6b6d061) [View git log for this commit](https://github.com/facebook/react/commits/6b6d0617eff48860c5b4e3e79c74cbd3312cf45a)

eps1lon · 2024-03-23T19:25:04Z

Resolved in #26446

Thank you @jasonwilliams for starting this effort 👍🏻

jasonwilliams added 2 commits July 15, 2021 23:52

attempt to add sourcemap support

1cd0b9a

include terser

7880112

facebook-github-bot added the CLA Signed label Jul 23, 2021

jasonwilliams mentioned this pull request Jul 23, 2021

Distribute source maps for easier debugging in Chrome's Performance tab #20186

Closed

jasonwilliams mentioned this pull request Oct 10, 2021

Question: Building react with sourcemaps #14361

Closed

stale bot added the Resolution: Stale Automatically closed due to inactivity label Jan 8, 2022

stale bot removed the Resolution: Stale Automatically closed due to inactivity label Jan 8, 2022

0xdevalias reviewed Jul 10, 2022

View reviewed changes

0xdevalias mentioned this pull request Mar 27, 2023

Generate sourcemaps for production build artifacts #26446

Merged

eps1lon closed this Mar 23, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add source maps #21946

Add source maps #21946

jasonwilliams commented Jul 23, 2021

stale bot commented Jan 8, 2022

justingrant commented Jan 8, 2022

0xdevalias left a comment

0xdevalias Jul 10, 2022

0xdevalias Jul 10, 2022

0xdevalias Jul 10, 2022 •

edited

Loading

0xdevalias Jul 10, 2022

0xdevalias Jul 10, 2022

0xdevalias Jul 10, 2022

0xdevalias Jul 10, 2022

0xdevalias Jul 10, 2022

0xdevalias Jul 10, 2022

0xdevalias Jul 10, 2022

0xdevalias commented Mar 27, 2023

0xdevalias commented Dec 1, 2023

eps1lon commented Mar 23, 2024

	// // Extract error codes from invariant() messages into a file.
	// Extract error codes from invariant() messages into a file.

	// // Shim any modules that need forking in this environment.
	// Shim any modules that need forking in this environment.

	// // Ensure we don't try to bundle any fbjs modules.
	// Ensure we don't try to bundle any fbjs modules.

	// // Use Node resolution mechanism.
	// Use Node resolution mechanism.

Add source maps #21946

Add source maps #21946

Conversation

jasonwilliams commented Jul 23, 2021

Summary

stale bot commented Jan 8, 2022

justingrant commented Jan 8, 2022

0xdevalias left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xdevalias Jul 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

0xdevalias commented Mar 27, 2023

0xdevalias commented Dec 1, 2023

eps1lon commented Mar 23, 2024

0xdevalias Jul 10, 2022 •

edited

Loading