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.

Sign up for GitHub

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

Jump to bottom

Add source maps #21946

Closed
wants to merge 2 commits into from
Closed

Add source maps #21946

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1cd0b9a
attempt to add sourcemap support
jasonwilliams Jul 15, 2021
7880112
include terser
jasonwilliams Jul 23, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 8 additions & 7 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,10 @@
"@babel/preset-react": "^7.10.4",
"@babel/traverse": "^7.11.0",
"@mattiasbuelens/web-streams-polyfill": "^0.3.2",
"@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",
Comment on lines +39 to +42
Copy link
Contributor

Choose a reason for hiding this comment

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

Latest versions:

"abort-controller": "^3.0.0",
"art": "0.10.1",
"babel-eslint": "^10.0.3",
Expand Down Expand Up @@ -83,13 +87,10 @@
"random-seed": "^0.3.0",
"react-lifecycles-compat": "^3.0.4",
"rimraf": "^3.0.0",
"rollup": "^1.19.4",
"rollup-plugin-babel": "^4.0.1",
"rollup-plugin-commonjs": "^9.3.4",
"rollup-plugin-node-resolve": "^2.1.1",
"rollup-plugin-prettier": "^0.6.0",
"rollup-plugin-replace": "^2.2.0",
"rollup-plugin-strip-banner": "^0.2.0",
"rollup": "^2.53.2",
"rollup-plugin-prettier": "^2.1.0",
"rollup-plugin-strip-banner": "^2.0.0",
"rollup-plugin-terser": "^7.0.2",
Comment on lines +90 to +93
Copy link
Contributor

Choose a reason for hiding this comment

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

Latest versions:

"semver": "^7.1.1",
"targz": "^1.0.1",
"through2": "^3.0.1",
Expand Down
115 changes: 56 additions & 59 deletions scripts/rollup/build.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
'use strict';

const rollup = require('rollup');
const babel = require('rollup-plugin-babel');
const babel = require('@rollup/plugin-babel').babel;
const closure = require('./plugins/closure-plugin');
const commonjs = require('rollup-plugin-commonjs');
const commonjs = require('@rollup/plugin-commonjs');
const prettier = require('rollup-plugin-prettier');
const replace = require('rollup-plugin-replace');
const replace = require('@rollup/plugin-replace');
const stripBanner = require('rollup-plugin-strip-banner');
const chalk = require('chalk');
const path = require('path');
const resolve = require('rollup-plugin-node-resolve');
const resolve = require('@rollup/plugin-node-resolve').nodeResolve;
const fs = require('fs');
const argv = require('minimist')(process.argv.slice(2));
const Modules = require('./modules');
Expand All @@ -19,6 +19,7 @@ const Sync = require('./sync');
const sizes = require('./plugins/sizes-plugin');
const useForks = require('./plugins/use-forks-plugin');
const stripUnusedImports = require('./plugins/strip-unused-imports');
const terser = require('rollup-plugin-terser').terser;
const extractErrorCodes = require('../error-codes/extract-errors');
const Packaging = require('./packaging');
const {asyncRimRaf} = require('./utils');
Expand Down Expand Up @@ -155,8 +156,10 @@ function getBabelConfig(
packageName === 'react' || externals.indexOf('react') !== -1;
let options = {
exclude: '/**/node_modules/**',
babelHelpers: 'bundled',
Copy link
Contributor

Choose a reason for hiding this comment

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

babelrc: false,
configFile: false,
sourceMaps: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

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,

presets: [],
plugins: [...babelPlugins],
};
Expand Down Expand Up @@ -224,7 +227,8 @@ function getRollupOutputOptions(
format,
globals,
globalName,
bundleType
bundleType,
isPretty
) {
const isProduction = isProductionBundleType(bundleType);

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

Choose a reason for hiding this comment

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

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,

esModule: false,
};
}
Expand Down Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

shouldExtractErrors && {
transform(source) {
findAndRecordErrorCodes(source);
return source;
},
},
// Shim any modules that need forking in this environment.
// // Turn __DEV__ and process.env checks into constants.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// // Turn __DEV__ and process.env checks into constants.
// 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,
},
}),
Comment on lines +378 to +391
Copy link
Contributor

@0xdevalias 0xdevalias Jul 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

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:

Old lib:

// // Shim any modules that need forking in this environment.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

useForks(forks),
// Ensure we don't try to bundle any fbjs modules.
// // Ensure we don't try to bundle any fbjs modules.
Copy link
Contributor

Choose a reason for hiding this comment

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

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

