From 4347d391e11e4c497186e54e431f99e01a747fdd Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Fri, 12 Jul 2024 11:21:07 -0400 Subject: [PATCH 01/14] [ci] Add yarn build_and_lint gh action Basic copy of the yarn build job combined with linting from circleci without any parallelization ghstack-source-id: 849023cb13e660c11444f7b9874ca91b35222354 Pull Request resolved: https://github.com/facebook/react/pull/30070 --- .github/workflows/runtime_build.yml | 42 +++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 .github/workflows/runtime_build.yml diff --git a/.github/workflows/runtime_build.yml b/.github/workflows/runtime_build.yml new file mode 100644 index 0000000000000..7236045f38aaa --- /dev/null +++ b/.github/workflows/runtime_build.yml @@ -0,0 +1,42 @@ +name: (Runtime) Build + +on: + push: + branches: [main] + pull_request: + paths-ignore: + - compiler/** + +env: + TZ: /usr/share/zoneinfo/America/Los_Angeles + +jobs: + build_and_lint: + name: yarn build and lint + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: 18.20.1 + cache: yarn + cache-dependency-path: yarn.lock + - uses: actions/setup-java@v4 + with: + distribution: temurin + java-version: 11.0.22 + - name: Restore cached node_modules + uses: actions/cache@v4 + id: node_modules + with: + path: "**/node_modules" + key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} + - run: yarn install --frozen-lockfile + - run: yarn build + - run: yarn lint-build + - name: Cache build + uses: actions/cache@v4 + id: build_cache + with: + path: build/** + key: yarn-build-${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} From 3bc79cd41c53d46776d17e11612f7ab56eda2c35 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Fri, 12 Jul 2024 11:21:08 -0400 Subject: [PATCH 02/14] [ci] Parallelize yarn build ghstack-source-id: 8a13b456f1638a44c6f960c44f5752e8e4d32507 Pull Request resolved: https://github.com/facebook/react/pull/30071 --- .circleci/config.yml | 2 +- .github/workflows/runtime_build.yml | 26 +- scripts/rollup/build-all-release-channels.js | 120 ++- scripts/rollup/build-ghaction.js | 869 +++++++++++++++++++ scripts/rollup/stats.js | 6 + 5 files changed, 979 insertions(+), 44 deletions(-) create mode 100644 scripts/rollup/build-ghaction.js diff --git a/.circleci/config.yml b/.circleci/config.yml index 18372084d3685..a4671f4f58a64 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -97,7 +97,7 @@ jobs: steps: - checkout - setup_node_modules - - run: yarn build + - run: yarn build --ci=circleci - persist_to_workspace: root: . paths: diff --git a/.github/workflows/runtime_build.yml b/.github/workflows/runtime_build.yml index 7236045f38aaa..9df8adf1777b8 100644 --- a/.github/workflows/runtime_build.yml +++ b/.github/workflows/runtime_build.yml @@ -14,6 +14,11 @@ jobs: build_and_lint: name: yarn build and lint runs-on: ubuntu-latest + strategy: + matrix: + # yml is dumb. update the --total arg to yarn build if you change the number of workers + worker_id: [0,1,2,3,4,5,6,7,8,9,10,11,12,13,14,15,16,17,18,19] + release_channel: [stable, experimental] steps: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 @@ -32,11 +37,18 @@ jobs: path: "**/node_modules" key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} - run: yarn install --frozen-lockfile - - run: yarn build - - run: yarn lint-build - - name: Cache build - uses: actions/cache@v4 - id: build_cache + - run: yarn build --index=${{ matrix.worker_id }} --total=20 --r=${{ matrix.release_channel }} --ci=github + env: + CI: github + RELEASE_CHANNEL: ${{ matrix.release_channel }} + NODE_INDEX: ${{ matrix.worker_id }} + - name: Lint build + run: yarn lint-build + - name: Display structure of build + run: ls -R build + - name: Archive build + uses: actions/upload-artifact@v4 with: - path: build/** - key: yarn-build-${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} + name: build_${{ matrix.worker_id }}_${{ matrix.release_channel }} + path: | + build diff --git a/scripts/rollup/build-all-release-channels.js b/scripts/rollup/build-all-release-channels.js index ff6858e422873..0e43faa1f837b 100644 --- a/scripts/rollup/build-all-release-channels.js +++ b/scripts/rollup/build-all-release-channels.js @@ -15,6 +15,8 @@ const { canaryChannelLabel, rcNumber, } = require('../../ReactVersions'); +const yargs = require('yargs'); +const {buildEverything} = require('./build-ghaction'); // Runs the build script for both stable and experimental release channels, // by configuring an environment variable. @@ -51,44 +53,88 @@ fs.writeFileSync( `export default '${PLACEHOLDER_REACT_VERSION}';\n` ); -if (process.env.CIRCLE_NODE_TOTAL) { - // In CI, we use multiple concurrent processes. Allocate half the processes to - // build the stable channel, and the other half for experimental. Override - // the environment variables to "trick" the underlying build script. - const total = parseInt(process.env.CIRCLE_NODE_TOTAL, 10); - const halfTotal = Math.floor(total / 2); - const index = parseInt(process.env.CIRCLE_NODE_INDEX, 10); - if (index < halfTotal) { - const nodeTotal = halfTotal; - const nodeIndex = index; - buildForChannel('stable', nodeTotal, nodeIndex); - processStable('./build'); +const argv = yargs.wrap(yargs.terminalWidth()).options({ + releaseChannel: { + alias: 'r', + describe: 'Build the given release channel.', + requiresArg: true, + type: 'string', + choices: ['experimental', 'stable'], + }, + index: { + alias: 'i', + describe: 'Worker id.', + requiresArg: true, + type: 'number', + }, + total: { + alias: 't', + describe: 'Total number of workers.', + requiresArg: true, + type: 'number', + }, + ci: { + describe: 'Run tests in CI', + requiresArg: false, + type: 'choices', + choices: ['circleci', 'github'], + }, +}).argv; + +async function main() { + if (argv.ci === 'github') { + await buildEverything(argv.index, argv.total); + switch (argv.releaseChannel) { + case 'stable': { + processStable('./build'); + break; + } + case 'experimental': { + processExperimental('./build'); + break; + } + default: + throw new Error(`Unknown release channel ${argv.releaseChannel}`); + } + } else if (argv.ci === 'circleci') { + // In CI, we use multiple concurrent processes. Allocate half the processes to + // build the stable channel, and the other half for experimental. Override + // the environment variables to "trick" the underlying build script. + const total = parseInt(process.env.CIRCLE_NODE_TOTAL, 10); + const halfTotal = Math.floor(total / 2); + const index = parseInt(process.env.CIRCLE_NODE_INDEX, 10); + if (index < halfTotal) { + const nodeTotal = halfTotal; + const nodeIndex = index; + buildForChannel('stable', nodeTotal, nodeIndex); + processStable('./build'); + } else { + const nodeTotal = total - halfTotal; + const nodeIndex = index - halfTotal; + buildForChannel('experimental', nodeTotal, nodeIndex); + processExperimental('./build'); + } } else { - const nodeTotal = total - halfTotal; - const nodeIndex = index - halfTotal; - buildForChannel('experimental', nodeTotal, nodeIndex); - processExperimental('./build'); + // Running locally, no concurrency. Move each channel's build artifacts into + // a temporary directory so that they don't conflict. + buildForChannel('stable', '', ''); + const stableDir = tmp.dirSync().name; + crossDeviceRenameSync('./build', stableDir); + processStable(stableDir); + buildForChannel('experimental', '', ''); + const experimentalDir = tmp.dirSync().name; + crossDeviceRenameSync('./build', experimentalDir); + processExperimental(experimentalDir); + + // Then merge the experimental folder into the stable one. processExperimental + // will have already removed conflicting files. + // + // In CI, merging is handled automatically by CircleCI's workspace feature. + mergeDirsSync(experimentalDir + '/', stableDir + '/'); + + // Now restore the combined directory back to its original name + crossDeviceRenameSync(stableDir, './build'); } -} else { - // Running locally, no concurrency. Move each channel's build artifacts into - // a temporary directory so that they don't conflict. - buildForChannel('stable', '', ''); - const stableDir = tmp.dirSync().name; - crossDeviceRenameSync('./build', stableDir); - processStable(stableDir); - buildForChannel('experimental', '', ''); - const experimentalDir = tmp.dirSync().name; - crossDeviceRenameSync('./build', experimentalDir); - processExperimental(experimentalDir); - - // Then merge the experimental folder into the stable one. processExperimental - // will have already removed conflicting files. - // - // In CI, merging is handled automatically by CircleCI's workspace feature. - mergeDirsSync(experimentalDir + '/', stableDir + '/'); - - // Now restore the combined directory back to its original name - crossDeviceRenameSync(stableDir, './build'); } function buildForChannel(channel, nodeTotal, nodeIndex) { @@ -455,3 +501,5 @@ function mergeDirsSync(source, destination) { } } } + +main(); diff --git a/scripts/rollup/build-ghaction.js b/scripts/rollup/build-ghaction.js new file mode 100644 index 0000000000000..08ac889aba43d --- /dev/null +++ b/scripts/rollup/build-ghaction.js @@ -0,0 +1,869 @@ +'use strict'; + +const rollup = require('rollup'); +const babel = require('@rollup/plugin-babel').babel; +const closure = require('./plugins/closure-plugin'); +const flowRemoveTypes = require('flow-remove-types'); +const prettier = require('rollup-plugin-prettier'); +const replace = require('@rollup/plugin-replace'); +const stripBanner = require('rollup-plugin-strip-banner'); +const chalk = require('chalk'); +const resolve = require('@rollup/plugin-node-resolve').nodeResolve; +const fs = require('fs'); +const argv = require('minimist')(process.argv.slice(2)); +const Modules = require('./modules'); +const Bundles = require('./bundles'); +const Stats = require('./stats'); +const Sync = require('./sync'); +const sizes = require('./plugins/sizes-plugin'); +const useForks = require('./plugins/use-forks-plugin'); +const dynamicImports = require('./plugins/dynamic-imports'); +const Packaging = require('./packaging'); +const {asyncRimRaf} = require('./utils'); +const codeFrame = require('@babel/code-frame'); +const Wrappers = require('./wrappers'); + +const RELEASE_CHANNEL = process.env.RELEASE_CHANNEL; + +// Default to building in experimental mode. If the release channel is set via +// an environment variable, then check if it's "experimental". +const __EXPERIMENTAL__ = + typeof RELEASE_CHANNEL === 'string' + ? RELEASE_CHANNEL === 'experimental' + : true; + +// Errors in promises should be fatal. +let loggedErrors = new Set(); +process.on('unhandledRejection', err => { + if (loggedErrors.has(err)) { + // No need to print it twice. + process.exit(1); + } + throw err; +}); + +const { + NODE_ES2015, + ESM_DEV, + ESM_PROD, + NODE_DEV, + NODE_PROD, + NODE_PROFILING, + BUN_DEV, + BUN_PROD, + FB_WWW_DEV, + FB_WWW_PROD, + FB_WWW_PROFILING, + RN_OSS_DEV, + RN_OSS_PROD, + RN_OSS_PROFILING, + RN_FB_DEV, + RN_FB_PROD, + RN_FB_PROFILING, + BROWSER_SCRIPT, +} = Bundles.bundleTypes; + +const {getFilename} = Bundles; + +function parseRequestedNames(names, toCase) { + let result = []; + for (let i = 0; i < names.length; i++) { + let splitNames = names[i].split(','); + for (let j = 0; j < splitNames.length; j++) { + let name = splitNames[j].trim(); + if (!name) { + continue; + } + if (toCase === 'uppercase') { + name = name.toUpperCase(); + } else if (toCase === 'lowercase') { + name = name.toLowerCase(); + } + result.push(name); + } + } + return result; +} + +const argvType = Array.isArray(argv.type) ? argv.type : [argv.type]; +const requestedBundleTypes = argv.type + ? parseRequestedNames(argvType, 'uppercase') + : []; + +const requestedBundleNames = parseRequestedNames(argv._, 'lowercase'); +const forcePrettyOutput = argv.pretty; +const isWatchMode = argv.watch; +const syncFBSourcePath = argv['sync-fbsource']; +const syncWWWPath = argv['sync-www']; + +// Non-ES2015 stuff applied before closure compiler. +const babelPlugins = [ + // These plugins filter out non-ES2015. + ['@babel/plugin-proposal-class-properties', {loose: true}], + 'syntax-trailing-function-commas', + // These use loose mode which avoids embedding a runtime. + // TODO: Remove object spread from the source. Prefer Object.assign instead. + [ + '@babel/plugin-proposal-object-rest-spread', + {loose: true, useBuiltIns: true}, + ], + ['@babel/plugin-transform-template-literals', {loose: true}], + // TODO: Remove for...of from the source. It requires a runtime to be embedded. + '@babel/plugin-transform-for-of', + // TODO: Remove array spread from the source. Prefer .apply instead. + ['@babel/plugin-transform-spread', {loose: true, useBuiltIns: true}], + '@babel/plugin-transform-parameters', + // TODO: Remove array destructuring from the source. Requires runtime. + ['@babel/plugin-transform-destructuring', {loose: true, useBuiltIns: true}], + // Transform Object spread to shared/assign + require('../babel/transform-object-assign'), +]; + +const babelToES5Plugins = [ + // These plugins transform DEV mode. Closure compiler deals with these in PROD. + '@babel/plugin-transform-literals', + '@babel/plugin-transform-arrow-functions', + '@babel/plugin-transform-block-scoped-functions', + '@babel/plugin-transform-shorthand-properties', + '@babel/plugin-transform-computed-properties', + ['@babel/plugin-transform-block-scoping', {throwIfClosureRequired: true}], +]; + +function getBabelConfig( + updateBabelOptions, + bundleType, + packageName, + externals, + isDevelopment, + bundle +) { + const canAccessReactObject = + packageName === 'react' || externals.indexOf('react') !== -1; + let options = { + exclude: '/**/node_modules/**', + babelrc: false, + configFile: false, + presets: [], + plugins: [...babelPlugins], + babelHelpers: 'bundled', + sourcemap: false, + }; + if (isDevelopment) { + options.plugins.push( + ...babelToES5Plugins, + // Turn console.error/warn() into a custom wrapper + [ + require('../babel/transform-replace-console-calls'), + { + shouldError: !canAccessReactObject, + }, + ] + ); + } + if (updateBabelOptions) { + options = updateBabelOptions(options); + } + // Controls whether to replace error messages with error codes in production. + // By default, error messages are replaced in production. + if (!isDevelopment && bundle.minifyWithProdErrorCodes !== false) { + options.plugins.push(require('../error-codes/transform-error-messages')); + } + + return options; +} + +let getRollupInteropValue = id => { + // We're setting Rollup to assume that imports are ES modules unless otherwise specified. + // However, we also compile ES import syntax to `require()` using Babel. + // This causes Rollup to turn uses of `import SomeDefaultImport from 'some-module' into + // references to `SomeDefaultImport.default` due to CJS/ESM interop. + // Some CJS modules don't have a `.default` export, and the rewritten import is incorrect. + // Specifying `interop: 'default'` instead will have Rollup use the imported variable as-is, + // without adding a `.default` to the reference. + const modulesWithCommonJsExports = [ + 'art/core/transform', + 'art/modes/current', + 'art/modes/fast-noSideEffects', + 'art/modes/svg', + 'JSResourceReferenceImpl', + 'error-stack-parser', + 'neo-async', + 'webpack/lib/dependencies/ModuleDependency', + 'webpack/lib/dependencies/NullDependency', + 'webpack/lib/Template', + ]; + + if (modulesWithCommonJsExports.includes(id)) { + return 'default'; + } + + // For all other modules, handle imports without any import helper utils + return 'esModule'; +}; + +function getRollupOutputOptions( + outputPath, + format, + globals, + globalName, + bundleType +) { + const isProduction = isProductionBundleType(bundleType); + + return { + file: outputPath, + format, + globals, + freeze: !isProduction, + interop: getRollupInteropValue, + name: globalName, + sourcemap: false, + esModule: false, + exports: 'auto', + }; +} + +function getFormat(bundleType) { + switch (bundleType) { + case NODE_ES2015: + case NODE_DEV: + case NODE_PROD: + case NODE_PROFILING: + case BUN_DEV: + case BUN_PROD: + case FB_WWW_DEV: + case FB_WWW_PROD: + case FB_WWW_PROFILING: + case RN_OSS_DEV: + case RN_OSS_PROD: + case RN_OSS_PROFILING: + case RN_FB_DEV: + case RN_FB_PROD: + case RN_FB_PROFILING: + return `cjs`; + case ESM_DEV: + case ESM_PROD: + return `es`; + case BROWSER_SCRIPT: + return `iife`; + } +} + +function isProductionBundleType(bundleType) { + switch (bundleType) { + case NODE_ES2015: + return true; + case ESM_DEV: + case NODE_DEV: + case BUN_DEV: + case FB_WWW_DEV: + case RN_OSS_DEV: + case RN_FB_DEV: + return false; + case ESM_PROD: + case NODE_PROD: + case BUN_PROD: + case NODE_PROFILING: + case FB_WWW_PROD: + case FB_WWW_PROFILING: + case RN_OSS_PROD: + case RN_OSS_PROFILING: + case RN_FB_PROD: + case RN_FB_PROFILING: + case BROWSER_SCRIPT: + return true; + default: + throw new Error(`Unknown type: ${bundleType}`); + } +} + +function isProfilingBundleType(bundleType) { + switch (bundleType) { + case NODE_ES2015: + case FB_WWW_DEV: + case FB_WWW_PROD: + case NODE_DEV: + case NODE_PROD: + case BUN_DEV: + case BUN_PROD: + case RN_FB_DEV: + case RN_FB_PROD: + case RN_OSS_DEV: + case RN_OSS_PROD: + case ESM_DEV: + case ESM_PROD: + case BROWSER_SCRIPT: + return false; + case FB_WWW_PROFILING: + case NODE_PROFILING: + case RN_FB_PROFILING: + case RN_OSS_PROFILING: + return true; + default: + throw new Error(`Unknown type: ${bundleType}`); + } +} + +function getBundleTypeFlags(bundleType) { + const isFBWWWBundle = + bundleType === FB_WWW_DEV || + bundleType === FB_WWW_PROD || + bundleType === FB_WWW_PROFILING; + const isRNBundle = + bundleType === RN_OSS_DEV || + bundleType === RN_OSS_PROD || + bundleType === RN_OSS_PROFILING || + bundleType === RN_FB_DEV || + bundleType === RN_FB_PROD || + bundleType === RN_FB_PROFILING; + + const isFBRNBundle = + bundleType === RN_FB_DEV || + bundleType === RN_FB_PROD || + bundleType === RN_FB_PROFILING; + + const shouldStayReadable = isFBWWWBundle || isRNBundle || forcePrettyOutput; + + return { + isFBWWWBundle, + isRNBundle, + isFBRNBundle, + shouldStayReadable, + }; +} + +function forbidFBJSImports() { + return { + name: 'forbidFBJSImports', + resolveId(importee, importer) { + if (/^fbjs\//.test(importee)) { + throw new Error( + `Don't import ${importee} (found in ${importer}). ` + + `Use the utilities in packages/shared/ instead.` + ); + } + }, + }; +} + +function getPlugins( + entry, + externals, + updateBabelOptions, + filename, + packageName, + bundleType, + globalName, + moduleType, + pureExternalModules, + bundle +) { + try { + const forks = Modules.getForks(bundleType, entry, moduleType, bundle); + const isProduction = isProductionBundleType(bundleType); + const isProfiling = isProfilingBundleType(bundleType); + + const needsMinifiedByClosure = + bundleType !== ESM_PROD && bundleType !== ESM_DEV; + + return [ + // Keep dynamic imports as externals + dynamicImports(), + { + name: 'rollup-plugin-flow-remove-types', + transform(code) { + const transformed = flowRemoveTypes(code); + return { + code: transformed.toString(), + map: null, + }; + }, + }, + // 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, // TODO: options.skip was removed in @rollup/plugin-node-resolve 3.0.0 + }), + // Remove license headers from individual modules + stripBanner({ + exclude: 'node_modules/**/*', + }), + // Compile to ES2015. + babel( + getBabelConfig( + updateBabelOptions, + bundleType, + packageName, + externals, + !isProduction, + bundle + ) + ), + // Remove 'use strict' from individual source files. + { + name: "remove 'use strict'", + transform(source) { + return source.replace(/['"]use strict["']/g, ''); + }, + }, + // Turn __DEV__ and process.env checks into constants. + replace({ + preventAssignment: true, + values: { + __DEV__: isProduction ? 'false' : 'true', + __PROFILE__: isProfiling || !isProduction ? 'true' : 'false', + 'process.env.NODE_ENV': isProduction + ? "'production'" + : "'development'", + __EXPERIMENTAL__, + }, + }), + { + name: 'top-level-definitions', + renderChunk(source) { + return Wrappers.wrapWithTopLevelDefinitions( + source, + bundleType, + globalName, + filename, + moduleType, + bundle.wrapWithModuleBoundaries + ); + }, + }, + // For production builds, compile with Closure. We do this even for the + // "non-minified" production builds because Closure is much better at + // minification than what most applications use. During this step, we do + // preserve the original symbol names, though, so the resulting code is + // relatively readable. + // + // For the minified builds, the names will be mangled later. + // + // We don't bother with sourcemaps at this step. The sourcemaps we publish + // are only for whitespace and symbol renaming; they don't map back to + // before Closure was applied. + needsMinifiedByClosure && + closure({ + compilation_level: 'SIMPLE', + language_in: 'ECMASCRIPT_2020', + language_out: + bundleType === NODE_ES2015 + ? 'ECMASCRIPT_2020' + : bundleType === BROWSER_SCRIPT + ? 'ECMASCRIPT5' + : 'ECMASCRIPT5_STRICT', + emit_use_strict: + bundleType !== BROWSER_SCRIPT && + bundleType !== ESM_PROD && + bundleType !== ESM_DEV, + env: 'CUSTOM', + warning_level: 'QUIET', + source_map_include_content: true, + use_types_for_optimization: false, + process_common_js_modules: false, + rewrite_polyfills: false, + inject_libraries: false, + allow_dynamic_import: true, + + // Don't let it create global variables in the browser. + // https://github.com/facebook/react/issues/10909 + assume_function_wrapper: true, + + // Don't rename symbols (variable names, functions, etc). We leave + // this up to the application to handle, if they want. Otherwise gzip + // takes care of it. + renaming: false, + }), + needsMinifiedByClosure && + // Add the whitespace back + prettier({ + parser: 'flow', + singleQuote: false, + trailingComma: 'none', + bracketSpacing: true, + }), + { + name: 'license-and-signature-header', + renderChunk(source) { + return Wrappers.wrapWithLicenseHeader( + source, + bundleType, + globalName, + filename, + moduleType + ); + }, + }, + // Record bundle size. + sizes({ + getSize: (size, gzip) => { + const currentSizes = Stats.currentBuildResults.bundleSizes; + const recordIndex = currentSizes.findIndex( + record => + record.filename === filename && record.bundleType === bundleType + ); + const index = recordIndex !== -1 ? recordIndex : currentSizes.length; + currentSizes[index] = { + filename, + bundleType, + packageName, + size, + gzip, + }; + }, + }), + ].filter(Boolean); + } catch (error) { + console.error( + chalk.red(`There was an error preparing plugins for entry "${entry}"`) + ); + throw error; + } +} + +function shouldSkipBundle(bundle, bundleType) { + const shouldSkipBundleType = bundle.bundleTypes.indexOf(bundleType) === -1; + if (shouldSkipBundleType) { + return true; + } + if (requestedBundleTypes.length > 0) { + const isAskingForDifferentType = requestedBundleTypes.some( + requestedType => !bundleType.includes(requestedType) + ); + if (isAskingForDifferentType) { + return true; + } + } + if (requestedBundleNames.length > 0) { + // If the name ends with `something/index` we only match if the + // entry ends in something. Such as `react-dom/index` only matches + // `react-dom` but not `react-dom/server`. Everything else is fuzzy + // search. + const entryLowerCase = bundle.entry.toLowerCase() + '/index.js'; + const isAskingForDifferentNames = requestedBundleNames.every( + requestedName => { + const matchEntry = entryLowerCase.indexOf(requestedName) !== -1; + if (!bundle.name) { + return !matchEntry; + } + const matchName = + bundle.name.toLowerCase().indexOf(requestedName) !== -1; + return !matchEntry && !matchName; + } + ); + if (isAskingForDifferentNames) { + return true; + } + } + return false; +} + +function resolveEntryFork(resolvedEntry, isFBBundle) { + // Pick which entry point fork to use: + // .modern.fb.js + // .classic.fb.js + // .fb.js + // .stable.js + // .experimental.js + // .js + // or any of those plus .development.js + + if (isFBBundle) { + const resolvedFBEntry = resolvedEntry.replace( + '.js', + __EXPERIMENTAL__ ? '.modern.fb.js' : '.classic.fb.js' + ); + const developmentFBEntry = resolvedFBEntry.replace( + '.js', + '.development.js' + ); + if (fs.existsSync(developmentFBEntry)) { + return developmentFBEntry; + } + if (fs.existsSync(resolvedFBEntry)) { + return resolvedFBEntry; + } + const resolvedGenericFBEntry = resolvedEntry.replace('.js', '.fb.js'); + const developmentGenericFBEntry = resolvedGenericFBEntry.replace( + '.js', + '.development.js' + ); + if (fs.existsSync(developmentGenericFBEntry)) { + return developmentGenericFBEntry; + } + if (fs.existsSync(resolvedGenericFBEntry)) { + return resolvedGenericFBEntry; + } + // Even if it's a FB bundle we fallthrough to pick stable or experimental if we don't have an FB fork. + } + const resolvedForkedEntry = resolvedEntry.replace( + '.js', + __EXPERIMENTAL__ ? '.experimental.js' : '.stable.js' + ); + const devForkedEntry = resolvedForkedEntry.replace('.js', '.development.js'); + if (fs.existsSync(devForkedEntry)) { + return devForkedEntry; + } + if (fs.existsSync(resolvedForkedEntry)) { + return resolvedForkedEntry; + } + // Just use the plain .js one. + return resolvedEntry; +} + +async function createBundle(bundle, bundleType) { + const filename = getFilename(bundle, bundleType); + const logKey = + chalk.white.bold(filename) + chalk.dim(` (${bundleType.toLowerCase()})`); + const format = getFormat(bundleType); + const packageName = Packaging.getPackageName(bundle.entry); + + const {isFBWWWBundle, isFBRNBundle} = getBundleTypeFlags(bundleType); + + let resolvedEntry = resolveEntryFork( + require.resolve(bundle.entry), + isFBWWWBundle || isFBRNBundle, + !isProductionBundleType(bundleType) + ); + + const peerGlobals = Modules.getPeerGlobals(bundle.externals, bundleType); + let externals = Object.keys(peerGlobals); + + const deps = Modules.getDependencies(bundleType, bundle.entry); + externals = externals.concat(deps); + + const importSideEffects = Modules.getImportSideEffects(); + const pureExternalModules = Object.keys(importSideEffects).filter( + module => !importSideEffects[module] + ); + + const rollupConfig = { + input: resolvedEntry, + treeshake: { + moduleSideEffects: (id, external) => + !(external && pureExternalModules.includes(id)), + propertyReadSideEffects: false, + }, + external(id) { + const containsThisModule = pkg => id === pkg || id.startsWith(pkg + '/'); + const isProvidedByDependency = externals.some(containsThisModule); + if (isProvidedByDependency) { + if (id.indexOf('/src/') !== -1) { + throw Error( + 'You are trying to import ' + + id + + ' but ' + + externals.find(containsThisModule) + + ' is one of npm dependencies, ' + + 'so it will not contain that source file. You probably want ' + + 'to create a new bundle entry point for it instead.' + ); + } + return true; + } + return !!peerGlobals[id]; + }, + onwarn: handleRollupWarning, + plugins: getPlugins( + bundle.entry, + externals, + bundle.babel, + filename, + packageName, + bundleType, + bundle.global, + bundle.moduleType, + pureExternalModules, + bundle + ), + output: { + externalLiveBindings: false, + freeze: false, + interop: getRollupInteropValue, + esModule: false, + }, + }; + const mainOutputPath = Packaging.getBundleOutputPath( + bundle, + bundleType, + filename, + packageName + ); + + const rollupOutputOptions = getRollupOutputOptions( + mainOutputPath, + format, + peerGlobals, + bundle.global, + bundleType + ); + + if (isWatchMode) { + rollupConfig.output = [rollupOutputOptions]; + const watcher = rollup.watch(rollupConfig); + watcher.on('event', async event => { + switch (event.code) { + case 'BUNDLE_START': + console.log(`${chalk.bgYellow.black(' BUILDING ')} ${logKey}`); + break; + case 'BUNDLE_END': + console.log(`${chalk.bgGreen.black(' COMPLETE ')} ${logKey}\n`); + break; + case 'ERROR': + case 'FATAL': + console.log(`${chalk.bgRed.black(' OH NOES! ')} ${logKey}\n`); + handleRollupError(event.error); + break; + } + }); + } else { + console.log(`${chalk.bgYellow.black(' BUILDING ')} ${logKey}`); + try { + const result = await rollup.rollup(rollupConfig); + await result.write(rollupOutputOptions); + } catch (error) { + console.log(`${chalk.bgRed.black(' OH NOES! ')} ${logKey}\n`); + handleRollupError(error); + throw error; + } + console.log(`${chalk.bgGreen.black(' COMPLETE ')} ${logKey}\n`); + } +} + +function handleRollupWarning(warning) { + if (warning.code === 'UNUSED_EXTERNAL_IMPORT') { + const match = warning.message.match(/external module "([^"]+)"/); + if (!match || typeof match[1] !== 'string') { + throw new Error( + 'Could not parse a Rollup warning. ' + 'Fix this method.' + ); + } + const importSideEffects = Modules.getImportSideEffects(); + const externalModule = match[1]; + if (typeof importSideEffects[externalModule] !== 'boolean') { + throw new Error( + 'An external module "' + + externalModule + + '" is used in a DEV-only code path ' + + 'but we do not know if it is safe to omit an unused require() to it in production. ' + + 'Please add it to the `importSideEffects` list in `scripts/rollup/modules.js`.' + ); + } + // Don't warn. We will remove side effectless require() in a later pass. + return; + } + + if (warning.code === 'CIRCULAR_DEPENDENCY') { + // Ignored + } else if (typeof warning.code === 'string') { + // This is a warning coming from Rollup itself. + // These tend to be important (e.g. clashes in namespaced exports) + // so we'll fail the build on any of them. + console.error(); + console.error(warning.message || warning); + console.error(); + process.exit(1); + } else { + // The warning is from one of the plugins. + // Maybe it's not important, so just print it. + console.warn(warning.message || warning); + } +} + +function handleRollupError(error) { + loggedErrors.add(error); + if (!error.code) { + console.error(error); + return; + } + console.error( + `\x1b[31m-- ${error.code}${error.plugin ? ` (${error.plugin})` : ''} --` + ); + console.error(error.stack); + if (error.loc && error.loc.file) { + const {file, line, column} = error.loc; + // This looks like an error from Rollup, e.g. missing export. + // We'll use the accurate line numbers provided by Rollup but + // use Babel code frame because it looks nicer. + const rawLines = fs.readFileSync(file, 'utf-8'); + // column + 1 is required due to rollup counting column start position from 0 + // whereas babel-code-frame counts from 1 + const frame = codeFrame(rawLines, line, column + 1, { + highlightCode: true, + }); + console.error(frame); + } else if (error.codeFrame) { + // This looks like an error from a plugin (e.g. Babel). + // In this case we'll resort to displaying the provided code frame + // because we can't be sure the reported location is accurate. + console.error(error.codeFrame); + } +} + +async function buildEverything(index, total) { + if (!argv['unsafe-partial']) { + await asyncRimRaf('build'); + } + + // Run them serially for better console output + // and to avoid any potential race conditions. + + let bundles = []; + // eslint-disable-next-line no-for-of-loops/no-for-of-loops + for (const bundle of Bundles.bundles) { + bundles.push( + [bundle, NODE_ES2015], + [bundle, ESM_DEV], + [bundle, ESM_PROD], + [bundle, NODE_DEV], + [bundle, NODE_PROD], + [bundle, NODE_PROFILING], + [bundle, BUN_DEV], + [bundle, BUN_PROD], + [bundle, FB_WWW_DEV], + [bundle, FB_WWW_PROD], + [bundle, FB_WWW_PROFILING], + [bundle, RN_OSS_DEV], + [bundle, RN_OSS_PROD], + [bundle, RN_OSS_PROFILING], + [bundle, RN_FB_DEV], + [bundle, RN_FB_PROD], + [bundle, RN_FB_PROFILING], + [bundle, BROWSER_SCRIPT] + ); + } + + bundles = bundles.filter(([bundle, bundleType]) => { + return !shouldSkipBundle(bundle, bundleType); + }); + + const nodeTotal = parseInt(total, 10); + const nodeIndex = parseInt(index, 10); + bundles = bundles.filter((_, i) => i % nodeTotal === nodeIndex); + + // eslint-disable-next-line no-for-of-loops/no-for-of-loops + for (const [bundle, bundleType] of bundles) { + await createBundle(bundle, bundleType); + } + + await Packaging.copyAllShims(); + await Packaging.prepareNpmPackages(); + + if (syncFBSourcePath) { + await Sync.syncReactNative(syncFBSourcePath); + } else if (syncWWWPath) { + await Sync.syncReactDom('build/facebook-www', syncWWWPath); + } + + console.log(Stats.printResults()); + if (!forcePrettyOutput) { + Stats.saveResults(); + } +} + +module.exports = { + buildEverything, +}; diff --git a/scripts/rollup/stats.js b/scripts/rollup/stats.js index 5a5b376c8b1a2..3763816fc9b53 100644 --- a/scripts/rollup/stats.js +++ b/scripts/rollup/stats.js @@ -28,6 +28,12 @@ function saveResults() { join('build', 'sizes', `bundle-sizes-${nodeIndex}.json`), JSON.stringify(currentBuildResults, null, 2) ); + } else if (process.env.CI === 'github') { + mkdirp.sync('build/sizes'); + fs.writeFileSync( + join('build', 'sizes', `bundle-sizes-${process.env.NODE_INDEX}.json`), + JSON.stringify(currentBuildResults, null, 2) + ); } else { // Write all the bundle sizes to a single JSON file. fs.writeFileSync( From bc576bfabec92438dddffe2db60e86d16268198d Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Fri, 12 Jul 2024 11:21:08 -0400 Subject: [PATCH 03/14] [ci] Add process_artifacts_combined job ghstack-source-id: 784640da83c93c49f1d924f8fcb848c7dc7d9e1c Pull Request resolved: https://github.com/facebook/react/pull/30073 --- .github/workflows/runtime_build.yml | 42 +++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/.github/workflows/runtime_build.yml b/.github/workflows/runtime_build.yml index 9df8adf1777b8..89eb83ead6270 100644 --- a/.github/workflows/runtime_build.yml +++ b/.github/workflows/runtime_build.yml @@ -52,3 +52,45 @@ jobs: name: build_${{ matrix.worker_id }}_${{ matrix.release_channel }} path: | build + + process_artifacts_combined: + name: Process artifacts combined + needs: build_and_lint + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: 18.20.1 + cache: yarn + cache-dependency-path: yarn.lock + - name: Restore cached node_modules + uses: actions/cache@v4 + id: node_modules + with: + path: "**/node_modules" + key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} + - run: yarn install --frozen-lockfile + - name: Restore archived build + uses: actions/download-artifact@v4 + with: + path: build + merge-multiple: true + - name: Display structure of build + run: ls -R build + - run: echo ${{ github.sha }} >> build/COMMIT_SHA + - name: Scrape warning messages + run: | + mkdir -p ./build/__test_utils__ + node ./scripts/print-warnings/print-warnings.js > build/__test_utils__/ReactAllWarnings.js + # Compress build directory into a single tarball for easy download + - run: tar -zcvf ./build.tgz ./build + # TODO: Migrate scripts to use `build` directory instead of `build2` + - run: cp ./build.tgz ./build2.tgz + - name: Archive build artifacts + uses: actions/upload-artifact@v4 + with: + name: combined_artifacts_${{ github.sha }} + path: | + ./build.tgz + ./build2.tgz From a6b895916965cb6563a5238aa6eccfff984e40b4 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Fri, 12 Jul 2024 11:21:09 -0400 Subject: [PATCH 04/14] [ci] Add yarn_test_build job to gh actions ghstack-source-id: 5cff9778611acb747c05b9d353bd1af3b11e7d7a Pull Request resolved: https://github.com/facebook/react/pull/30072 --- .github/workflows/runtime_build.yml | 60 +++++++++++++++++++ .../storeStressTestConcurrent-test.js | 2 + 2 files changed, 62 insertions(+) diff --git a/.github/workflows/runtime_build.yml b/.github/workflows/runtime_build.yml index 89eb83ead6270..1543af1c2e51f 100644 --- a/.github/workflows/runtime_build.yml +++ b/.github/workflows/runtime_build.yml @@ -53,6 +53,66 @@ jobs: path: | build + test_build: + name: yarn test-build + needs: build_and_lint + strategy: + matrix: + test_params: [ + # Intentionally passing these as strings instead of creating a + # separate parameter per CLI argument, since it's easier to + # control/see which combinations we want to run. + -r=stable --env=development, + -r=stable --env=production, + -r=experimental --env=development, + -r=experimental --env=production, + + # Dev Tools + --project=devtools -r=experimental, + + # TODO: Update test config to support www build tests + # - "-r=www-classic --env=development --variant=false" + # - "-r=www-classic --env=production --variant=false" + # - "-r=www-classic --env=development --variant=true" + # - "-r=www-classic --env=production --variant=true" + # - "-r=www-modern --env=development --variant=false" + # - "-r=www-modern --env=production --variant=false" + # - "-r=www-modern --env=development --variant=true" + # - "-r=www-modern --env=production --variant=true" + + # TODO: Update test config to support xplat build tests + # - "-r=xplat --env=development --variant=false" + # - "-r=xplat --env=development --variant=true" + # - "-r=xplat --env=production --variant=false" + # - "-r=xplat --env=production --variant=true" + + # TODO: Test more persistent configurations? + ] + continue-on-error: true + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: 18.20.1 + cache: yarn + cache-dependency-path: yarn.lock + - name: Restore cached node_modules + uses: actions/cache@v4 + id: node_modules + with: + path: "**/node_modules" + key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} + - run: yarn install --frozen-lockfile + - name: Restore archived build + uses: actions/download-artifact@v4 + with: + path: build + merge-multiple: true + - name: Display structure of build + run: ls -R build + - run: yarn test --build ${{ matrix.test_params }} --ci=github + process_artifacts_combined: name: Process artifacts combined needs: build_and_lint diff --git a/packages/react-devtools-shared/src/__tests__/storeStressTestConcurrent-test.js b/packages/react-devtools-shared/src/__tests__/storeStressTestConcurrent-test.js index 472ad31671a76..8dd4ce428438f 100644 --- a/packages/react-devtools-shared/src/__tests__/storeStressTestConcurrent-test.js +++ b/packages/react-devtools-shared/src/__tests__/storeStressTestConcurrent-test.js @@ -16,6 +16,8 @@ describe('StoreStressConcurrent', () => { let store; let print; + jest.setTimeout(15000); + beforeEach(() => { global.IS_REACT_ACT_ENVIRONMENT = true; From df5f273629d8da586351573347e8b32ba19dafa7 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Fri, 12 Jul 2024 11:21:10 -0400 Subject: [PATCH 05/14] [ci] Add check_release_dependencies job ghstack-source-id: 492e7e0a299b146d237f1456fedd5a95789c1636 Pull Request resolved: https://github.com/facebook/react/pull/30169 --- .github/workflows/runtime_build.yml | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/.github/workflows/runtime_build.yml b/.github/workflows/runtime_build.yml index 1543af1c2e51f..304f03482ddbd 100644 --- a/.github/workflows/runtime_build.yml +++ b/.github/workflows/runtime_build.yml @@ -154,3 +154,30 @@ jobs: path: | ./build.tgz ./build2.tgz + + check_release_dependencies: + name: Check release dependencies + needs: build_and_lint + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: 18.20.1 + cache: yarn + cache-dependency-path: yarn.lock + - name: Restore cached node_modules + uses: actions/cache@v4 + id: node_modules + with: + path: "**/node_modules" + key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} + - run: yarn install --frozen-lockfile + - name: Restore archived build + uses: actions/download-artifact@v4 + with: + path: build + merge-multiple: true + - name: Display structure of build + run: ls -R build + - run: yarn check-release-dependencies From ff3f1fac65e7b82efe3f90af42af1278bc6f1d5d Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Fri, 12 Jul 2024 11:21:10 -0400 Subject: [PATCH 06/14] [RN] Set enableOwnerStacks and enableUseDeferredValueInitialArg to false In www, the experimental versions get a .modern.js or .classic.js prefix and get copied into the same folder. In RN, they don't seem to have .modern.js and .classic.js versions so they end up getting the same name. sebmarkbage's theory is that what happens is that they then override the file that was already there. So depending on if experimental or stable build finishes first you get a different version at the end. It doesn't make sense to use `__EXPERIMENTAL__` for flags in native-fb since there's no modern/classic split there. So that flag should just be hardcoded to true or false and then it doesn't matter which one finishes first. We don't support experimental builds in OSS RN neither so the same thing could happen with [`enableOwnerStacks`](https://github.com/facebook/react/blob/5dcf3ca8d45a276a8b4cee0cedd234967661ca35/packages/shared/forks/ReactFeatureFlags.native-oss.js#L60). You can see that the build errors in the previous PR but passes after these flag changes. ghstack-source-id: d10f37bcea0e485fdb4f136370c179999badd560 Pull Request resolved: https://github.com/facebook/react/pull/30322 --- packages/shared/forks/ReactFeatureFlags.native-oss.js | 2 +- .../shared/forks/ReactFeatureFlags.test-renderer.native-fb.js | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/packages/shared/forks/ReactFeatureFlags.native-oss.js b/packages/shared/forks/ReactFeatureFlags.native-oss.js index 15caf9713bf75..3ef19e4b127e7 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-oss.js +++ b/packages/shared/forks/ReactFeatureFlags.native-oss.js @@ -57,7 +57,7 @@ export const enableLegacyFBSupport = false; export const enableLegacyHidden = false; export const enableNoCloningMemoCache = false; export const enableObjectFiber = false; -export const enableOwnerStacks = __EXPERIMENTAL__; +export const enableOwnerStacks = false; export const enablePostpone = false; export const enableReactTestRendererWarning = false; export const enableRefAsProp = true; diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js index d5429617569ca..803d669460611 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.native-fb.js @@ -69,7 +69,7 @@ export const enableTaint = true; export const enableTransitionTracing = false; export const enableTrustedTypesIntegration = false; export const enableUpdaterTracking = false; -export const enableUseDeferredValueInitialArg = __EXPERIMENTAL__; +export const enableUseDeferredValueInitialArg = true; export const enableUseEffectEventHook = false; export const enableUseMemoCacheHook = true; export const favorSafetyOverHydrationPerf = true; From 922c7971ba3e52adf46678774bef3022d1aad4e0 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Fri, 12 Jul 2024 12:26:30 -0400 Subject: [PATCH 07/14] [ci] Standardize node version and timezones Quick change to standardize on a single timezone across all workflows and to use the same version of node (18.20.1). Also updates .nvmrc ghstack-source-id: e1d43006ec018acfcd88444feadde1b1d260de9d Pull Request resolved: https://github.com/facebook/react/pull/30323 --- .github/workflows/compiler_playground.yml | 9 ++++++--- .github/workflows/compiler_rust.yml | 1 + .github/workflows/compiler_typescript.yml | 17 ++++++++++------- .github/workflows/devtools_check_repro.yml | 3 +++ .../workflows/runtime_commit_artifacts.yml | 3 +++ .github/workflows/runtime_fizz.yml | 9 ++++++--- .github/workflows/runtime_flags.yml | 9 ++++++--- .github/workflows/runtime_flow.yml | 9 ++++++--- .github/workflows/runtime_fuzz_tests.yml | 4 ++++ .github/workflows/runtime_test.yml | 9 ++++++--- .github/workflows/shared_lint.yml | 19 +++++++++++-------- .github/workflows/shared_stale.yml | 3 +++ .nvmrc | 2 +- 13 files changed, 66 insertions(+), 31 deletions(-) diff --git a/.github/workflows/compiler_playground.yml b/.github/workflows/compiler_playground.yml index 7f2a75d324d77..eb48a9b210b22 100644 --- a/.github/workflows/compiler_playground.yml +++ b/.github/workflows/compiler_playground.yml @@ -5,9 +5,12 @@ on: branches: [main] pull_request: paths: - - "compiler/**" + - compiler/** - .github/workflows/compiler-playground.yml +env: + TZ: /usr/share/zoneinfo/America/Los_Angeles + defaults: run: working-directory: compiler @@ -20,8 +23,8 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: - node-version: 18.x - cache: "yarn" + node-version: 18.20.1 + cache: yarn cache-dependency-path: compiler/yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 diff --git a/.github/workflows/compiler_rust.yml b/.github/workflows/compiler_rust.yml index cf911b9d476b3..3cda1325ca49e 100644 --- a/.github/workflows/compiler_rust.yml +++ b/.github/workflows/compiler_rust.yml @@ -18,6 +18,7 @@ on: env: CARGO_TERM_COLOR: always RUSTFLAGS: -Dwarnings + TZ: /usr/share/zoneinfo/America/Los_Angeles defaults: run: diff --git a/.github/workflows/compiler_typescript.yml b/.github/workflows/compiler_typescript.yml index 233e963bfad74..8576f66a10057 100644 --- a/.github/workflows/compiler_typescript.yml +++ b/.github/workflows/compiler_typescript.yml @@ -5,9 +5,12 @@ on: branches: [main] pull_request: paths: - - "compiler/**" + - compiler/** - .github/workflows/compiler-typescript.yml +env: + TZ: /usr/share/zoneinfo/America/Los_Angeles + defaults: run: working-directory: compiler @@ -31,8 +34,8 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: - node-version: 18.x - cache: "yarn" + node-version: 18.20.1 + cache: yarn cache-dependency-path: compiler/yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 @@ -50,8 +53,8 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: - node-version: 18.x - cache: "yarn" + node-version: 18.20.1 + cache: yarn cache-dependency-path: compiler/yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 @@ -74,8 +77,8 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: - node-version: 18.x - cache: "yarn" + node-version: 18.20.1 + cache: yarn cache-dependency-path: compiler/yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 diff --git a/.github/workflows/devtools_check_repro.yml b/.github/workflows/devtools_check_repro.yml index adaef6ac32bd6..735371e3a8f7d 100644 --- a/.github/workflows/devtools_check_repro.yml +++ b/.github/workflows/devtools_check_repro.yml @@ -5,6 +5,9 @@ on: issue_comment: types: [created, edited] +env: + TZ: /usr/share/zoneinfo/America/Los_Angeles + jobs: check-repro: runs-on: ubuntu-latest diff --git a/.github/workflows/runtime_commit_artifacts.yml b/.github/workflows/runtime_commit_artifacts.yml index 2c32281634c7c..c665f9b1bd40e 100644 --- a/.github/workflows/runtime_commit_artifacts.yml +++ b/.github/workflows/runtime_commit_artifacts.yml @@ -4,6 +4,9 @@ on: push: branches: [main, meta-www, meta-fbsource] +env: + TZ: /usr/share/zoneinfo/America/Los_Angeles + jobs: download_artifacts: runs-on: ubuntu-latest diff --git a/.github/workflows/runtime_fizz.yml b/.github/workflows/runtime_fizz.yml index 43097728e37de..97e4a0f7be677 100644 --- a/.github/workflows/runtime_fizz.yml +++ b/.github/workflows/runtime_fizz.yml @@ -5,7 +5,10 @@ on: branches: [main] pull_request: paths-ignore: - - 'compiler/**' + - compiler/** + +env: + TZ: /usr/share/zoneinfo/America/Los_Angeles jobs: check_generated_fizz_runtime: @@ -15,8 +18,8 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: - node-version: 18.x - cache: "yarn" + node-version: 18.20.1 + cache: yarn cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 diff --git a/.github/workflows/runtime_flags.yml b/.github/workflows/runtime_flags.yml index baf9a48242c7c..178d015bbf587 100644 --- a/.github/workflows/runtime_flags.yml +++ b/.github/workflows/runtime_flags.yml @@ -5,7 +5,10 @@ on: branches: [main] pull_request: paths-ignore: - - 'compiler/**' + - compiler/** + +env: + TZ: /usr/share/zoneinfo/America/Los_Angeles jobs: flags: @@ -15,8 +18,8 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: - node-version: 18.x - cache: "yarn" + node-version: 18.20.1 + cache: yarn cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 diff --git a/.github/workflows/runtime_flow.yml b/.github/workflows/runtime_flow.yml index 6b8eb4b774e35..bba1faaf9fd40 100644 --- a/.github/workflows/runtime_flow.yml +++ b/.github/workflows/runtime_flow.yml @@ -5,7 +5,10 @@ on: branches: [main] pull_request: paths-ignore: - - 'compiler/**' + - compiler/** + +env: + TZ: /usr/share/zoneinfo/America/Los_Angeles jobs: discover_flow_inline_configs: @@ -34,8 +37,8 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: - node-version: 18.x - cache: "yarn" + node-version: 18.20.1 + cache: yarn cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 diff --git a/.github/workflows/runtime_fuzz_tests.yml b/.github/workflows/runtime_fuzz_tests.yml index 5b31d115dcd23..dac4dd095a8d4 100644 --- a/.github/workflows/runtime_fuzz_tests.yml +++ b/.github/workflows/runtime_fuzz_tests.yml @@ -9,6 +9,10 @@ on: inputs: prerelease_commit_sha: required: false + +env: + TZ: /usr/share/zoneinfo/America/Los_Angeles + jobs: test_fuzz: if: inputs.prerelease_commit_sha == '' diff --git a/.github/workflows/runtime_test.yml b/.github/workflows/runtime_test.yml index 90ead473611ce..84cfe79e5d3d7 100644 --- a/.github/workflows/runtime_test.yml +++ b/.github/workflows/runtime_test.yml @@ -5,7 +5,10 @@ on: branches: [main] pull_request: paths-ignore: - - 'compiler/**' + - compiler/** + +env: + TZ: /usr/share/zoneinfo/America/Los_Angeles jobs: test: @@ -44,8 +47,8 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: - node-version: 18.x - cache: "yarn" + node-version: 18.20.1 + cache: yarn cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 diff --git a/.github/workflows/shared_lint.yml b/.github/workflows/shared_lint.yml index ca851ff2324bf..e06637b168bf9 100644 --- a/.github/workflows/shared_lint.yml +++ b/.github/workflows/shared_lint.yml @@ -5,6 +5,9 @@ on: branches: [main] pull_request: +env: + TZ: /usr/share/zoneinfo/America/Los_Angeles + jobs: prettier: name: Run prettier @@ -13,8 +16,8 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: - node-version: 18.x - cache: "yarn" + node-version: 18.20.1 + cache: yarn cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 @@ -31,8 +34,8 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: - node-version: 18.x - cache: "yarn" + node-version: 18.20.1 + cache: yarn cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 @@ -49,8 +52,8 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: - node-version: 18.x - cache: "yarn" + node-version: 18.20.1 + cache: yarn cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 @@ -67,8 +70,8 @@ jobs: - uses: actions/checkout@v4 - uses: actions/setup-node@v4 with: - node-version: 18.x - cache: "yarn" + node-version: 18.20.1 + cache: yarn cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 diff --git a/.github/workflows/shared_stale.yml b/.github/workflows/shared_stale.yml index df9854c270a28..9135b9afcaaeb 100644 --- a/.github/workflows/shared_stale.yml +++ b/.github/workflows/shared_stale.yml @@ -5,6 +5,9 @@ on: # Run hourly - cron: '0 * * * *' +env: + TZ: /usr/share/zoneinfo/America/Los_Angeles + jobs: stale: runs-on: ubuntu-latest diff --git a/.nvmrc b/.nvmrc index 91f7588a1a5df..ef33d6510196d 100644 --- a/.nvmrc +++ b/.nvmrc @@ -1 +1 @@ -v18.20.0 +v18.20.1 From 1e8efe7fd401384226865fb4e220038f3505a748 Mon Sep 17 00:00:00 2001 From: Lauren Tan Date: Fri, 12 Jul 2024 12:26:31 -0400 Subject: [PATCH 08/14] [ci] Combine Runtime workflows Merges most workflows into a single workflow, which should make the "Checks" tab easier to visualize sincee it will all be graphed in a single interface. There should be no change in behavior, this is a mechanical change. ghstack-source-id: a5241a24205a375555fd7f7d26339fb887070242 Pull Request resolved: https://github.com/facebook/react/pull/30324 --- ...e_build.yml => runtime_build_and_test.yml} | 133 +++++++++++++++++- .github/workflows/runtime_fizz.yml | 33 ----- .github/workflows/runtime_flags.yml | 31 ---- .github/workflows/runtime_flow.yml | 50 ------- .github/workflows/runtime_test.yml | 60 -------- 5 files changed, 132 insertions(+), 175 deletions(-) rename .github/workflows/{runtime_build.yml => runtime_build_and_test.yml} (57%) delete mode 100644 .github/workflows/runtime_fizz.yml delete mode 100644 .github/workflows/runtime_flags.yml delete mode 100644 .github/workflows/runtime_flow.yml delete mode 100644 .github/workflows/runtime_test.yml diff --git a/.github/workflows/runtime_build.yml b/.github/workflows/runtime_build_and_test.yml similarity index 57% rename from .github/workflows/runtime_build.yml rename to .github/workflows/runtime_build_and_test.yml index 304f03482ddbd..a90a983cbce04 100644 --- a/.github/workflows/runtime_build.yml +++ b/.github/workflows/runtime_build_and_test.yml @@ -1,4 +1,4 @@ -name: (Runtime) Build +name: (Runtime) Build and Test on: push: @@ -11,6 +11,137 @@ env: TZ: /usr/share/zoneinfo/America/Los_Angeles jobs: + # ----- FLOW ----- + discover_flow_inline_configs: + name: Discover flow inline configs + runs-on: ubuntu-latest + outputs: + matrix: ${{ steps.set-matrix.outputs.result }} + steps: + - uses: actions/checkout@v4 + - uses: actions/github-script@v7 + id: set-matrix + with: + script: | + const inlinedHostConfigs = require('./scripts/shared/inlinedHostConfigs.js'); + return inlinedHostConfigs.map(config => config.shortName); + + flow: + name: Flow check ${{ matrix.flow_inline_config_shortname }} + needs: discover_flow_inline_configs + runs-on: ubuntu-latest + continue-on-error: true + strategy: + matrix: + flow_inline_config_shortname: ${{ fromJSON(needs.discover_flow_inline_configs.outputs.matrix) }} + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: 18.20.1 + cache: yarn + cache-dependency-path: yarn.lock + - name: Restore cached node_modules + uses: actions/cache@v4 + id: node_modules + with: + path: "**/node_modules" + key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} + - run: yarn install --frozen-lockfile + - run: node ./scripts/tasks/flow-ci ${{ matrix.flow_inline_config_shortname }} + + # ----- FIZZ ----- + check_generated_fizz_runtime: + name: Confirm generated inline Fizz runtime is up to date + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: 18.20.1 + cache: yarn + cache-dependency-path: yarn.lock + - name: Restore cached node_modules + uses: actions/cache@v4 + id: node_modules + with: + path: "**/node_modules" + key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} + - run: yarn install --frozen-lockfile + - run: | + yarn generate-inline-fizz-runtime + git diff --quiet || (echo "There was a change to the Fizz runtime. Run `yarn generate-inline-fizz-runtime` and check in the result." && false) + + # ----- FEATURE FLAGS ----- + flags: + name: Check flags + runs-on: ubuntu-latest + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: 18.20.1 + cache: yarn + cache-dependency-path: yarn.lock + - name: Restore cached node_modules + uses: actions/cache@v4 + id: node_modules + with: + path: "**/node_modules" + key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} + - run: yarn install --frozen-lockfile + - run: yarn flags + + # ----- TESTS ----- + test: + name: yarn test ${{ matrix.params }} (Shard ${{ matrix.shard }}) + runs-on: ubuntu-latest + strategy: + matrix: + params: + - "-r=stable --env=development" + - "-r=stable --env=production" + - "-r=experimental --env=development" + - "-r=experimental --env=production" + - "-r=www-classic --env=development --variant=false" + - "-r=www-classic --env=production --variant=false" + - "-r=www-classic --env=development --variant=true" + - "-r=www-classic --env=production --variant=true" + - "-r=www-modern --env=development --variant=false" + - "-r=www-modern --env=production --variant=false" + - "-r=www-modern --env=development --variant=true" + - "-r=www-modern --env=production --variant=true" + - "-r=xplat --env=development --variant=false" + - "-r=xplat --env=development --variant=true" + - "-r=xplat --env=production --variant=false" + - "-r=xplat --env=production --variant=true" + # TODO: Test more persistent configurations? + - "-r=stable --env=development --persistent" + - "-r=experimental --env=development --persistent" + shard: + - 1/5 + - 2/5 + - 3/5 + - 4/5 + - 5/5 + continue-on-error: true + steps: + - uses: actions/checkout@v4 + - uses: actions/setup-node@v4 + with: + node-version: 18.20.1 + cache: yarn + cache-dependency-path: yarn.lock + - name: Restore cached node_modules + uses: actions/cache@v4 + id: node_modules + with: + path: "**/node_modules" + key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} + - run: yarn install --frozen-lockfile + - run: yarn test ${{ matrix.params }} --ci=github --shard=${{ matrix.shard }} + + # ----- BUILD ----- build_and_lint: name: yarn build and lint runs-on: ubuntu-latest diff --git a/.github/workflows/runtime_fizz.yml b/.github/workflows/runtime_fizz.yml deleted file mode 100644 index 97e4a0f7be677..0000000000000 --- a/.github/workflows/runtime_fizz.yml +++ /dev/null @@ -1,33 +0,0 @@ -name: (Runtime) Fizz - -on: - push: - branches: [main] - pull_request: - paths-ignore: - - compiler/** - -env: - TZ: /usr/share/zoneinfo/America/Los_Angeles - -jobs: - check_generated_fizz_runtime: - name: Confirm generated inline Fizz runtime is up to date - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - uses: actions/setup-node@v4 - with: - node-version: 18.20.1 - cache: yarn - cache-dependency-path: yarn.lock - - name: Restore cached node_modules - uses: actions/cache@v4 - id: node_modules - with: - path: "**/node_modules" - key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} - - run: yarn install --frozen-lockfile - - run: | - yarn generate-inline-fizz-runtime - git diff --quiet || (echo "There was a change to the Fizz runtime. Run `yarn generate-inline-fizz-runtime` and check in the result." && false) diff --git a/.github/workflows/runtime_flags.yml b/.github/workflows/runtime_flags.yml deleted file mode 100644 index 178d015bbf587..0000000000000 --- a/.github/workflows/runtime_flags.yml +++ /dev/null @@ -1,31 +0,0 @@ -name: (Runtime) Flags - -on: - push: - branches: [main] - pull_request: - paths-ignore: - - compiler/** - -env: - TZ: /usr/share/zoneinfo/America/Los_Angeles - -jobs: - flags: - name: Check flags - runs-on: ubuntu-latest - steps: - - uses: actions/checkout@v4 - - uses: actions/setup-node@v4 - with: - node-version: 18.20.1 - cache: yarn - cache-dependency-path: yarn.lock - - name: Restore cached node_modules - uses: actions/cache@v4 - id: node_modules - with: - path: "**/node_modules" - key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} - - run: yarn install --frozen-lockfile - - run: yarn flags diff --git a/.github/workflows/runtime_flow.yml b/.github/workflows/runtime_flow.yml deleted file mode 100644 index bba1faaf9fd40..0000000000000 --- a/.github/workflows/runtime_flow.yml +++ /dev/null @@ -1,50 +0,0 @@ -name: (Runtime) Flow - -on: - push: - branches: [main] - pull_request: - paths-ignore: - - compiler/** - -env: - TZ: /usr/share/zoneinfo/America/Los_Angeles - -jobs: - discover_flow_inline_configs: - name: Discover flow inline configs - runs-on: ubuntu-latest - outputs: - matrix: ${{ steps.set-matrix.outputs.result }} - steps: - - uses: actions/checkout@v4 - - uses: actions/github-script@v7 - id: set-matrix - with: - script: | - const inlinedHostConfigs = require('./scripts/shared/inlinedHostConfigs.js'); - return inlinedHostConfigs.map(config => config.shortName); - - flow: - name: Flow check ${{ matrix.flow_inline_config_shortname }} - needs: discover_flow_inline_configs - runs-on: ubuntu-latest - continue-on-error: true - strategy: - matrix: - flow_inline_config_shortname: ${{ fromJSON(needs.discover_flow_inline_configs.outputs.matrix) }} - steps: - - uses: actions/checkout@v4 - - uses: actions/setup-node@v4 - with: - node-version: 18.20.1 - cache: yarn - cache-dependency-path: yarn.lock - - name: Restore cached node_modules - uses: actions/cache@v4 - id: node_modules - with: - path: "**/node_modules" - key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} - - run: yarn install --frozen-lockfile - - run: node ./scripts/tasks/flow-ci ${{ matrix.flow_inline_config_shortname }} diff --git a/.github/workflows/runtime_test.yml b/.github/workflows/runtime_test.yml deleted file mode 100644 index 84cfe79e5d3d7..0000000000000 --- a/.github/workflows/runtime_test.yml +++ /dev/null @@ -1,60 +0,0 @@ -name: (Runtime) Test - -on: - push: - branches: [main] - pull_request: - paths-ignore: - - compiler/** - -env: - TZ: /usr/share/zoneinfo/America/Los_Angeles - -jobs: - test: - name: yarn test ${{ matrix.params }} (Shard ${{ matrix.shard }}) - runs-on: ubuntu-latest - strategy: - matrix: - params: - - "-r=stable --env=development" - - "-r=stable --env=production" - - "-r=experimental --env=development" - - "-r=experimental --env=production" - - "-r=www-classic --env=development --variant=false" - - "-r=www-classic --env=production --variant=false" - - "-r=www-classic --env=development --variant=true" - - "-r=www-classic --env=production --variant=true" - - "-r=www-modern --env=development --variant=false" - - "-r=www-modern --env=production --variant=false" - - "-r=www-modern --env=development --variant=true" - - "-r=www-modern --env=production --variant=true" - - "-r=xplat --env=development --variant=false" - - "-r=xplat --env=development --variant=true" - - "-r=xplat --env=production --variant=false" - - "-r=xplat --env=production --variant=true" - # TODO: Test more persistent configurations? - - "-r=stable --env=development --persistent" - - "-r=experimental --env=development --persistent" - shard: - - 1/5 - - 2/5 - - 3/5 - - 4/5 - - 5/5 - continue-on-error: true - steps: - - uses: actions/checkout@v4 - - uses: actions/setup-node@v4 - with: - node-version: 18.20.1 - cache: yarn - cache-dependency-path: yarn.lock - - name: Restore cached node_modules - uses: actions/cache@v4 - id: node_modules - with: - path: "**/node_modules" - key: ${{ runner.arch }}-${{ runner.os }}-modules-${{ hashFiles('yarn.lock') }} - - run: yarn install --frozen-lockfile - - run: yarn test ${{ matrix.params }} --ci=github --shard=${{ matrix.shard }} From 400e82227747f6b60fecbc3b43f7515b4fd89d8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 12 Jul 2024 13:02:22 -0400 Subject: [PATCH 09/14] Remove Component Stack from React Logged Warnings and Error Reporting (#30308) React transpiles some of its own `console.error` calls into a helper that appends component stacks to those calls. However, this doesn't cover user space `console.error` calls - which includes React helpers that React has moved into third parties like createClass and prop-types. The idea is that any user space component can add a warning just like React can which is why React DevTools adds them too if they don't already exist. Having them appended in both places is tricky because now you have to know whether to remove them from React's logs. Similarly it's often common for server-side frameworks to forget to cover the `console.error` logs from other sources since React DevTools isn't active there. However, it's also annoying to get component stacks clogging the terminal - depending on where the log came from. In the future `console.createTask()` will cover this use case natively and when available we don't append them at all. The new strategy relies on either: - React DevTools existing to add them to React logs as well as third parties. - `console.createTask` being supported and surfaced. - A third party framework showing the component stack either in an Error Dialog or appended to terminal output. For a third party to be able to implement this they need to be able to get the component stack. To get the component stack from within a `console.error` call you need to use the `React.captureOwnerStack()` helper which is only available in `enableOwnerStacks` flag. However, it's possible to polyfill with parent stacks using internals as a stop gap. There's a question of whether React 19 should just go out with `enableOwnerStacks` to expose this but regardless I think it's best it doesn't include component stacks from the runtime for consistency. In practice it's not really a regression though because typically either of the other options exists and error dialogs don't implement `console.error` overrides anyway yet. SSR terminals might miss them but they'd only have them in DEV warnings to begin with an a subset of React warnings. Typically those are either going to happen on the client anyway or replayed. Our tests are written to assert that component stacks work in various scenarios all over the place. To ensure that this keeps working I implement a "polyfill" that is similar to that expected a server framework might do - in `assertConsoleErrorDev` and `toErrorDev`. This PR doesn't yet change www or RN since they have their own forks of consoleWithStackDev for now. --- .../__tests__/ReactInternalTestUtils-test.js | 442 +++++++++++++++--- packages/internal-test-utils/consoleMock.js | 45 +- .../src/__tests__/ReactFlight-test.js | 26 +- .../ReactDOMConsoleErrorReporting-test.js | 18 +- ...eactDOMConsoleErrorReportingLegacy-test.js | 18 +- .../src/__tests__/ReactUpdates-test.js | 17 +- .../ReactIncrementalErrorLogging-test.js | 74 +-- .../react-server/src/ReactFlightServer.js | 22 +- .../createReactClassIntegration-test.js | 2 +- packages/shared/consoleWithStackDev.js | 31 +- .../shared/forks/consoleWithStackDev.rn.js | 4 - scripts/jest/matchers/toWarnDev.js | 28 ++ 12 files changed, 544 insertions(+), 183 deletions(-) diff --git a/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js b/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js index ad3a7cdce7cae..befbdc617d361 100644 --- a/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js +++ b/packages/internal-test-utils/__tests__/ReactInternalTestUtils-test.js @@ -866,16 +866,40 @@ describe('ReactInternalTestUtils console assertions', () => { const message = expectToThrowFailure(() => { expect(root).toMatchRenderedOutput(
foobarbaz
); }); - expect(message).toMatchInlineSnapshot(` - "asserConsoleLogsCleared(expected) + if (!__DEV__) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) - console.log was called without assertConsoleLogDev: - + Not asserted - + Not asserted - + Not asserted + console.log was called without assertConsoleLogDev: + + Not asserted + + Not asserted + + Not asserted - You must call one of the assertConsoleDev helpers between each act call." - `); + You must call one of the assertConsoleDev helpers between each act call." + `); + } else if (gate(flags => flags.enableOwnerStacks)) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.log was called without assertConsoleLogDev: + + Not asserted + + Not asserted + + Not asserted + + You must call one of the assertConsoleDev helpers between each act call." + `); + } else { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.log was called without assertConsoleLogDev: + + Not asserted + + Not asserted + + Not asserted + + You must call one of the assertConsoleDev helpers between each act call." + `); + } expect(root).toMatchRenderedOutput(
foobarbaz
); }); @@ -922,16 +946,52 @@ describe('ReactInternalTestUtils console assertions', () => { }); }); - expect(message).toMatchInlineSnapshot(` - "asserConsoleLogsCleared(expected) + if (!__DEV__) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) - console.warn was called without assertConsoleWarnDev: - + A - + B - + C + console.warn was called without assertConsoleWarnDev: + + A + + B + + C - You must call one of the assertConsoleDev helpers between each act call." - `); + You must call one of the assertConsoleDev helpers between each act call." + `); + } else if (gate(flags => flags.enableOwnerStacks)) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.warn was called without assertConsoleWarnDev: + + A%s, + + in App (at **) + + B%s, + + in App (at **) + + C%s, + + in App (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } else { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.warn was called without assertConsoleWarnDev: + + A%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + B%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + C%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } }); it('fails if act is called without any assertConsoleDev helpers', async () => { @@ -962,26 +1022,94 @@ describe('ReactInternalTestUtils console assertions', () => { }); }); - expect(message).toMatchInlineSnapshot(` - "asserConsoleLogsCleared(expected) + if (!__DEV__) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) - console.log was called without assertConsoleLogDev: - + A - + B - + C + console.log was called without assertConsoleLogDev: + + A + + B + + C - console.warn was called without assertConsoleWarnDev: - + A - + B - + C + console.warn was called without assertConsoleWarnDev: + + A + + B + + C - console.error was called without assertConsoleErrorDev: - + A - + B - + C + console.error was called without assertConsoleErrorDev: + + A + + B + + C - You must call one of the assertConsoleDev helpers between each act call." - `); + You must call one of the assertConsoleDev helpers between each act call." + `); + } else if (gate(flags => flags.enableOwnerStacks)) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.log was called without assertConsoleLogDev: + + A + + B + + C + + console.warn was called without assertConsoleWarnDev: + + A%s, + + in App (at **) + + B%s, + + in App (at **) + + C%s, + + in App (at **) + + console.error was called without assertConsoleErrorDev: + + A%s, + + in App (at **) + + B%s, + + in App (at **) + + C%s, + + in App (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } else { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.log was called without assertConsoleLogDev: + + A + + B + + C + + console.warn was called without assertConsoleWarnDev: + + A%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + B%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + C%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + console.error was called without assertConsoleErrorDev: + + A%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + B%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + C%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } }); // @gate __DEV__ @@ -1804,16 +1932,49 @@ describe('ReactInternalTestUtils console assertions', () => { const message = expectToThrowFailure(() => { expect(root).toMatchRenderedOutput(
foobarbaz
); }); - expect(message).toMatchInlineSnapshot(` - "asserConsoleLogsCleared(expected) + if (!__DEV__) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) - console.warn was called without assertConsoleWarnDev: - + Not asserted - + Not asserted - + Not asserted + console.warn was called without assertConsoleWarnDev: + + Not asserted + + Not asserted + + Not asserted - You must call one of the assertConsoleDev helpers between each act call." - `); + You must call one of the assertConsoleDev helpers between each act call." + `); + } else if (gate(flags => flags.enableOwnerStacks)) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.warn was called without assertConsoleWarnDev: + + Not asserted%s, + + in Yield (at **) + + Not asserted%s, + + in Yield (at **) + + Not asserted%s, + + in Yield (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } else { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.warn was called without assertConsoleWarnDev: + + Not asserted%s, + + in Yield (at **) + + in div (at **) + + Not asserted%s, + + in Yield (at **) + + in div (at **) + + Not asserted%s, + + in Yield (at **) + + in div (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } expect(root).toMatchRenderedOutput(
foobarbaz
); }); @@ -1860,16 +2021,52 @@ describe('ReactInternalTestUtils console assertions', () => { }); }); - expect(message).toMatchInlineSnapshot(` - "asserConsoleLogsCleared(expected) + if (!__DEV__) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) - console.error was called without assertConsoleErrorDev: - + A - + B - + C + console.error was called without assertConsoleErrorDev: + + A + + B + + C - You must call one of the assertConsoleDev helpers between each act call." - `); + You must call one of the assertConsoleDev helpers between each act call." + `); + } else if (gate(flags => flags.enableOwnerStacks)) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.error was called without assertConsoleErrorDev: + + A%s, + + in App (at **) + + B%s, + + in App (at **) + + C%s, + + in App (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } else { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.error was called without assertConsoleErrorDev: + + A%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + B%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + C%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } }); it('fails if act is called without any assertConsoleDev helpers', async () => { @@ -1900,26 +2097,94 @@ describe('ReactInternalTestUtils console assertions', () => { }); }); - expect(message).toMatchInlineSnapshot(` - "asserConsoleLogsCleared(expected) + if (!__DEV__) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) - console.log was called without assertConsoleLogDev: - + A - + B - + C + console.log was called without assertConsoleLogDev: + + A + + B + + C - console.warn was called without assertConsoleWarnDev: - + A - + B - + C + console.warn was called without assertConsoleWarnDev: + + A + + B + + C - console.error was called without assertConsoleErrorDev: - + A - + B - + C + console.error was called without assertConsoleErrorDev: + + A + + B + + C - You must call one of the assertConsoleDev helpers between each act call." - `); + You must call one of the assertConsoleDev helpers between each act call." + `); + } else if (gate(flags => flags.enableOwnerStacks)) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.log was called without assertConsoleLogDev: + + A + + B + + C + + console.warn was called without assertConsoleWarnDev: + + A%s, + + in App (at **) + + B%s, + + in App (at **) + + C%s, + + in App (at **) + + console.error was called without assertConsoleErrorDev: + + A%s, + + in App (at **) + + B%s, + + in App (at **) + + C%s, + + in App (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } else { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.log was called without assertConsoleLogDev: + + A + + B + + C + + console.warn was called without assertConsoleWarnDev: + + A%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + B%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + C%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + console.error was called without assertConsoleErrorDev: + + A%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + B%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + C%s, + + in Yield (at **) + + in div (at **) + + in App (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } }); // @gate __DEV__ @@ -2786,16 +3051,49 @@ describe('ReactInternalTestUtils console assertions', () => { const message = expectToThrowFailure(() => { expect(root).toMatchRenderedOutput(
foobarbaz
); }); - expect(message).toMatchInlineSnapshot(` - "asserConsoleLogsCleared(expected) + if (!__DEV__) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) - console.error was called without assertConsoleErrorDev: - + Not asserted - + Not asserted - + Not asserted + console.error was called without assertConsoleErrorDev: + + Not asserted + + Not asserted + + Not asserted - You must call one of the assertConsoleDev helpers between each act call." - `); + You must call one of the assertConsoleDev helpers between each act call." + `); + } else if (gate(flags => flags.enableOwnerStacks)) { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.error was called without assertConsoleErrorDev: + + Not asserted%s, + + in Yield (at **) + + Not asserted%s, + + in Yield (at **) + + Not asserted%s, + + in Yield (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } else { + expect(message).toMatchInlineSnapshot(` + "asserConsoleLogsCleared(expected) + + console.error was called without assertConsoleErrorDev: + + Not asserted%s, + + in Yield (at **) + + in div (at **) + + Not asserted%s, + + in Yield (at **) + + in div (at **) + + Not asserted%s, + + in Yield (at **) + + in div (at **) + + You must call one of the assertConsoleDev helpers between each act call." + `); + } expect(root).toMatchRenderedOutput(
foobarbaz
); }); diff --git a/packages/internal-test-utils/consoleMock.js b/packages/internal-test-utils/consoleMock.js index 3f35d9e12243a..6d03c74c1dfee 100644 --- a/packages/internal-test-utils/consoleMock.js +++ b/packages/internal-test-utils/consoleMock.js @@ -44,6 +44,34 @@ const patchConsoleMethod = ( return; } + // Append Component Stacks. Simulates a framework or DevTools appending them. + if ( + typeof format === 'string' && + (methodName === 'error' || methodName === 'warn') + ) { + const React = require('react'); + if (React.captureOwnerStack) { + // enableOwnerStacks enabled. When it's always on, we can assume this case. + const stack = React.captureOwnerStack(); + if (stack) { + format += '%s'; + args.push(stack); + } + } else { + // Otherwise we have to use internals to emulate parent stacks. + const ReactSharedInternals = + React.__CLIENT_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE || + React.__SERVER_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE; + if (ReactSharedInternals && ReactSharedInternals.getCurrentStack) { + const stack = ReactSharedInternals.getCurrentStack(); + if (stack !== '') { + format += '%s'; + args.push(stack); + } + } + } + } + // Capture the call stack now so we can warn about it later. // The call stack has helpful information for the test author. // Don't throw yet though b'c it might be accidentally caught and suppressed. @@ -204,7 +232,7 @@ export function assertConsoleLogsCleared() { if (warnings.length > 0) { message += `\nconsole.warn was called without assertConsoleWarnDev:\n${diff( '', - warnings.join('\n'), + warnings.map(normalizeComponentStack).join('\n'), { omitAnnotationLines: true, }, @@ -213,7 +241,7 @@ export function assertConsoleLogsCleared() { if (errors.length > 0) { message += `\nconsole.error was called without assertConsoleErrorDev:\n${diff( '', - errors.join('\n'), + errors.map(normalizeComponentStack).join('\n'), { omitAnnotationLines: true, }, @@ -249,6 +277,19 @@ function normalizeCodeLocInfo(str) { }); } +function normalizeComponentStack(entry) { + if ( + typeof entry[0] === 'string' && + entry[0].endsWith('%s') && + isLikelyAComponentStack(entry[entry.length - 1]) + ) { + const clone = entry.slice(0); + clone[clone.length - 1] = normalizeCodeLocInfo(entry[entry.length - 1]); + return clone; + } + return entry; +} + const isLikelyAComponentStack = message => typeof message === 'string' && (message.indexOf('') > -1 || diff --git a/packages/react-client/src/__tests__/ReactFlight-test.js b/packages/react-client/src/__tests__/ReactFlight-test.js index af56b8c1a276d..e48c926a9111a 100644 --- a/packages/react-client/src/__tests__/ReactFlight-test.js +++ b/packages/react-client/src/__tests__/ReactFlight-test.js @@ -1436,12 +1436,23 @@ describe('ReactFlight', () => { it('should warn in DEV a child is missing keys on server component', () => { function NoKey({children}) { - return
; + return ReactServer.createElement('div', { + key: "this has a key but parent doesn't", + }); } expect(() => { + // While we're on the server we need to have the Server version active to track component stacks. + jest.resetModules(); + jest.mock('react', () => ReactServer); const transport = ReactNoopFlightServer.render( -
{Array(6).fill()}
, + ReactServer.createElement( + 'div', + null, + Array(6).fill(ReactServer.createElement(NoKey)), + ), ); + jest.resetModules(); + jest.mock('react', () => React); ReactNoopFlightClient.read(transport); }).toErrorDev('Each child in a list should have a unique "key" prop.'); }); @@ -2814,7 +2825,7 @@ describe('ReactFlight', () => { }); // @gate (enableOwnerStacks && enableServerComponentLogs) || !__DEV__ - it('should not include component stacks in replayed logs (unless DevTools add them)', () => { + it('should include only one component stack in replayed logs (if DevTools or polyfill adds them)', () => { class MyError extends Error { toJSON() { return 123; @@ -2839,6 +2850,9 @@ describe('ReactFlight', () => { return ReactServer.createElement(Bar); } + // While we're on the server we need to have the Server version active to track component stacks. + jest.resetModules(); + jest.mock('react', () => ReactServer); const transport = ReactNoopFlightServer.render( ReactServer.createElement(App), ); @@ -2857,6 +2871,8 @@ describe('ReactFlight', () => { ]); // Replay logs on the client + jest.resetModules(); + jest.mock('react', () => React); ReactNoopFlightClient.read(transport); assertConsoleErrorDev( [ @@ -2866,8 +2882,8 @@ describe('ReactFlight', () => { '
Womp womp: {Error}
\n' + ' ^^^^^^^', ], - // We should not have a stack in the replay because that should be added either by console.createTask - // or React DevTools on the client. Neither of which we do here. + // We should have a stack in the replay but we don't yet set the owner from the Flight replaying + // so our simulated polyfill doesn't end up getting any component stacks yet. {withoutStack: true}, ); }); diff --git a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js index 9eeb4e7c072a1..2aca1e0588462 100644 --- a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReporting-test.js @@ -143,7 +143,8 @@ describe('ReactDOMConsoleErrorReporting', () => { expect.stringContaining('%s'), expect.stringContaining('An error occurred in the component'), expect.stringContaining('Consider adding an error boundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); } else { @@ -208,7 +209,8 @@ describe('ReactDOMConsoleErrorReporting', () => { 'The above error occurred in the component', ), expect.stringContaining('ErrorBoundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); } else { @@ -274,7 +276,8 @@ describe('ReactDOMConsoleErrorReporting', () => { expect.stringContaining('%s'), expect.stringContaining('An error occurred in the component'), expect.stringContaining('Consider adding an error boundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); } else { @@ -344,7 +347,8 @@ describe('ReactDOMConsoleErrorReporting', () => { 'The above error occurred in the component', ), expect.stringContaining('ErrorBoundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); } else { @@ -410,7 +414,8 @@ describe('ReactDOMConsoleErrorReporting', () => { expect.stringContaining('%s'), expect.stringContaining('An error occurred in the component'), expect.stringContaining('Consider adding an error boundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); } else { @@ -478,7 +483,8 @@ describe('ReactDOMConsoleErrorReporting', () => { 'The above error occurred in the component', ), expect.stringContaining('ErrorBoundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); } else { diff --git a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js index 65301c789d0f0..99bfeac56b1d3 100644 --- a/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMConsoleErrorReportingLegacy-test.js @@ -162,7 +162,8 @@ describe('ReactDOMConsoleErrorReporting', () => { // Addendum by React: expect.stringContaining('An error occurred in the component'), expect.stringContaining('Consider adding an error boundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); @@ -239,7 +240,8 @@ describe('ReactDOMConsoleErrorReporting', () => { 'The above error occurred in the component', ), expect.stringContaining('ErrorBoundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); } else { @@ -309,7 +311,8 @@ describe('ReactDOMConsoleErrorReporting', () => { // Addendum by React: expect.stringContaining('An error occurred in the component'), expect.stringContaining('Consider adding an error boundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); @@ -390,7 +393,8 @@ describe('ReactDOMConsoleErrorReporting', () => { 'The above error occurred in the component', ), expect.stringContaining('ErrorBoundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); } else { @@ -460,7 +464,8 @@ describe('ReactDOMConsoleErrorReporting', () => { // Addendum by React: expect.stringContaining('An error occurred in the component'), expect.stringContaining('Consider adding an error boundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); @@ -540,7 +545,8 @@ describe('ReactDOMConsoleErrorReporting', () => { 'The above error occurred in the component', ), expect.stringContaining('ErrorBoundary'), - expect.stringContaining('Foo'), + // The component stack is not added without the polyfill/devtools. + // expect.stringContaining('Foo'), ], ]); } else { diff --git a/packages/react-dom/src/__tests__/ReactUpdates-test.js b/packages/react-dom/src/__tests__/ReactUpdates-test.js index 1d71c71476744..4916a7212937d 100644 --- a/packages/react-dom/src/__tests__/ReactUpdates-test.js +++ b/packages/react-dom/src/__tests__/ReactUpdates-test.js @@ -1857,12 +1857,14 @@ describe('ReactUpdates', () => { } let error = null; - let stack = null; + let ownerStack = null; let nativeStack = null; const originalConsoleError = console.error; - console.error = (e, s) => { + console.error = e => { error = e; - stack = s; + ownerStack = gate(flags => flags.enableOwnerStacks) + ? React.captureOwnerStack() + : null; nativeStack = new Error().stack; Scheduler.log('stop'); }; @@ -1878,12 +1880,11 @@ describe('ReactUpdates', () => { expect(error).toContain('Maximum update depth exceeded'); // The currently executing effect should be on the native stack expect(nativeStack).toContain('at myEffect'); - if (!gate(flags => flags.enableOwnerStacks)) { - // The currently running component's name is not in the owner - // stack because it's just its JSX callsite. - expect(stack).toContain('at NonTerminating'); + if (gate(flags => flags.enableOwnerStacks)) { + expect(ownerStack).toContain('at App'); + } else { + expect(ownerStack).toBe(null); } - expect(stack).toContain('at App'); }); it('can have nested updates if they do not cross the limit', async () => { diff --git a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js index 8333445c69784..6aa4034636c31 100644 --- a/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js +++ b/packages/react-reconciler/src/__tests__/ReactIncrementalErrorLogging-test.js @@ -91,15 +91,16 @@ describe('ReactIncrementalErrorLogging', () => { 'Consider adding an error boundary to your tree ' + 'to customize error handling behavior.', ), - expect.stringMatching( - new RegExp( - gate(flags => flags.enableOwnerStacks) - ? '\\s+(in|at) ErrorThrowingComponent' - : '\\s+(in|at) ErrorThrowingComponent (.*)\n' + - '\\s+(in|at) span(.*)\n' + - '\\s+(in|at) div(.*)', - ), - ), + // The component stack is not added without the polyfill/devtools. + // expect.stringMatching( + // new RegExp( + // gate(flags => flags.enableOwnerStacks) + // ? '\\s+(in|at) ErrorThrowingComponent' + // : '\\s+(in|at) ErrorThrowingComponent (.*)\n' + + // '\\s+(in|at) span(.*)\n' + + // '\\s+(in|at) div(.*)', + // ), + // ), ); } }); @@ -139,15 +140,16 @@ describe('ReactIncrementalErrorLogging', () => { 'Consider adding an error boundary to your tree ' + 'to customize error handling behavior.', ), - expect.stringMatching( - new RegExp( - gate(flags => flags.enableOwnerStacks) - ? '\\s+(in|at) ErrorThrowingComponent' - : '\\s+(in|at) ErrorThrowingComponent (.*)\n' + - '\\s+(in|at) span(.*)\n' + - '\\s+(in|at) div(.*)', - ), - ), + // The component stack is not added without the polyfill/devtools. + // expect.stringMatching( + // new RegExp( + // gate(flags => flags.enableOwnerStacks) + // ? '\\s+(in|at) ErrorThrowingComponent' + // : '\\s+(in|at) ErrorThrowingComponent (.*)\n' + + // '\\s+(in|at) span(.*)\n' + + // '\\s+(in|at) div(.*)', + // ), + // ), ); } }); @@ -199,16 +201,17 @@ describe('ReactIncrementalErrorLogging', () => { 'React will try to recreate this component tree from scratch ' + 'using the error boundary you provided, ErrorBoundary.', ), - expect.stringMatching( - new RegExp( - gate(flags => flags.enableOwnerStacks) - ? '\\s+(in|at) ErrorThrowingComponent' - : '\\s+(in|at) ErrorThrowingComponent (.*)\n' + - '\\s+(in|at) span(.*)\n' + - '\\s+(in|at) ErrorBoundary(.*)\n' + - '\\s+(in|at) div(.*)', - ), - ), + // The component stack is not added without the polyfill/devtools. + // expect.stringMatching( + // new RegExp( + // gate(flags => flags.enableOwnerStacks) + // ? '\\s+(in|at) ErrorThrowingComponent' + // : '\\s+(in|at) ErrorThrowingComponent (.*)\n' + + // '\\s+(in|at) span(.*)\n' + + // '\\s+(in|at) ErrorBoundary(.*)\n' + + // '\\s+(in|at) div(.*)', + // ), + // ), ); } else { expect(logCapturedErrorCalls[0]).toEqual( @@ -282,13 +285,14 @@ describe('ReactIncrementalErrorLogging', () => { 'React will try to recreate this component tree from scratch ' + 'using the error boundary you provided, ErrorBoundary.', ), - expect.stringMatching( - gate(flag => flag.enableOwnerStacks) - ? new RegExp('\\s+(in|at) Foo') - : new RegExp( - '\\s+(in|at) Foo (.*)\n' + '\\s+(in|at) ErrorBoundary(.*)', - ), - ), + // The component stack is not added without the polyfill/devtools. + // expect.stringMatching( + // gate(flag => flag.enableOwnerStacks) + // ? new RegExp('\\s+(in|at) Foo') + // : new RegExp( + // '\\s+(in|at) Foo (.*)\n' + '\\s+(in|at) ErrorBoundary(.*)', + // ), + // ), ); } else { expect(console.error).toHaveBeenCalledWith( diff --git a/packages/react-server/src/ReactFlightServer.js b/packages/react-server/src/ReactFlightServer.js index a0d4b9e140566..1eaf5369c79c8 100644 --- a/packages/react-server/src/ReactFlightServer.js +++ b/packages/react-server/src/ReactFlightServer.js @@ -99,8 +99,6 @@ import {resolveOwner, setCurrentOwner} from './flight/ReactFlightCurrentOwner'; import {getOwnerStackByComponentInfoInDev} from './flight/ReactFlightComponentStack'; -import {isWritingAppendedStack} from 'shared/consoleWithStackDev'; - import { getIteratorFn, REACT_ELEMENT_TYPE, @@ -267,9 +265,8 @@ function patchConsole(consoleInst: typeof console, methodName: string) { 'name', ); const wrapperMethod = function (this: typeof console) { - let args = arguments; const request = resolveRequest(); - if (methodName === 'assert' && args[0]) { + if (methodName === 'assert' && arguments[0]) { // assert doesn't emit anything unless first argument is falsy so we can skip it. } else if (request !== null) { // Extract the stack. Not all console logs print the full stack but they have at @@ -281,22 +278,7 @@ function patchConsole(consoleInst: typeof console, methodName: string) { // refer to previous logs in debug info to associate them with a component. const id = request.nextChunkId++; const owner: null | ReactComponentInfo = resolveOwner(); - if ( - isWritingAppendedStack && - (methodName === 'error' || methodName === 'warn') && - args.length > 1 && - typeof args[0] === 'string' && - args[0].endsWith('%s') - ) { - // This looks like we've appended the component stack to the error from our own logs. - // We don't want those added to the replayed logs since those have the opportunity to add - // their own stacks or use console.createTask on the client as needed. - // TODO: Remove this special case once we remove consoleWithStackDev. - // $FlowFixMe[method-unbinding] - args = Array.prototype.slice.call(args, 0, args.length - 1); - args[0] = args[0].slice(0, args[0].length - 2); - } - emitConsoleChunk(request, id, methodName, owner, stack, args); + emitConsoleChunk(request, id, methodName, owner, stack, arguments); } // $FlowFixMe[prop-missing] return originalMethod.apply(this, arguments); diff --git a/packages/react/src/__tests__/createReactClassIntegration-test.js b/packages/react/src/__tests__/createReactClassIntegration-test.js index 330150506c384..811d897e859e7 100644 --- a/packages/react/src/__tests__/createReactClassIntegration-test.js +++ b/packages/react/src/__tests__/createReactClassIntegration-test.js @@ -804,7 +804,7 @@ describe('create-react-class-integration', () => { 'MyComponent: isMounted is deprecated. Instead, make sure to ' + 'clean up subscriptions and pending requests in componentWillUnmount ' + 'to prevent memory leaks.', - {withoutStack: true}, + // This now has a component stack even though it's part of a third-party library. ); // Dedupe diff --git a/packages/shared/consoleWithStackDev.js b/packages/shared/consoleWithStackDev.js index f709ca1e1fb98..b978b0b1d0804 100644 --- a/packages/shared/consoleWithStackDev.js +++ b/packages/shared/consoleWithStackDev.js @@ -5,9 +5,6 @@ * LICENSE file in the root directory of this source tree. */ -import ReactSharedInternals from 'shared/ReactSharedInternals'; -import {enableOwnerStacks} from 'shared/ReactFeatureFlags'; - export function setSuppressWarning(newSuppressWarning) { // TODO: Noop. Delete. } @@ -19,39 +16,25 @@ export function setSuppressWarning(newSuppressWarning) { // they are left as they are instead. export function warn(format, ...args) { - printWarning('warn', format, args, new Error('react-stack-top-frame')); + if (__DEV__) { + printWarning('warn', format, args); + } } export function error(format, ...args) { - printWarning('error', format, args, new Error('react-stack-top-frame')); + if (__DEV__) { + printWarning('error', format, args); + } } -// eslint-disable-next-line react-internal/no-production-logging -const supportsCreateTask = __DEV__ && enableOwnerStacks && !!console.createTask; - -export let isWritingAppendedStack = false; - -function printWarning(level, format, args, currentStack) { +function printWarning(level, format, args) { // When changing this logic, you might want to also // update consoleWithStackDev.www.js as well. if (__DEV__) { - if (!supportsCreateTask && ReactSharedInternals.getCurrentStack) { - // We only add the current stack to the console when createTask is not supported. - // Since createTask requires DevTools to be open to work, this means that stacks - // can be lost while DevTools isn't open but we can't detect this. - const stack = ReactSharedInternals.getCurrentStack(currentStack); - if (stack !== '') { - isWritingAppendedStack = true; - format += '%s'; - args = args.concat([stack]); - } - } - args.unshift(format); // We intentionally don't use spread (or .apply) directly because it // breaks IE9: https://github.com/facebook/react/issues/13610 // eslint-disable-next-line react-internal/no-production-logging Function.prototype.apply.call(console[level], console, args); - isWritingAppendedStack = false; } } diff --git a/packages/shared/forks/consoleWithStackDev.rn.js b/packages/shared/forks/consoleWithStackDev.rn.js index 44767992cdd86..6ac30b39fb384 100644 --- a/packages/shared/forks/consoleWithStackDev.rn.js +++ b/packages/shared/forks/consoleWithStackDev.rn.js @@ -36,14 +36,11 @@ export function error(format, ...args) { } } -export let isWritingAppendedStack = false; - function printWarning(level, format, args) { if (__DEV__) { if (ReactSharedInternals.getCurrentStack) { const stack = ReactSharedInternals.getCurrentStack(); if (stack !== '') { - isWritingAppendedStack = true; format += '%s'; args = args.concat([stack]); } @@ -54,6 +51,5 @@ function printWarning(level, format, args) { // breaks IE9: https://github.com/facebook/react/issues/13610 // eslint-disable-next-line react-internal/no-production-logging Function.prototype.apply.call(console[level], console, args); - isWritingAppendedStack = false; } } diff --git a/scripts/jest/matchers/toWarnDev.js b/scripts/jest/matchers/toWarnDev.js index 1114b65dfa821..b93a4daf75da5 100644 --- a/scripts/jest/matchers/toWarnDev.js +++ b/scripts/jest/matchers/toWarnDev.js @@ -80,6 +80,34 @@ const createMatcherFor = (consoleMethod, matcherName) => return; } + // Append Component Stacks. Simulates a framework or DevTools appending them. + if ( + typeof format === 'string' && + (consoleMethod === 'error' || consoleMethod === 'warn') + ) { + const React = require('react'); + if (React.captureOwnerStack) { + // enableOwnerStacks enabled. When it's always on, we can assume this case. + const stack = React.captureOwnerStack(); + if (stack) { + format += '%s'; + args.push(stack); + } + } else { + // Otherwise we have to use internals to emulate parent stacks. + const ReactSharedInternals = + React.__CLIENT_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE || + React.__SERVER_INTERNALS_DO_NOT_USE_OR_WARN_USERS_THEY_CANNOT_UPGRADE; + if (ReactSharedInternals && ReactSharedInternals.getCurrentStack) { + const stack = ReactSharedInternals.getCurrentStack(); + if (stack !== '') { + format += '%s'; + args.push(stack); + } + } + } + } + const message = util.format(format, ...args); const normalizedMessage = normalizeCodeLocInfo(message); From ff89ba734668fdac06e8de476486830bbf9e0785 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 12 Jul 2024 14:39:38 -0400 Subject: [PATCH 10/14] Disable consoleWithStackDev Transform except in RN/WWW (#30313) Stacked on #30308. This is now a noop module so we can stop applying the transform of console.error using the Babel plugin in the mainline builds. I'm keeping the transform for RN/WWW for now although it might be nice if the transform moved into those systems as it gets synced instead of keeping it upstream. In jest tests we're already not running the forks for RN/WWW so we don't need it at all there. --- .../src/ReactClientConsoleConfigBrowser.js | 12 +----- .../src/ReactClientConsoleConfigPlain.js | 12 +----- .../src/ReactClientConsoleConfigServer.js | 12 +----- .../react-client/src/ReactFlightClient.js | 2 +- .../src/ReactFiberErrorLogger.js | 4 +- packages/shared/consoleWithStackDev.js | 38 ++++--------------- scripts/jest/preprocessor.js | 12 +----- scripts/print-warnings/print-warnings.js | 1 - scripts/rollup/build-ghaction.js | 26 ++++++++----- scripts/rollup/build.js | 26 ++++++++----- 10 files changed, 50 insertions(+), 95 deletions(-) diff --git a/packages/react-client/src/ReactClientConsoleConfigBrowser.js b/packages/react-client/src/ReactClientConsoleConfigBrowser.js index a80a410d28d90..c1f525d28a8c7 100644 --- a/packages/react-client/src/ReactClientConsoleConfigBrowser.js +++ b/packages/react-client/src/ReactClientConsoleConfigBrowser.js @@ -7,8 +7,6 @@ * @flow */ -import {warn, error} from 'shared/consoleWithStackDev'; - const badgeFormat = '%c%s%c '; // Same badge styling as DevTools. const badgeStyle = @@ -65,12 +63,6 @@ export function printToConsole( ); } - if (methodName === 'error' && __DEV__) { - error.apply(console, newArgs); - } else if (methodName === 'warn' && __DEV__) { - warn.apply(console, newArgs); - } else { - // $FlowFixMe[invalid-computed-prop] - console[methodName].apply(console, newArgs); // eslint-disable-line react-internal/no-production-logging - } + // $FlowFixMe[invalid-computed-prop] + console[methodName].apply(console, newArgs); // eslint-disable-line react-internal/no-production-logging } diff --git a/packages/react-client/src/ReactClientConsoleConfigPlain.js b/packages/react-client/src/ReactClientConsoleConfigPlain.js index 45069ea7bd087..f2ec996b6af13 100644 --- a/packages/react-client/src/ReactClientConsoleConfigPlain.js +++ b/packages/react-client/src/ReactClientConsoleConfigPlain.js @@ -7,8 +7,6 @@ * @flow */ -import {warn, error} from 'shared/consoleWithStackDev'; - const badgeFormat = '[%s] '; const pad = ' '; @@ -46,12 +44,6 @@ export function printToConsole( newArgs.splice(offset, 0, badgeFormat, pad + badgeName + pad); } - if (methodName === 'error' && __DEV__) { - error.apply(console, newArgs); - } else if (methodName === 'warn' && __DEV__) { - warn.apply(console, newArgs); - } else { - // $FlowFixMe[invalid-computed-prop] - console[methodName].apply(console, newArgs); // eslint-disable-line react-internal/no-production-logging - } + // $FlowFixMe[invalid-computed-prop] + console[methodName].apply(console, newArgs); // eslint-disable-line react-internal/no-production-logging } diff --git a/packages/react-client/src/ReactClientConsoleConfigServer.js b/packages/react-client/src/ReactClientConsoleConfigServer.js index 8ae8152a08809..19da98a176ada 100644 --- a/packages/react-client/src/ReactClientConsoleConfigServer.js +++ b/packages/react-client/src/ReactClientConsoleConfigServer.js @@ -7,8 +7,6 @@ * @flow */ -import {warn, error} from 'shared/consoleWithStackDev'; - // This flips color using ANSI, then sets a color styling, then resets. const badgeFormat = '\x1b[0m\x1b[7m%c%s\x1b[0m%c '; // Same badge styling as DevTools. @@ -66,12 +64,6 @@ export function printToConsole( ); } - if (methodName === 'error' && __DEV__) { - error.apply(console, newArgs); - } else if (methodName === 'warn' && __DEV__) { - warn.apply(console, newArgs); - } else { - // $FlowFixMe[invalid-computed-prop] - console[methodName].apply(console, newArgs); // eslint-disable-line react-internal/no-production-logging - } + // $FlowFixMe[invalid-computed-prop] + console[methodName].apply(console, newArgs); // eslint-disable-line react-internal/no-production-logging } diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 8243e67c51884..03a4ab96c0abc 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -2090,7 +2090,7 @@ function resolveConsoleEntry( task.run(callStack); return; } - // TODO: Set the current owner so that consoleWithStackDev adds the component + // TODO: Set the current owner so that captureOwnerStack() adds the component // stack during the replay - if needed. } const rootTask = response._debugRootTask; diff --git a/packages/react-reconciler/src/ReactFiberErrorLogger.js b/packages/react-reconciler/src/ReactFiberErrorLogger.js index a8667b4bb5969..b969b9c54cb19 100644 --- a/packages/react-reconciler/src/ReactFiberErrorLogger.js +++ b/packages/react-reconciler/src/ReactFiberErrorLogger.js @@ -124,7 +124,7 @@ export function defaultOnCaughtError( error, componentNameMessage, recreateMessage, - // We let our consoleWithStackDev wrapper add the component stack to the end. + // We let DevTools or console.createTask add the component stack to the end. ], error.environmentName, ); @@ -134,7 +134,7 @@ export function defaultOnCaughtError( error, componentNameMessage, recreateMessage, - // We let our consoleWithStackDev wrapper add the component stack to the end. + // We let our DevTools or console.createTask add the component stack to the end. ); } } finally { diff --git a/packages/shared/consoleWithStackDev.js b/packages/shared/consoleWithStackDev.js index b978b0b1d0804..072f11859985f 100644 --- a/packages/shared/consoleWithStackDev.js +++ b/packages/shared/consoleWithStackDev.js @@ -5,36 +5,14 @@ * LICENSE file in the root directory of this source tree. */ -export function setSuppressWarning(newSuppressWarning) { - // TODO: Noop. Delete. -} - -// In DEV, calls to console.warn and console.error get replaced -// by calls to these methods by a Babel plugin. +// We expect that our Rollup, Jest, and Flow configurations +// always shim this module with the corresponding environment +// (either rn or www). // -// In PROD (or in packages without access to React internals), -// they are left as they are instead. - -export function warn(format, ...args) { - if (__DEV__) { - printWarning('warn', format, args); - } -} - -export function error(format, ...args) { - if (__DEV__) { - printWarning('error', format, args); - } -} +// We should never resolve to this file, but it exists to make +// sure that if we *do* accidentally break the configuration, +// the failure isn't silent. -function printWarning(level, format, args) { - // When changing this logic, you might want to also - // update consoleWithStackDev.www.js as well. - if (__DEV__) { - args.unshift(format); - // We intentionally don't use spread (or .apply) directly because it - // breaks IE9: https://github.com/facebook/react/issues/13610 - // eslint-disable-next-line react-internal/no-production-logging - Function.prototype.apply.call(console[level], console, args); - } +export function setSuppressWarning() { + // TODO: Delete this and error when even importing this module. } diff --git a/scripts/jest/preprocessor.js b/scripts/jest/preprocessor.js index f04cd2c3cfc84..974ff7137ab9f 100644 --- a/scripts/jest/preprocessor.js +++ b/scripts/jest/preprocessor.js @@ -16,9 +16,6 @@ const pathToBabel = path.join( '../..', 'package.json' ); -const pathToBabelPluginReplaceConsoleCalls = require.resolve( - '../babel/transform-replace-console-calls' -); const pathToTransformInfiniteLoops = require.resolve( '../babel/transform-prevent-infinite-loops' ); @@ -73,14 +70,7 @@ module.exports = { const isInDevToolsPackages = !!filePath.match( /\/packages\/react-devtools.*\// ); - const testOnlyPlugins = []; - const sourceOnlyPlugins = []; - if (process.env.NODE_ENV === 'development' && !isInDevToolsPackages) { - sourceOnlyPlugins.push(pathToBabelPluginReplaceConsoleCalls); - } - const plugins = (isTestFile ? testOnlyPlugins : sourceOnlyPlugins).concat( - babelOptions.plugins - ); + const plugins = [].concat(babelOptions.plugins); if (isTestFile && isInDevToolsPackages) { plugins.push(pathToTransformReactVersionPragma); } diff --git a/scripts/print-warnings/print-warnings.js b/scripts/print-warnings/print-warnings.js index 69b986b3a0d58..62f248d9987ea 100644 --- a/scripts/print-warnings/print-warnings.js +++ b/scripts/print-warnings/print-warnings.js @@ -68,7 +68,6 @@ function transform(file, enc, cb) { gs([ 'packages/**/*.js', '!packages/*/npm/**/*.js', - '!packages/shared/consoleWithStackDev.js', '!packages/react-devtools*/**/*.js', '!**/__tests__/**/*.js', '!**/__mocks__/**/*.js', diff --git a/scripts/rollup/build-ghaction.js b/scripts/rollup/build-ghaction.js index 08ac889aba43d..32a606772f0c0 100644 --- a/scripts/rollup/build-ghaction.js +++ b/scripts/rollup/build-ghaction.js @@ -149,16 +149,22 @@ function getBabelConfig( sourcemap: false, }; if (isDevelopment) { - options.plugins.push( - ...babelToES5Plugins, - // Turn console.error/warn() into a custom wrapper - [ - require('../babel/transform-replace-console-calls'), - { - shouldError: !canAccessReactObject, - }, - ] - ); + options.plugins.push(...babelToES5Plugins); + if ( + bundleType === FB_WWW_DEV || + bundleType === RN_OSS_DEV || + bundleType === RN_FB_DEV + ) { + options.plugins.push( + // Turn console.error/warn() into a custom wrapper + [ + require('../babel/transform-replace-console-calls'), + { + shouldError: !canAccessReactObject, + }, + ] + ); + } } if (updateBabelOptions) { options = updateBabelOptions(options); diff --git a/scripts/rollup/build.js b/scripts/rollup/build.js index bee54c66f156f..32aa543fc03d5 100644 --- a/scripts/rollup/build.js +++ b/scripts/rollup/build.js @@ -149,16 +149,22 @@ function getBabelConfig( sourcemap: false, }; if (isDevelopment) { - options.plugins.push( - ...babelToES5Plugins, - // Turn console.error/warn() into a custom wrapper - [ - require('../babel/transform-replace-console-calls'), - { - shouldError: !canAccessReactObject, - }, - ] - ); + options.plugins.push(...babelToES5Plugins); + if ( + bundleType === FB_WWW_DEV || + bundleType === RN_OSS_DEV || + bundleType === RN_FB_DEV + ) { + options.plugins.push( + // Turn console.error/warn() into a custom wrapper + [ + require('../babel/transform-replace-console-calls'), + { + shouldError: !canAccessReactObject, + }, + ] + ); + } } if (updateBabelOptions) { options = updateBabelOptions(options); From 127a0fbf1a4b2607c11854f97d8798a9f6dd99f8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 12 Jul 2024 18:03:10 -0400 Subject: [PATCH 11/14] Add build-for-flight-dev script to package.json (#30326) Similar to the build-for-devtools script. I often have to build locally for the flight fixture. This builds a subset that makes it work (in DEV). --- package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/package.json b/package.json index aafb48ecb9ffc..e310883cc8d70 100644 --- a/package.json +++ b/package.json @@ -112,6 +112,7 @@ "build-for-devtools": "cross-env RELEASE_CHANNEL=experimental yarn build react/index,react/jsx,react/compiler-runtime,react-dom/index,react-dom/client,react-dom/unstable_testing,react-dom/test-utils,react-is,react-debug-tools,scheduler,react-test-renderer,react-refresh,react-art --type=NODE", "build-for-devtools-dev": "yarn build-for-devtools --type=NODE_DEV", "build-for-devtools-prod": "yarn build-for-devtools --type=NODE_PROD", + "build-for-flight-dev": "cross-env RELEASE_CHANNEL=experimental node ./scripts/rollup/build.js react/index,react/jsx,react-dom/index,react-dom/client,react-dom/server,react-dom-server.node,react-dom-server-legacy.node,scheduler,react-server-dom-webpack/ --type=NODE_DEV && mv ./build/node_modules ./build/oss-experimental", "linc": "node ./scripts/tasks/linc.js", "lint": "node ./scripts/tasks/eslint.js", "lint-build": "node ./scripts/rollup/validate/index.js", From c5eca9b0824b6325dbc9613f9befb41077fe35a0 Mon Sep 17 00:00:00 2001 From: Jelle Voost Date: Sat, 13 Jul 2024 05:57:45 +0200 Subject: [PATCH 12/14] Fixed `useSyncExternalStoreWithSelector` to update memoizedSnapshot on change (#25968) ## Summary A proposed fix for the bug described in https://github.com/facebook/react/issues/25967 ## How did you test this change? See the issue linked above, test scenario included in the code sandbox: https://codesandbox.io/s/fervent-ives-0vm9es?file=/src/App.jsx --- .../src/useSyncExternalStoreWithSelector.js | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/use-sync-external-store/src/useSyncExternalStoreWithSelector.js b/packages/use-sync-external-store/src/useSyncExternalStoreWithSelector.js index 5b04ae892c875..a7be5c0fe8fda 100644 --- a/packages/use-sync-external-store/src/useSyncExternalStoreWithSelector.js +++ b/packages/use-sync-external-store/src/useSyncExternalStoreWithSelector.js @@ -93,6 +93,9 @@ export function useSyncExternalStoreWithSelector( // to React that the selections are conceptually equal, and we can bail // out of rendering. if (isEqual !== undefined && isEqual(prevSelection, nextSelection)) { + // The snapshot still has changed, so make sure to update to not keep + // old references alive + memoizedSnapshot = nextSnapshot; return prevSelection; } From 79e4f23808435d74eaa4a199ae0093cfc176f583 Mon Sep 17 00:00:00 2001 From: Josh Story Date: Sat, 13 Jul 2024 07:24:43 -0700 Subject: [PATCH 13/14] [Fizz] correctly track header length (#30327) The interstital characters in our link header tracking are not contributing to the remaining capacity calculation so when a lot of inditidual links are present in the link header it can allow an overflowing link header to be included. This change corrects the math so it properly prevents overflow. --- .../src/server/ReactFizzConfigDOM.js | 20 ++++--- .../src/__tests__/ReactDOMFizzServer-test.js | 53 +++++++++++++++++++ 2 files changed, 65 insertions(+), 8 deletions(-) diff --git a/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js b/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js index 54504deb6e517..8a670f84592a9 100644 --- a/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js +++ b/packages/react-dom-bindings/src/server/ReactFizzConfigDOM.js @@ -431,9 +431,13 @@ export function createRenderState( fontPreloads: '', highImagePreloads: '', remainingCapacity: - typeof maxHeadersLength === 'number' + // We seed the remainingCapacity with 2 extra bytes because when we decrement the capacity + // we always assume we are inserting an interstitial ", " however the first header does not actually + // consume these two extra bytes. + 2 + + (typeof maxHeadersLength === 'number' ? maxHeadersLength - : DEFAULT_HEADERS_CAPACITY_IN_UTF16_CODE_UNITS, + : DEFAULT_HEADERS_CAPACITY_IN_UTF16_CODE_UNITS), } : null; const renderState: RenderState = { @@ -2953,7 +2957,7 @@ function pushImg( // make this behavior different between render and prerender since in the latter case // we are less sensitive to the current requests runtime per and more sensitive to maximizing // headers. - (headers.remainingCapacity -= header.length) >= 2) + (headers.remainingCapacity -= header.length + 2) >= 0) ) { // If we postpone in the shell we will still emit this preload so we track // it to make sure we don't reset it. @@ -5393,7 +5397,7 @@ function prefetchDNS(href: string) { // make this behavior different between render and prerender since in the latter case // we are less sensitive to the current requests runtime per and more sensitive to maximizing // headers. - (headers.remainingCapacity -= header.length) >= 2) + (headers.remainingCapacity -= header.length + 2) >= 0) ) { // Store this as resettable in case we are prerendering and postpone in the Shell renderState.resets.dns[key] = EXISTS; @@ -5452,7 +5456,7 @@ function preconnect(href: string, crossOrigin: ?CrossOriginEnum) { // make this behavior different between render and prerender since in the latter case // we are less sensitive to the current requests runtime per and more sensitive to maximizing // headers. - (headers.remainingCapacity -= header.length) >= 2) + (headers.remainingCapacity -= header.length + 2) >= 0) ) { // Store this in resettableState in case we are prerending and postpone in the Shell renderState.resets.connect[bucket][key] = EXISTS; @@ -5518,7 +5522,7 @@ function preload(href: string, as: string, options?: ?PreloadImplOptions) { // make this behavior different between render and prerender since in the latter case // we are less sensitive to the current requests runtime per and more sensitive to maximizing // headers. - (headers.remainingCapacity -= header.length) >= 2) + (headers.remainingCapacity -= header.length + 2) >= 0) ) { // If we postpone in the shell we will still emit a preload as a header so we // track this to make sure we don't reset it. @@ -5633,7 +5637,7 @@ function preload(href: string, as: string, options?: ?PreloadImplOptions) { // make this behavior different between render and prerender since in the latter case // we are less sensitive to the current requests runtime per and more sensitive to maximizing // headers. - (headers.remainingCapacity -= header.length) >= 2) + (headers.remainingCapacity -= header.length + 2) >= 0) ) { // If we postpone in the shell we will still emit this preload so we // track it here to prevent it from being reset. @@ -6260,7 +6264,7 @@ export function emitEarlyPreloads( // This means that a particularly long header might close out the header queue where later // headers could still fit. We could in the future alter the behavior here based on prerender vs render // since during prerender we aren't as concerned with pure runtime performance. - if ((headers.remainingCapacity -= header.length) >= 2) { + if ((headers.remainingCapacity -= header.length + 2) >= 0) { renderState.resets.style[key] = PRELOAD_NO_CREDS; if (linkHeader) { linkHeader += ', '; diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index f895bbd55e71b..f51101b4d40b6 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -3940,6 +3940,59 @@ describe('ReactDOMFizzServer', () => { expect(getVisibleChildren(container)).toEqual(
hello
); }); + it('accounts for the length of the interstitial between links when computing the headers length', async () => { + let headers = null; + function onHeaders(x) { + headers = x; + } + + function App() { + // 20 bytes + ReactDOM.preconnect('01'); + // 42 bytes + ReactDOM.preconnect('02'); + // 64 bytes + ReactDOM.preconnect('03'); + // 86 bytes + ReactDOM.preconnect('04'); + // 108 bytes + ReactDOM.preconnect('05'); + // 130 bytes + ReactDOM.preconnect('06'); + // 152 bytes + ReactDOM.preconnect('07'); + // 174 bytes + ReactDOM.preconnect('08'); + // 196 bytes + ReactDOM.preconnect('09'); + // 218 bytes + ReactDOM.preconnect('10'); + // 240 bytes + ReactDOM.preconnect('11'); + // 262 bytes + ReactDOM.preconnect('12'); + // 284 bytes + ReactDOM.preconnect('13'); + // 306 bytes + ReactDOM.preconnect('14'); + return ( + + hello + + ); + } + + await act(() => { + renderToPipeableStream(, {onHeaders, maxHeadersLength: 305}); + }); + expect(headers.Link.length).toBe(284); + + await act(() => { + renderToPipeableStream(, {onHeaders, maxHeadersLength: 306}); + }); + expect(headers.Link.length).toBe(306); + }); + describe('error escaping', () => { it('escapes error hash, message, and component stack values in directly flushed errors (html escaping)', async () => { window.__outlet = {}; From 8b08e99efa56b848538768e25265fd3aa24dd8a1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Sat, 13 Jul 2024 16:33:48 -0400 Subject: [PATCH 14/14] [Flight] Encode the name of a function as an object property (#30325) Unfortunately, Firefox doesn't include the name of a function in stack traces if you set it as either `.name` or `.displayName` at runtime. Only if you include it declarative. We also can't include it into a named function expression because not all possible names are expressible declaratively. E.g. spaces or punctuations. However, we can express any name if it's an object property and since object properties now give their name declarative to the function defined inside of them, we can declaratively express any name this way. --- .../react-client/src/ReactFlightClient.js | 26 +++++++++++++------ 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/packages/react-client/src/ReactFlightClient.js b/packages/react-client/src/ReactFlightClient.js index 03a4ab96c0abc..c0d599297858e 100644 --- a/packages/react-client/src/ReactFlightClient.js +++ b/packages/react-client/src/ReactFlightClient.js @@ -1891,19 +1891,33 @@ function createFakeFunction( const comment = '/* This module was rendered by a Server Component. Turn on Source Maps to see the server source. */'; + if (!name) { + // An eval:ed function with no name gets the name "eval". We give it something more descriptive. + name = '(anonymous)'; + } + const encodedName = JSON.stringify(name); // We generate code where the call is at the line and column of the server executed code. // This allows us to use the original source map as the source map of this fake file to // point to the original source. let code; if (line <= 1) { - code = '_=>' + ' '.repeat(col < 4 ? 0 : col - 4) + '_()\n' + comment; + const minSize = encodedName.length + 7; + code = + '({' + + encodedName + + ':_=>' + + ' '.repeat(col < minSize ? 0 : col - minSize) + + '_()})\n' + + comment; } else { code = comment + '\n'.repeat(line - 2) + - '_=>\n' + + '({' + + encodedName + + ':_=>\n' + ' '.repeat(col < 1 ? 0 : col - 1) + - '_()'; + '_()})'; } if (filename.startsWith('/')) { @@ -1931,7 +1945,7 @@ function createFakeFunction( let fn: FakeFunction; try { // eslint-disable-next-line no-eval - fn = (0, eval)(code); + fn = (0, eval)(code)[name]; } catch (x) { // If eval fails, such as if in an environment that doesn't support it, // we fallback to creating a function here. It'll still have the right @@ -1940,10 +1954,6 @@ function createFakeFunction( return _(); }; } - // $FlowFixMe[cannot-write] - Object.defineProperty(fn, 'name', {value: name || '(anonymous)'}); - // $FlowFixMe[prop-missing] - fn.displayName = name; return fn; }