forbidFBJSImports(),
// Use Node resolution mechanism.
resolve({
skip: externals,
}),
// Remove license headers from individual modules
// // Use Node resolution mechanism.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// // Use Node resolution mechanism.
// Use Node resolution mechanism.

resolve(),
Comment on lines -378 to +397
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

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

// // Remove license headers from individual modules
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// // Remove license headers from individual modules
// Remove license headers from individual modules

stripBanner({
exclude: 'node_modules/**/*',
}),
// Compile to ES2015.
// // Compile to ES2015.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// // Compile to ES2015.
// Compile to ES2015.

babel(
getBabelConfig(
updateBabelOptions,
Expand All @@ -393,61 +409,39 @@ function getPlugins(
!isProduction
)
),
// Remove 'use strict' from individual source files.
{
transform(source) {
return source.replace(/['"]use strict["']/g, '');
},
},
Comment on lines -396 to -401
Copy link
Contributor

Choose a reason for hiding this comment

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

This was presumably replaced with the strict: false option in the rollupConfig as per:

  • https://rollupjs.org/guide/en/#outputstrict

    • Whether to include the 'use strict' pragma at the top of generated non-ES bundles. Strictly speaking, ES modules are always in strict mode, so you shouldn't disable this without good reason.

I believe the removed code stripped use strict from the source files before being passed into the latter plugins (such as the closure compiler); whereas I believe the change made here only relates to the files eventually output from rollup; which would seem to be different from how it currently is, as use strict is included in all of the output files I checked:

My assumption is that this transform may have been originally included to workaround a limitation in closure-compiler, eg a quick search lead me to the following:

Looking at the closureOptions, language_in appears to currently be set to ECMASCRIPT_2015:

Presumably this was done prior to the ECMASCRIPT5_STRICT option being added to language_in.

I would have to check out and build the react code to be certain, but my guess is that leaving everything else the same as it currently is in main, if this transform was removed, and the language_in set to ECMASCRIPT5_STRICT, I suspect it may 'just work'. If that's true, then I think this transform can be removed in this PR, as well as removing the strict: false option from the rollupConfig that was modified in this PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

If it's still needed, we might be able to get this functionality back in a 'sourcemap compatible' way by using @rollup/plugin-replace (that we already have/use as a dependency):

The regular expression /['"]use strict["']/g (while remaining syntax correct with matching quotes) equates to basically just matching:

  • 'use strict'
  • "use strict"

So I think a call like this would probably manage it (or include these in the existing call to it):

replace({
      '\'use strict\'': '',
      '"use strict"': '',
    })

Some consideration of the word boundary regex may be required also, since i'm not sure if it will currently match against the ; that likely follows these calls:

Copy link
Contributor

@0xdevalias 0xdevalias Jul 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

See also my comments elsewhere in this review (ref) that talk about potentially using magic-string (which is the underlying lib used by @rollup/plugin-replace to generate sourcemaps) directly:

// Turn __DEV__ and process.env checks into constants.
replace({
__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,
}),
Comment on lines -402 to -412
Copy link
Contributor

@0xdevalias 0xdevalias Jul 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

[note] This was moved higher up, around line 379-391.

See also:

// 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(
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,
})
),
// 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.
terser({
// Don't let it create global variables in the browser.
enclose: !isUMDBundle,
mangle: !shouldStayReadable,
}),
Comment on lines -419 to +423
Copy link
Contributor

Choose a reason for hiding this comment

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

This replaces google-closure-compiler with rollup-plugin-terser

Since closure-compiler was removed from here, and doesn't appear to be used anywhere else in the React codebase (based on a quick search), we should probably clean up the remaining references to it:

I also see that the following appears to have been removed here:

    // 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),

Is this no longer needed with the current version of Rollup/etc?

Copy link
Contributor

Choose a reason for hiding this comment

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

We'll want to keep GCC instead of Terser here. We found GCC to improve the code size significantly compared to Terser.

shouldStayReadable &&
prettier({
parser: 'babel',
singleQuote: false,
trailingComma: 'none',
bracketSpacing: true,
sourceMap: false,
}),
Comment on lines 424 to 431
Copy link
Contributor

Choose a reason for hiding this comment

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

Tracing for the origins of shouldStayReadable to try and infer it's usage/intent:

It seems like it is mostly used for FB internal builds, and RN (React Native?) builds. While I can't speak for FB internal usage at all, presumably there wouldn't be a huge benefit to having the sourcemap for the prettified file when it's already readable.

  • https://github.com/mjeanroy/rollup-plugin-prettier#source-maps

    • If source map is enabled in the global rollup options, then a source map will be generated on the formatted bundle (except if sourcemap are explicitely disabled in the prettier options).

    • Note that this may take some time since prettier package is not able to generate a sourcemap and this plugin must compute the diff between the original bundle and the formatted result and generate the corresponding sourcemap: for this reason, sourcemap are disabled by default.

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.

Originally posted by @jasonwilliams in #20186 (comment)

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.

@jasonwilliams I'm curious, how much slower is it when generating the sourcemaps for prettier compared to not?

// License and haste headers, top-level `if` blocks.
{
renderChunk(source) {
return Wrappers.wrapBundle(
source,
bundleType,
globalName,
filename,
moduleType
);
},
},
// Record bundle size.
// // License and haste headers, top-level `if` blocks.
// {
// renderChunk(source) {
// return Wrappers.wrapBundle(
// source,
// bundleType,
// globalName,
// filename,
// moduleType
// );
// },
// },
Comment on lines -438 to +443
Copy link
Contributor

Choose a reason for hiding this comment

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

only the renderChunk is commented out for now as I had to give up with that one

@jasonwilliams What was the issue you ran into with renderChunk here out of curiosity?

Copy link
Contributor

Choose a reason for hiding this comment

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

https://github.com/facebook/react/blob/main/scripts/rollup/build.js

const Wrappers = require('./wrappers');

https://github.com/facebook/react/blob/main/scripts/rollup/wrappers.js#L353-L385

function wrapBundle(
  source,
  bundleType,
  globalName,
  filename,
  moduleType,
  wrapWithModuleBoundaries
) {
  if (wrapWithModuleBoundaries) {
    switch (bundleType) {
      case NODE_DEV:
      case NODE_PROFILING:
      case FB_WWW_DEV:
      case FB_WWW_PROFILING:
      case RN_OSS_DEV:
      case RN_OSS_PROFILING:
      case RN_FB_DEV:
      case RN_FB_PROFILING:
        // Remove the 'use strict' directive from source.
        // The module start wrapper will add its own.
        // This directive is only meaningful when it is the first statement in a file or function.
        source = source.replace(USE_STRICT_HEADER_REGEX, '');

        // Certain DEV and Profiling bundles should self-register their own module boundaries with DevTools.
        // This allows the Timeline to de-emphasize (dim) internal stack frames.
        source = `
          ${registerInternalModuleStart(globalName)}
          ${source}
          ${registerInternalModuleStop(globalName)}
        `;
        break;
    }
  }

  // ..snip..

}

The first part of wrapBundle only seems to do anything when wrapWithModuleBoundaries is truthy, yet that option doesn't even seem to be getting passed in from this renderChunk call within this PR.

Though looking at the current main source, it seems that it is actually getting passed in there:

Which was added in:

  • Scheduling Profiler: De-emphasize React internal frames #22588

    • This commit adds code to all React bundles to explicitly register the beginning and ending of the module. This is done by creating Error objects (which capture the file name, line number, and column number) and passing them explicitly to a DevTools hook (when present).

    • The net effect of this is that user code (and 3rd party code) stands out clearly in the flame graph while React internal modules are dimmed.

This relies on bundle.wrapWithModuleBoundaries. bundle seems to come from Bundles.bundles here:

Which is imported from:

For each of the bundle object definitions they define the boolean wrapWithModuleBoundaries. When true, the renderChunk call to wrapBundle will run in a meaningful way (assuming the bundleType also matches the switch within wrapBundle)

When meaningfully run, wrapBundle seems to:

  • remove use strict from the source code
  • prepend the output from registerInternalModuleStart (used in DevTools to dim internal code paths)
  • append the output from registerInternalModuleStop (used in DevTools to dim internal code paths)

It might be possibly to meaningfully generate the sourcemaps to handle this functionality, and/or rewrite it to use existing rollup plugins/features that already support sourcemaps.

Copy link
Contributor

@0xdevalias 0xdevalias Jul 10, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

See my comments elsewhere in this review (ref) for how we could potentially use @rollup/plugin-replace to handle the use strict removal.

Also, looking at the dependencies of @rollup/plugin-replace, I noticed that it uses magic-string for manipulating strings and generating sourcemaps:

While I haven't tried to use it yet, it would seem that this would be a perfect use case for fixing up the prepend/append (aka: 'wrapping with header/footer') of the output from registerInternalModuleStart/registerInternalModuleStop. Seemingly it even handles regex replacements.. so could potentially be used directly for our use strict case:

Therefore, using magic-string, the code would likely end up looking something like:

+ const s = new MagicString(source);

// Remove the 'use strict' directive from source.
// The module start wrapper will add its own.
// This directive is only meaningful when it is the first statement in a file or function.
- source = source.replace(USE_STRICT_HEADER_REGEX, '');
+ s.replace(USE_STRICT_HEADER_REGEX, '');

// Certain DEV and Profiling bundles should self-register their own module boundaries with DevTools.
// This allows the Timeline to de-emphasize (dim) internal stack frames.
- source = `
-   ${registerInternalModuleStart(globalName)}
-   ${source}
-  ${registerInternalModuleStop(globalName)}
- `;
+ // Note: If the newlines don't matter in the output then we can remove the \n's below
+ s.prepend(`${registerInternalModuleStart(globalName)}\n`)
+ s.append(`\n${registerInternalModuleStop(globalName)}\n`)
+
+ source = s.toString()

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like I missed the bit of code at the end of wrapBundle:

  if (moduleType === RECONCILER) {
    // Standalone reconciler is only used by third-party renderers.
    // It is handled separately.
    const wrapper = reconcilerWrappers[bundleType];
    if (typeof wrapper !== 'function') {
      throw new Error(
        `Unsupported build type for the reconciler package: ${bundleType}.`
      );
    }
    return wrapper(source, globalName, filename, moduleType);
  }
  // All the other packages.
  const wrapper = wrappers[bundleType];
  if (typeof wrapper !== 'function') {
    throw new Error(`Unsupported build type: ${bundleType}.`);
  }
  return wrapper(source, globalName, filename, moduleType);

Each of these functions (or the code that calls them) would need to similarly make use of magic-string to not break the sourcemap support:

From a quick skim through:

  • it looks like most of the wrappers could use magic-string's prepend, though some (eg the UMD_*, NODE_DEV, FB_WWW_DEV, RN_OSS_DEV, RN_FB_DEV, etc builds) would also need to be able to use append.
  • the reconcilerWrappers would all need to use both prepend and append

While the original code just manipulates the source variable throughout this entire function, we would need to ensure we're only manipulating the MagicString object with it's functions until the very end, where we can use s.toString() or similar in our final return.

Looking at Rollup's renderChunk:

It looks like to enable sourcemap support we would need to return an object { code: string, map: SourceMap } rather than just the current string.

We can use magic-string's s.generateMap( options ):

Which would end up looking something like:

const s = new MagicString(source);

// do some manipulations with s.prepend, s.append, s.replace, etc

// TODO: maybe add some other options to s.generateMap as well?
return { code: s.toString(), map: s.generateMap({ source: filename }) };

Copy link
Contributor

@0xdevalias 0xdevalias Jul 12, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like magic-string only added s.replace in 0.26.0, which requires Node.js v12 or higher:

The current yarn.lock contains the following versions:

  • magic-string@0.25.6
  • magic-string@0.25.7
⇒  yarn why magic-string
yarn why v1.22.19
[1/4] 🤔  Why do we have the module "magic-string"...?
[2/4] 🚚  Initialising dependency graph...
[3/4] 🔍  Finding dependency...
[4/4] 🚡  Calculating file sizes...
=> Found "magic-string@0.25.7"
info Has been hoisted to "magic-string"
info Reasons this module exists
   - "workspace-aggregator-0c762641-e299-4a7b-b334-fee60ce16010" depends on it
   - Hoisted from "_project_#rollup-plugin-prettier#magic-string"
   - Hoisted from "_project_#rollup-plugin-strip-banner#magic-string"
   - Hoisted from "_project_#@rollup#plugin-replace#magic-string"
   - Hoisted from "_project_#@rollup#plugin-commonjs#magic-string"
info Disk size without dependencies: "388KB"
info Disk size with unique dependencies: "456KB"
info Disk size with transitive dependencies: "456KB"
info Number of shared dependencies: 1
=> Found "rollup-plugin-commonjs#magic-string@0.25.6"
info This module exists because "_project_#react-devtools-extensions#rollup-plugin-commonjs" depends on it.
✨  Done in 1.08s.

Looking at .circleci/config.yml, the jobs seem to run on the cimg/openjdk:17.0.0-node docker container:

But looking at appveyor.yml, Windows seems to be tested against node 10.x, which would presumably fail to build if we updated to magic-string 0.26.0 or higher, since it relies on Node 12.x or higher.

This PR attempted to add testing support to AppVeyor for Node 11.x and 12.x, but never landed:

Edit: I have opened an issue/PR to explore whether AppVeyor's node version can be bumped to a modern version, or other potential solutions:

Copy link
Contributor

Choose a reason for hiding this comment

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

It looks like the potential concerns raised around appveyor.yml and node versions is now resolved (the config was unused, and has been removed), as per the following:

Based on the following comment on the associated PR I opened, I believe this issue can now be closed as well:

The unused config was now deleted. Thanks for drawing attention at it!

Originally posted by @kassens in #24892 (comment)

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

// // Record bundle size.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// // Record bundle size.
// Record bundle size.

sizes({
getSize: (size, gzip) => {
const currentSizes = Stats.currentBuildResults.bundleSizes;
Expand Down Expand Up @@ -576,7 +570,7 @@ async function createBundle(bundle, bundleType) {
const rollupConfig = {
input: resolvedEntry,
treeshake: {
pureExternalModules,
moduleSideEffects: pureExternalModules,
Comment on lines -579 to +573
Copy link
Contributor

Choose a reason for hiding this comment

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

pureExternalModules usage:

https://github.com/facebook/react/blob/main/scripts/rollup/build.js

  const Modules = require('./modules');

  // ..snip..

  const importSideEffects = Modules.getImportSideEffects();
  const pureExternalModules = Object.keys(importSideEffects).filter(
    module => !importSideEffects[module]
  );

  // ..snip..

    // 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),

  // ..snip..

https://github.com/facebook/react/blob/main/scripts/rollup/modules.js

// For any external that is used in a DEV-only condition, explicitly
// specify whether it has side effects during import or not. This lets
// us know whether we can safely omit them when they are unused.
const HAS_NO_SIDE_EFFECTS_ON_IMPORT = false;
// const HAS_SIDE_EFFECTS_ON_IMPORT = true;
const importSideEffects = Object.freeze({
  fs: HAS_NO_SIDE_EFFECTS_ON_IMPORT,
  'fs/promises': HAS_NO_SIDE_EFFECTS_ON_IMPORT,
  path: HAS_NO_SIDE_EFFECTS_ON_IMPORT,
  stream: HAS_NO_SIDE_EFFECTS_ON_IMPORT,
  'prop-types/checkPropTypes': HAS_NO_SIDE_EFFECTS_ON_IMPORT,
  'react-native/Libraries/ReactPrivate/ReactNativePrivateInterface': HAS_NO_SIDE_EFFECTS_ON_IMPORT,
  scheduler: HAS_NO_SIDE_EFFECTS_ON_IMPORT,
  react: HAS_NO_SIDE_EFFECTS_ON_IMPORT,
  'react-dom/server': HAS_NO_SIDE_EFFECTS_ON_IMPORT,
  'react/jsx-dev-runtime': HAS_NO_SIDE_EFFECTS_ON_IMPORT,
  'react-fetch/node': HAS_NO_SIDE_EFFECTS_ON_IMPORT,
  'react-dom': HAS_NO_SIDE_EFFECTS_ON_IMPORT,
  url: HAS_NO_SIDE_EFFECTS_ON_IMPORT,
  ReactNativeInternalFeatureFlags: HAS_NO_SIDE_EFFECTS_ON_IMPORT,
});

// ..snip..

function getImportSideEffects() {
  return importSideEffects;
}

This code seems to be equivalent, just updated to no longer use the deprecated option.

},
external(id) {
const containsThisModule = pkg => id === pkg || id.startsWith(pkg + '/');
Expand Down Expand Up @@ -615,6 +609,8 @@ async function createBundle(bundle, bundleType) {
freeze: false,
interop: false,
esModule: false,
// Remove 'use strict' from individual source files.
strict: false,
Comment on lines +612 to +613
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure that this is something we should be doing here. See my earlier comments in this review for more context.

Suggested change
// Remove 'use strict' from individual source files.
strict: false,

},
};
const mainOutputPath = Packaging.getBundleOutputPath(
Expand All @@ -627,7 +623,8 @@ async function createBundle(bundle, bundleType) {
format,
peerGlobals,
bundle.global,
bundleType
bundleType,
isFBWWWBundle || forcePrettyOutput
Comment on lines +626 to +627
Copy link
Contributor

Choose a reason for hiding this comment

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

Why was isFBWWWBundle || forcePrettyOutput added here instead of just using the existing shouldStayReadable?

const shouldStayReadable = isFBWWWBundle || isRNBundle || forcePrettyOutput;

This ends up being passed to the isPretty arg of getRollupOutputOptions, which is then returned as sourcemap: !isPretty,

Based on my skim of the changes in this PR, I believe the only case where we aren't going to want the sourcemaps is when prettier is called, since it explicitly sets sourceMap: false,. prettier is conditionally called based on shouldStayReadable, so presumably that should be what gets used here as well.

I'd suggest reverting back to not passing this in as an arg at all, and just using the existing shouldStayReadable var directly within getRollupOutputOptions:

Suggested change
bundleType,
isFBWWWBundle || forcePrettyOutput
bundleType

);

if (isWatchMode) {
Expand Down
Loading