From 67338703aa52d662998733e58671dc9fe1edae47 Mon Sep 17 00:00:00 2001 From: lauren Date: Thu, 13 Mar 2025 20:31:04 -0400 Subject: [PATCH 01/12] [ci] Update yarn and node_modules cache key (#32603) Now that the compiler lint rule is merged into eslint-plugin-react-hooks, we also need to update our caches so compiler dependencies are also cached. This should fix the CI walltime regression we are now seeing. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32603). * #32604 * __->__ #32603 --- .github/workflows/runtime_build_and_test.yml | 80 ++++++++++++------- .../workflows/runtime_eslint_plugin_e2e.yml | 8 +- .github/workflows/runtime_prereleases.yml | 4 +- .../runtime_releases_from_npm_manual.yml | 4 +- .github/workflows/shared_lint.yml | 8 +- package.json | 2 +- scripts/react-compiler/build-compiler.sh | 1 - 7 files changed, 66 insertions(+), 41 deletions(-) diff --git a/.github/workflows/runtime_build_and_test.yml b/.github/workflows/runtime_build_and_test.yml index d53e352fbb..272411b302 100644 --- a/.github/workflows/runtime_build_and_test.yml +++ b/.github/workflows/runtime_build_and_test.yml @@ -50,13 +50,13 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -74,16 +74,18 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile + - run: yarn install --frozen-lockfile + working-directory: compiler - 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) @@ -100,16 +102,18 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile + - run: yarn install --frozen-lockfile + working-directory: compiler - run: yarn flags # ----- TESTS ----- @@ -153,16 +157,18 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile + - run: yarn install --frozen-lockfile + working-directory: compiler - run: yarn test ${{ matrix.params }} --ci --shard=${{ matrix.shard }} # ----- BUILD ----- @@ -183,7 +189,7 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - uses: actions/setup-java@v4 with: distribution: temurin @@ -193,10 +199,12 @@ jobs: id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile + - run: yarn install --frozen-lockfile + working-directory: compiler - run: yarn build --index=${{ matrix.worker_id }} --total=20 --r=${{ matrix.release_channel }} --ci env: CI: github @@ -261,16 +269,18 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile + - run: yarn install --frozen-lockfile + working-directory: compiler - name: Restore archived build uses: actions/download-artifact@v4 with: @@ -293,16 +303,18 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile + - run: yarn install --frozen-lockfile + working-directory: compiler - name: Restore archived build uses: actions/download-artifact@v4 with: @@ -340,16 +352,18 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile + - run: yarn install --frozen-lockfile + working-directory: compiler - name: Restore archived build uses: actions/download-artifact@v4 with: @@ -375,16 +389,18 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile + - run: yarn install --frozen-lockfile + working-directory: compiler - name: Restore archived build uses: actions/download-artifact@v4 with: @@ -407,13 +423,13 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: 'fixtures/dom/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: fixtures_dom-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: fixtures_dom-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('fixtures/dom/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -447,7 +463,7 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' # Fixture copies some built packages from the workroot after install. # That means dependencies of the built packages are not installed. # We need to install dependencies of the workroot to fulfill all dependency constraints @@ -456,10 +472,12 @@ jobs: id: node_modules with: path: "**/node_modules" - key: fixtures_flight-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: fixtures_flight-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile + - run: yarn install --frozen-lockfile + working-directory: compiler - name: Restore archived build uses: actions/download-artifact@v4 with: @@ -512,16 +530,18 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile + - run: yarn install --frozen-lockfile + working-directory: compiler - name: Restore archived build uses: actions/download-artifact@v4 with: @@ -563,16 +583,18 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile + - run: yarn install --frozen-lockfile + working-directory: compiler - name: Restore archived build uses: actions/download-artifact@v4 with: @@ -600,13 +622,13 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -626,6 +648,8 @@ jobs: - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile + - run: yarn install --frozen-lockfile + working-directory: compiler - name: Restore archived build for PR uses: actions/download-artifact@v4 with: diff --git a/.github/workflows/runtime_eslint_plugin_e2e.yml b/.github/workflows/runtime_eslint_plugin_e2e.yml index f8878548c0..07bcf16c23 100644 --- a/.github/workflows/runtime_eslint_plugin_e2e.yml +++ b/.github/workflows/runtime_eslint_plugin_e2e.yml @@ -35,16 +35,18 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: - path: "node_modules" - key: runtime-eslint_e2e-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: "**/node_modules" + key: runtime-eslint_e2e-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile + - run: yarn install --frozen-lockfile + working-directory: compiler - name: Build plugin working-directory: fixtures/eslint-v${{ matrix.eslint_major }} run: node build.mjs diff --git a/.github/workflows/runtime_prereleases.yml b/.github/workflows/runtime_prereleases.yml index e52bed6bb1..7536519ba3 100644 --- a/.github/workflows/runtime_prereleases.yml +++ b/.github/workflows/runtime_prereleases.yml @@ -34,13 +34,13 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} + key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/runtime_releases_from_npm_manual.yml b/.github/workflows/runtime_releases_from_npm_manual.yml index c0b3867e03..e0a9630277 100644 --- a/.github/workflows/runtime_releases_from_npm_manual.yml +++ b/.github/workflows/runtime_releases_from_npm_manual.yml @@ -66,13 +66,13 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} + key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/shared_lint.yml b/.github/workflows/shared_lint.yml index 4b077eff65..a492a320d4 100644 --- a/.github/workflows/shared_lint.yml +++ b/.github/workflows/shared_lint.yml @@ -24,7 +24,7 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 with: @@ -44,7 +44,7 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 with: @@ -64,7 +64,7 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 with: @@ -84,7 +84,7 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: '**/yarn.lock' - name: Restore cached node_modules uses: actions/cache@v4 with: diff --git a/package.json b/package.json index 2c7041f0fb..ca0b62e846 100644 --- a/package.json +++ b/package.json @@ -118,7 +118,7 @@ "testRegex": "/scripts/jest/dont-run-jest-directly\\.js$" }, "scripts": { - "prebuild": "yarn --cwd compiler install --frozen-lockfile && ./scripts/react-compiler/link-compiler.sh", + "prebuild": "./scripts/react-compiler/link-compiler.sh", "build": "node ./scripts/rollup/build-all-release-channels.js", "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", diff --git a/scripts/react-compiler/build-compiler.sh b/scripts/react-compiler/build-compiler.sh index 1c65fdcaa6..f732586462 100755 --- a/scripts/react-compiler/build-compiler.sh +++ b/scripts/react-compiler/build-compiler.sh @@ -11,5 +11,4 @@ if [[ "$REACT_CLASS_EQUIVALENCE_TEST" == "true" ]]; then fi echo "Building babel-plugin-react-compiler..." -yarn --cwd compiler install --frozen-lockfile yarn --cwd compiler workspace babel-plugin-react-compiler build --dts From ef1103d3e92168803000e12d9a27cb7440e5e1c8 Mon Sep 17 00:00:00 2001 From: lauren Date: Thu, 13 Mar 2025 20:59:27 -0400 Subject: [PATCH 02/12] [ci] Fix node_modules cache glob (#32604) Seems like the stringified cache path can cause some directories not to be cached, trying an alternative format --- .github/workflows/compiler_playground.yml | 5 +- .github/workflows/compiler_prereleases.yml | 5 +- .github/workflows/compiler_typescript.yml | 15 ++-- .../workflows/devtools_regression_tests.yml | 20 +++--- .github/workflows/runtime_build_and_test.yml | 70 +++++++++++-------- .../workflows/runtime_commit_artifacts.yml | 5 +- .../workflows/runtime_eslint_plugin_e2e.yml | 5 +- .github/workflows/runtime_prereleases.yml | 5 +- .../runtime_releases_from_npm_manual.yml | 5 +- .github/workflows/shared_lint.yml | 20 +++--- 10 files changed, 93 insertions(+), 62 deletions(-) diff --git a/.github/workflows/compiler_playground.yml b/.github/workflows/compiler_playground.yml index d3d2420ee2..21f18e4a95 100644 --- a/.github/workflows/compiler_playground.yml +++ b/.github/workflows/compiler_playground.yml @@ -36,8 +36,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: compiler-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} + path: | + **/node_modules + key: compiler-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} - run: yarn install --frozen-lockfile - run: npx playwright install --with-deps chromium - run: CI=true yarn test diff --git a/.github/workflows/compiler_prereleases.yml b/.github/workflows/compiler_prereleases.yml index 4f4954dd95..9b6744158b 100644 --- a/.github/workflows/compiler_prereleases.yml +++ b/.github/workflows/compiler_prereleases.yml @@ -46,8 +46,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: compiler-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} + path: | + **/node_modules + key: compiler-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} - run: yarn install --frozen-lockfile - name: Publish packages to npm run: | diff --git a/.github/workflows/compiler_typescript.yml b/.github/workflows/compiler_typescript.yml index d3b9517c8e..133b7c969b 100644 --- a/.github/workflows/compiler_typescript.yml +++ b/.github/workflows/compiler_typescript.yml @@ -46,8 +46,9 @@ jobs: - name: Restore cached node_modules uses: actions/cache@v4 with: - path: "**/node_modules" - key: compiler-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} + path: | + **/node_modules + key: compiler-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} - run: yarn install --frozen-lockfile - run: yarn workspace babel-plugin-react-compiler lint @@ -66,8 +67,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: compiler-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} + path: | + **/node_modules + key: compiler-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} - run: yarn install --frozen-lockfile - run: yarn workspace babel-plugin-react-compiler jest @@ -90,8 +92,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: compiler-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} + path: | + **/node_modules + key: compiler-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} - run: yarn install --frozen-lockfile - run: xvfb-run -a yarn workspace ${{ matrix.workspace_name }} test if: runner.os == 'Linux' && matrix.workspace_name == 'react-forgive' diff --git a/.github/workflows/devtools_regression_tests.yml b/.github/workflows/devtools_regression_tests.yml index 4babfeefb0..d76c2795a7 100644 --- a/.github/workflows/devtools_regression_tests.yml +++ b/.github/workflows/devtools_regression_tests.yml @@ -29,8 +29,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} + path: | + **/node_modules + key: runtime-release-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -63,8 +64,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -120,8 +122,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - run: yarn install --frozen-lockfile - name: Restore all archived build artifacts uses: actions/download-artifact@v4 @@ -154,8 +157,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - run: yarn install --frozen-lockfile - name: Restore all archived build artifacts uses: actions/download-artifact@v4 diff --git a/.github/workflows/runtime_build_and_test.yml b/.github/workflows/runtime_build_and_test.yml index 272411b302..2036ca74fb 100644 --- a/.github/workflows/runtime_build_and_test.yml +++ b/.github/workflows/runtime_build_and_test.yml @@ -55,8 +55,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -79,8 +80,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -107,8 +109,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -162,8 +165,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -198,8 +202,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -274,8 +279,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -308,8 +314,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -357,8 +364,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -394,8 +402,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -428,8 +437,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: fixtures_dom-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('fixtures/dom/yarn.lock') }} + path: | + **/node_modules + key: fixtures_dom-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('fixtures/dom/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -471,8 +481,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: fixtures_flight-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: fixtures_flight-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -535,8 +546,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -588,8 +600,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -627,8 +640,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/runtime_commit_artifacts.yml b/.github/workflows/runtime_commit_artifacts.yml index b136c60bb7..c195d0e4e7 100644 --- a/.github/workflows/runtime_commit_artifacts.yml +++ b/.github/workflows/runtime_commit_artifacts.yml @@ -73,8 +73,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} + path: | + **/node_modules + key: runtime-release-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/runtime_eslint_plugin_e2e.yml b/.github/workflows/runtime_eslint_plugin_e2e.yml index 07bcf16c23..2cbfa8d109 100644 --- a/.github/workflows/runtime_eslint_plugin_e2e.yml +++ b/.github/workflows/runtime_eslint_plugin_e2e.yml @@ -40,8 +40,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-eslint_e2e-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: runtime-eslint_e2e-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/runtime_prereleases.yml b/.github/workflows/runtime_prereleases.yml index 7536519ba3..124e94530b 100644 --- a/.github/workflows/runtime_prereleases.yml +++ b/.github/workflows/runtime_prereleases.yml @@ -39,8 +39,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: runtime-release-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/runtime_releases_from_npm_manual.yml b/.github/workflows/runtime_releases_from_npm_manual.yml index e0a9630277..d39e0d9364 100644 --- a/.github/workflows/runtime_releases_from_npm_manual.yml +++ b/.github/workflows/runtime_releases_from_npm_manual.yml @@ -71,8 +71,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: runtime-release-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/shared_lint.yml b/.github/workflows/shared_lint.yml index a492a320d4..1a4dae9cf2 100644 --- a/.github/workflows/shared_lint.yml +++ b/.github/workflows/shared_lint.yml @@ -28,8 +28,9 @@ jobs: - name: Restore cached node_modules uses: actions/cache@v4 with: - path: "**/node_modules" - key: shared-lint-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: shared-lint-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -48,8 +49,9 @@ jobs: - name: Restore cached node_modules uses: actions/cache@v4 with: - path: "**/node_modules" - key: shared-lint-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: shared-lint-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -68,8 +70,9 @@ jobs: - name: Restore cached node_modules uses: actions/cache@v4 with: - path: "**/node_modules" - key: shared-lint-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: shared-lint-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -88,8 +91,9 @@ jobs: - name: Restore cached node_modules uses: actions/cache@v4 with: - path: "**/node_modules" - key: shared-lint-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: shared-lint-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile From e9d80d939ee0f6b1d6146eb11917c433a7791d17 Mon Sep 17 00:00:00 2001 From: lauren Date: Thu, 13 Mar 2025 21:52:46 -0400 Subject: [PATCH 03/12] Revert "[ci] Fix node_modules cache glob (#32604)" (#32606) This reverts commit ef1103d3e92168803000e12d9a27cb7440e5e1c8. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32606). * #32609 * #32608 * #32607 * __->__ #32606 --- .github/workflows/compiler_playground.yml | 5 +- .github/workflows/compiler_prereleases.yml | 5 +- .github/workflows/compiler_typescript.yml | 15 ++-- .../workflows/devtools_regression_tests.yml | 20 +++--- .github/workflows/runtime_build_and_test.yml | 70 ++++++++----------- .../workflows/runtime_commit_artifacts.yml | 5 +- .../workflows/runtime_eslint_plugin_e2e.yml | 5 +- .github/workflows/runtime_prereleases.yml | 5 +- .../runtime_releases_from_npm_manual.yml | 5 +- .github/workflows/shared_lint.yml | 20 +++--- 10 files changed, 62 insertions(+), 93 deletions(-) diff --git a/.github/workflows/compiler_playground.yml b/.github/workflows/compiler_playground.yml index 21f18e4a95..d3d2420ee2 100644 --- a/.github/workflows/compiler_playground.yml +++ b/.github/workflows/compiler_playground.yml @@ -36,9 +36,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: compiler-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} + path: "**/node_modules" + key: compiler-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} - run: yarn install --frozen-lockfile - run: npx playwright install --with-deps chromium - run: CI=true yarn test diff --git a/.github/workflows/compiler_prereleases.yml b/.github/workflows/compiler_prereleases.yml index 9b6744158b..4f4954dd95 100644 --- a/.github/workflows/compiler_prereleases.yml +++ b/.github/workflows/compiler_prereleases.yml @@ -46,9 +46,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: compiler-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} + path: "**/node_modules" + key: compiler-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} - run: yarn install --frozen-lockfile - name: Publish packages to npm run: | diff --git a/.github/workflows/compiler_typescript.yml b/.github/workflows/compiler_typescript.yml index 133b7c969b..d3b9517c8e 100644 --- a/.github/workflows/compiler_typescript.yml +++ b/.github/workflows/compiler_typescript.yml @@ -46,9 +46,8 @@ jobs: - name: Restore cached node_modules uses: actions/cache@v4 with: - path: | - **/node_modules - key: compiler-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} + path: "**/node_modules" + key: compiler-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} - run: yarn install --frozen-lockfile - run: yarn workspace babel-plugin-react-compiler lint @@ -67,9 +66,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: compiler-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} + path: "**/node_modules" + key: compiler-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} - run: yarn install --frozen-lockfile - run: yarn workspace babel-plugin-react-compiler jest @@ -92,9 +90,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: compiler-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} + path: "**/node_modules" + key: compiler-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} - run: yarn install --frozen-lockfile - run: xvfb-run -a yarn workspace ${{ matrix.workspace_name }} test if: runner.os == 'Linux' && matrix.workspace_name == 'react-forgive' diff --git a/.github/workflows/devtools_regression_tests.yml b/.github/workflows/devtools_regression_tests.yml index d76c2795a7..4babfeefb0 100644 --- a/.github/workflows/devtools_regression_tests.yml +++ b/.github/workflows/devtools_regression_tests.yml @@ -29,9 +29,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-release-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} + path: "**/node_modules" + key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -64,9 +63,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: "**/node_modules" + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -122,9 +120,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: "**/node_modules" + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - run: yarn install --frozen-lockfile - name: Restore all archived build artifacts uses: actions/download-artifact@v4 @@ -157,9 +154,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: "**/node_modules" + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - run: yarn install --frozen-lockfile - name: Restore all archived build artifacts uses: actions/download-artifact@v4 diff --git a/.github/workflows/runtime_build_and_test.yml b/.github/workflows/runtime_build_and_test.yml index 2036ca74fb..272411b302 100644 --- a/.github/workflows/runtime_build_and_test.yml +++ b/.github/workflows/runtime_build_and_test.yml @@ -55,9 +55,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -80,9 +79,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -109,9 +107,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -165,9 +162,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -202,9 +198,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -279,9 +274,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -314,9 +308,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -364,9 +357,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -402,9 +394,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -437,9 +428,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: fixtures_dom-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('fixtures/dom/yarn.lock') }} + path: "**/node_modules" + key: fixtures_dom-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('fixtures/dom/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -481,9 +471,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: fixtures_flight-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: fixtures_flight-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -546,9 +535,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -600,9 +588,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -640,9 +627,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/runtime_commit_artifacts.yml b/.github/workflows/runtime_commit_artifacts.yml index c195d0e4e7..b136c60bb7 100644 --- a/.github/workflows/runtime_commit_artifacts.yml +++ b/.github/workflows/runtime_commit_artifacts.yml @@ -73,9 +73,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-release-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} + path: "**/node_modules" + key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/runtime_eslint_plugin_e2e.yml b/.github/workflows/runtime_eslint_plugin_e2e.yml index 2cbfa8d109..07bcf16c23 100644 --- a/.github/workflows/runtime_eslint_plugin_e2e.yml +++ b/.github/workflows/runtime_eslint_plugin_e2e.yml @@ -40,9 +40,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-eslint_e2e-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: runtime-eslint_e2e-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/runtime_prereleases.yml b/.github/workflows/runtime_prereleases.yml index 124e94530b..7536519ba3 100644 --- a/.github/workflows/runtime_prereleases.yml +++ b/.github/workflows/runtime_prereleases.yml @@ -39,9 +39,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-release-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/runtime_releases_from_npm_manual.yml b/.github/workflows/runtime_releases_from_npm_manual.yml index d39e0d9364..e0a9630277 100644 --- a/.github/workflows/runtime_releases_from_npm_manual.yml +++ b/.github/workflows/runtime_releases_from_npm_manual.yml @@ -71,9 +71,8 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: | - **/node_modules - key: runtime-release-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/shared_lint.yml b/.github/workflows/shared_lint.yml index 1a4dae9cf2..a492a320d4 100644 --- a/.github/workflows/shared_lint.yml +++ b/.github/workflows/shared_lint.yml @@ -28,9 +28,8 @@ jobs: - name: Restore cached node_modules uses: actions/cache@v4 with: - path: | - **/node_modules - key: shared-lint-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: shared-lint-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -49,9 +48,8 @@ jobs: - name: Restore cached node_modules uses: actions/cache@v4 with: - path: | - **/node_modules - key: shared-lint-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: shared-lint-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -70,9 +68,8 @@ jobs: - name: Restore cached node_modules uses: actions/cache@v4 with: - path: | - **/node_modules - key: shared-lint-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: shared-lint-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -91,9 +88,8 @@ jobs: - name: Restore cached node_modules uses: actions/cache@v4 with: - path: | - **/node_modules - key: shared-lint-node_modules-v2-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "**/node_modules" + key: shared-lint-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile From 5200721e5c93a59f8af0403d61abe513cb7d32fe Mon Sep 17 00:00:00 2001 From: lauren Date: Thu, 13 Mar 2025 22:14:00 -0400 Subject: [PATCH 04/12] Revert "[ci] Update yarn and node_modules cache key (#32603)" (#32607) This reverts commit 67338703aa52d662998733e58671dc9fe1edae47. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32607). * #32609 * #32608 * __->__ #32607 --- .github/workflows/runtime_build_and_test.yml | 80 +++++++------------ .../workflows/runtime_eslint_plugin_e2e.yml | 8 +- .github/workflows/runtime_prereleases.yml | 4 +- .../runtime_releases_from_npm_manual.yml | 4 +- .github/workflows/shared_lint.yml | 8 +- package.json | 2 +- scripts/react-compiler/build-compiler.sh | 1 + 7 files changed, 41 insertions(+), 66 deletions(-) diff --git a/.github/workflows/runtime_build_and_test.yml b/.github/workflows/runtime_build_and_test.yml index 272411b302..d53e352fbb 100644 --- a/.github/workflows/runtime_build_and_test.yml +++ b/.github/workflows/runtime_build_and_test.yml @@ -50,13 +50,13 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -74,18 +74,16 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile - - run: yarn install --frozen-lockfile - working-directory: compiler - 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) @@ -102,18 +100,16 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile - - run: yarn install --frozen-lockfile - working-directory: compiler - run: yarn flags # ----- TESTS ----- @@ -157,18 +153,16 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile - - run: yarn install --frozen-lockfile - working-directory: compiler - run: yarn test ${{ matrix.params }} --ci --shard=${{ matrix.shard }} # ----- BUILD ----- @@ -189,7 +183,7 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - uses: actions/setup-java@v4 with: distribution: temurin @@ -199,12 +193,10 @@ jobs: id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile - - run: yarn install --frozen-lockfile - working-directory: compiler - run: yarn build --index=${{ matrix.worker_id }} --total=20 --r=${{ matrix.release_channel }} --ci env: CI: github @@ -269,18 +261,16 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile - - run: yarn install --frozen-lockfile - working-directory: compiler - name: Restore archived build uses: actions/download-artifact@v4 with: @@ -303,18 +293,16 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile - - run: yarn install --frozen-lockfile - working-directory: compiler - name: Restore archived build uses: actions/download-artifact@v4 with: @@ -352,18 +340,16 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile - - run: yarn install --frozen-lockfile - working-directory: compiler - name: Restore archived build uses: actions/download-artifact@v4 with: @@ -389,18 +375,16 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile - - run: yarn install --frozen-lockfile - working-directory: compiler - name: Restore archived build uses: actions/download-artifact@v4 with: @@ -423,13 +407,13 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: 'fixtures/dom/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: fixtures_dom-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('fixtures/dom/yarn.lock') }} + key: fixtures_dom-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -463,7 +447,7 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock # Fixture copies some built packages from the workroot after install. # That means dependencies of the built packages are not installed. # We need to install dependencies of the workroot to fulfill all dependency constraints @@ -472,12 +456,10 @@ jobs: id: node_modules with: path: "**/node_modules" - key: fixtures_flight-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + key: fixtures_flight-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile - - run: yarn install --frozen-lockfile - working-directory: compiler - name: Restore archived build uses: actions/download-artifact@v4 with: @@ -530,18 +512,16 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile - - run: yarn install --frozen-lockfile - working-directory: compiler - name: Restore archived build uses: actions/download-artifact@v4 with: @@ -583,18 +563,16 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile - - run: yarn install --frozen-lockfile - working-directory: compiler - name: Restore archived build uses: actions/download-artifact@v4 with: @@ -622,13 +600,13 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -648,8 +626,6 @@ jobs: - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile - - run: yarn install --frozen-lockfile - working-directory: compiler - name: Restore archived build for PR uses: actions/download-artifact@v4 with: diff --git a/.github/workflows/runtime_eslint_plugin_e2e.yml b/.github/workflows/runtime_eslint_plugin_e2e.yml index 07bcf16c23..f8878548c0 100644 --- a/.github/workflows/runtime_eslint_plugin_e2e.yml +++ b/.github/workflows/runtime_eslint_plugin_e2e.yml @@ -35,18 +35,16 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-eslint_e2e-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: "node_modules" + key: runtime-eslint_e2e-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile - - run: yarn install --frozen-lockfile - working-directory: compiler - name: Build plugin working-directory: fixtures/eslint-v${{ matrix.eslint_major }} run: node build.mjs diff --git a/.github/workflows/runtime_prereleases.yml b/.github/workflows/runtime_prereleases.yml index 7536519ba3..e52bed6bb1 100644 --- a/.github/workflows/runtime_prereleases.yml +++ b/.github/workflows/runtime_prereleases.yml @@ -34,13 +34,13 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/runtime_releases_from_npm_manual.yml b/.github/workflows/runtime_releases_from_npm_manual.yml index e0a9630277..c0b3867e03 100644 --- a/.github/workflows/runtime_releases_from_npm_manual.yml +++ b/.github/workflows/runtime_releases_from_npm_manual.yml @@ -66,13 +66,13 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/shared_lint.yml b/.github/workflows/shared_lint.yml index a492a320d4..4b077eff65 100644 --- a/.github/workflows/shared_lint.yml +++ b/.github/workflows/shared_lint.yml @@ -24,7 +24,7 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 with: @@ -44,7 +44,7 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 with: @@ -64,7 +64,7 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 with: @@ -84,7 +84,7 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: '**/yarn.lock' + cache-dependency-path: yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 with: diff --git a/package.json b/package.json index ca0b62e846..2c7041f0fb 100644 --- a/package.json +++ b/package.json @@ -118,7 +118,7 @@ "testRegex": "/scripts/jest/dont-run-jest-directly\\.js$" }, "scripts": { - "prebuild": "./scripts/react-compiler/link-compiler.sh", + "prebuild": "yarn --cwd compiler install --frozen-lockfile && ./scripts/react-compiler/link-compiler.sh", "build": "node ./scripts/rollup/build-all-release-channels.js", "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", diff --git a/scripts/react-compiler/build-compiler.sh b/scripts/react-compiler/build-compiler.sh index f732586462..1c65fdcaa6 100755 --- a/scripts/react-compiler/build-compiler.sh +++ b/scripts/react-compiler/build-compiler.sh @@ -11,4 +11,5 @@ if [[ "$REACT_CLASS_EQUIVALENCE_TEST" == "true" ]]; then fi echo "Building babel-plugin-react-compiler..." +yarn --cwd compiler install --frozen-lockfile yarn --cwd compiler workspace babel-plugin-react-compiler build --dts From 4eba294c69422ade02f2bb4d6a0255755b406b32 Mon Sep 17 00:00:00 2001 From: lauren Date: Thu, 13 Mar 2025 22:21:35 -0400 Subject: [PATCH 05/12] [ci] Cache runtime and compiler only for test runs (#32608) We only need the compiler built for `yarn test` in the root directory. Rather than always cache both for every step, let's just do it where it's needed explicitly. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/32608). * #32609 * __->__ #32608 --- .github/workflows/runtime_build_and_test.yml | 46 ++++++++++++------- .../workflows/runtime_eslint_plugin_e2e.yml | 8 +++- package.json | 2 +- scripts/react-compiler/build-compiler.sh | 1 - 4 files changed, 36 insertions(+), 21 deletions(-) diff --git a/.github/workflows/runtime_build_and_test.yml b/.github/workflows/runtime_build_and_test.yml index d53e352fbb..44cafd8a09 100644 --- a/.github/workflows/runtime_build_and_test.yml +++ b/.github/workflows/runtime_build_and_test.yml @@ -56,7 +56,7 @@ jobs: id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -80,7 +80,7 @@ jobs: id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -106,7 +106,7 @@ jobs: id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -153,16 +153,20 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: | + yarn.lock + compiler/yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-and-compiler-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'compiler/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile + - run: yarn install --frozen-lockfile + working-directory: compiler - run: yarn test ${{ matrix.params }} --ci --shard=${{ matrix.shard }} # ----- BUILD ----- @@ -183,7 +187,9 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: | + yarn.lock + compiler/yarn.lock - uses: actions/setup-java@v4 with: distribution: temurin @@ -193,10 +199,12 @@ jobs: id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-and-compiler-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'compiler/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile + - run: yarn install --frozen-lockfile + working-directory: compiler - run: yarn build --index=${{ matrix.worker_id }} --total=20 --r=${{ matrix.release_channel }} --ci env: CI: github @@ -261,16 +269,20 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: | + yarn.lock + compiler/yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-and-compiler-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'compiler/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile + - run: yarn install --frozen-lockfile + working-directory: compiler - name: Restore archived build uses: actions/download-artifact@v4 with: @@ -299,7 +311,7 @@ jobs: id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -346,7 +358,7 @@ jobs: id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -381,7 +393,7 @@ jobs: id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -413,7 +425,7 @@ jobs: id: node_modules with: path: "**/node_modules" - key: fixtures_dom-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: fixtures_dom-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -456,7 +468,7 @@ jobs: id: node_modules with: path: "**/node_modules" - key: fixtures_flight-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: fixtures_flight-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -518,7 +530,7 @@ jobs: id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -569,7 +581,7 @@ jobs: id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -606,7 +618,7 @@ jobs: id: node_modules with: path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/runtime_eslint_plugin_e2e.yml b/.github/workflows/runtime_eslint_plugin_e2e.yml index f8878548c0..ade779e743 100644 --- a/.github/workflows/runtime_eslint_plugin_e2e.yml +++ b/.github/workflows/runtime_eslint_plugin_e2e.yml @@ -35,16 +35,20 @@ jobs: with: node-version-file: '.nvmrc' cache: yarn - cache-dependency-path: yarn.lock + cache-dependency-path: | + yarn.lock + compiler/yarn.lock - name: Restore cached node_modules uses: actions/cache@v4 id: node_modules with: path: "node_modules" - key: runtime-eslint_e2e-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + key: runtime-and-compiler-eslint_e2e-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'compiler/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile + - run: yarn install --frozen-lockfile + working-directory: compiler - name: Build plugin working-directory: fixtures/eslint-v${{ matrix.eslint_major }} run: node build.mjs diff --git a/package.json b/package.json index 2c7041f0fb..ca0b62e846 100644 --- a/package.json +++ b/package.json @@ -118,7 +118,7 @@ "testRegex": "/scripts/jest/dont-run-jest-directly\\.js$" }, "scripts": { - "prebuild": "yarn --cwd compiler install --frozen-lockfile && ./scripts/react-compiler/link-compiler.sh", + "prebuild": "./scripts/react-compiler/link-compiler.sh", "build": "node ./scripts/rollup/build-all-release-channels.js", "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", diff --git a/scripts/react-compiler/build-compiler.sh b/scripts/react-compiler/build-compiler.sh index 1c65fdcaa6..f732586462 100755 --- a/scripts/react-compiler/build-compiler.sh +++ b/scripts/react-compiler/build-compiler.sh @@ -11,5 +11,4 @@ if [[ "$REACT_CLASS_EQUIVALENCE_TEST" == "true" ]]; then fi echo "Building babel-plugin-react-compiler..." -yarn --cwd compiler install --frozen-lockfile yarn --cwd compiler workspace babel-plugin-react-compiler build --dts From f3c956006a90dc68210bd3e19497d10fb9b028d3 Mon Sep 17 00:00:00 2001 From: lauren Date: Thu, 13 Mar 2025 22:29:48 -0400 Subject: [PATCH 06/12] [ci] Update node_modules cache path (#32609) Alternative of #32604. Bust all old caches since I reverted some other changes to the keys. --- .github/workflows/compiler_playground.yml | 5 +- .github/workflows/compiler_prereleases.yml | 5 +- .github/workflows/compiler_typescript.yml | 15 ++-- .../workflows/devtools_regression_tests.yml | 20 +++--- .github/workflows/runtime_build_and_test.yml | 70 +++++++++++-------- .../workflows/runtime_commit_artifacts.yml | 5 +- .../workflows/runtime_eslint_plugin_e2e.yml | 5 +- .github/workflows/runtime_prereleases.yml | 5 +- .../runtime_releases_from_npm_manual.yml | 5 +- .github/workflows/shared_lint.yml | 20 +++--- 10 files changed, 93 insertions(+), 62 deletions(-) diff --git a/.github/workflows/compiler_playground.yml b/.github/workflows/compiler_playground.yml index d3d2420ee2..e53158bf60 100644 --- a/.github/workflows/compiler_playground.yml +++ b/.github/workflows/compiler_playground.yml @@ -36,8 +36,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: compiler-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} + path: | + **/node_modules + key: compiler-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} - run: yarn install --frozen-lockfile - run: npx playwright install --with-deps chromium - run: CI=true yarn test diff --git a/.github/workflows/compiler_prereleases.yml b/.github/workflows/compiler_prereleases.yml index 4f4954dd95..8baede7ac2 100644 --- a/.github/workflows/compiler_prereleases.yml +++ b/.github/workflows/compiler_prereleases.yml @@ -46,8 +46,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: compiler-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} + path: | + **/node_modules + key: compiler-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} - run: yarn install --frozen-lockfile - name: Publish packages to npm run: | diff --git a/.github/workflows/compiler_typescript.yml b/.github/workflows/compiler_typescript.yml index d3b9517c8e..dbde1b9962 100644 --- a/.github/workflows/compiler_typescript.yml +++ b/.github/workflows/compiler_typescript.yml @@ -46,8 +46,9 @@ jobs: - name: Restore cached node_modules uses: actions/cache@v4 with: - path: "**/node_modules" - key: compiler-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} + path: | + **/node_modules + key: compiler-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} - run: yarn install --frozen-lockfile - run: yarn workspace babel-plugin-react-compiler lint @@ -66,8 +67,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: compiler-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} + path: | + **/node_modules + key: compiler-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} - run: yarn install --frozen-lockfile - run: yarn workspace babel-plugin-react-compiler jest @@ -90,8 +92,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: compiler-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} + path: | + **/node_modules + key: compiler-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('compiler/**/yarn.lock') }} - run: yarn install --frozen-lockfile - run: xvfb-run -a yarn workspace ${{ matrix.workspace_name }} test if: runner.os == 'Linux' && matrix.workspace_name == 'react-forgive' diff --git a/.github/workflows/devtools_regression_tests.yml b/.github/workflows/devtools_regression_tests.yml index 4babfeefb0..9399b80f0e 100644 --- a/.github/workflows/devtools_regression_tests.yml +++ b/.github/workflows/devtools_regression_tests.yml @@ -29,8 +29,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} + path: | + **/node_modules + key: runtime-release-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -63,8 +64,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -120,8 +122,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - run: yarn install --frozen-lockfile - name: Restore all archived build artifacts uses: actions/download-artifact@v4 @@ -154,8 +157,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - run: yarn install --frozen-lockfile - name: Restore all archived build artifacts uses: actions/download-artifact@v4 diff --git a/.github/workflows/runtime_build_and_test.yml b/.github/workflows/runtime_build_and_test.yml index 44cafd8a09..1cb04920e8 100644 --- a/.github/workflows/runtime_build_and_test.yml +++ b/.github/workflows/runtime_build_and_test.yml @@ -55,8 +55,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -79,8 +80,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -105,8 +107,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -160,8 +163,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-and-compiler-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'compiler/yarn.lock') }} + path: | + **/node_modules + key: runtime-and-compiler-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'compiler/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -198,8 +202,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-and-compiler-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'compiler/yarn.lock') }} + path: | + **/node_modules + key: runtime-and-compiler-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'compiler/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -276,8 +281,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-and-compiler-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'compiler/yarn.lock') }} + path: | + **/node_modules + key: runtime-and-compiler-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'compiler/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -310,8 +316,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -357,8 +364,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -392,8 +400,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -424,8 +433,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: fixtures_dom-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: | + **/node_modules + key: fixtures_dom-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -467,8 +477,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: fixtures_flight-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: | + **/node_modules + key: fixtures_flight-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -529,8 +540,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -580,8 +592,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -617,8 +630,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} + path: | + **/node_modules + key: runtime-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/runtime_commit_artifacts.yml b/.github/workflows/runtime_commit_artifacts.yml index b136c60bb7..9e60606210 100644 --- a/.github/workflows/runtime_commit_artifacts.yml +++ b/.github/workflows/runtime_commit_artifacts.yml @@ -73,8 +73,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} + path: | + **/node_modules + key: runtime-release-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/runtime_eslint_plugin_e2e.yml b/.github/workflows/runtime_eslint_plugin_e2e.yml index ade779e743..8b8222721c 100644 --- a/.github/workflows/runtime_eslint_plugin_e2e.yml +++ b/.github/workflows/runtime_eslint_plugin_e2e.yml @@ -42,8 +42,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "node_modules" - key: runtime-and-compiler-eslint_e2e-node_modules-v3-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'compiler/yarn.lock') }} + path: | + **/node_modules + key: runtime-and-compiler-eslint_e2e-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'compiler/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/runtime_prereleases.yml b/.github/workflows/runtime_prereleases.yml index e52bed6bb1..293e5f2ce9 100644 --- a/.github/workflows/runtime_prereleases.yml +++ b/.github/workflows/runtime_prereleases.yml @@ -39,8 +39,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} + path: | + **/node_modules + key: runtime-release-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/runtime_releases_from_npm_manual.yml b/.github/workflows/runtime_releases_from_npm_manual.yml index c0b3867e03..c4675bfb7f 100644 --- a/.github/workflows/runtime_releases_from_npm_manual.yml +++ b/.github/workflows/runtime_releases_from_npm_manual.yml @@ -71,8 +71,9 @@ jobs: uses: actions/cache@v4 id: node_modules with: - path: "**/node_modules" - key: runtime-release-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} + path: | + **/node_modules + key: runtime-release-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('yarn.lock', 'scripts/release/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile diff --git a/.github/workflows/shared_lint.yml b/.github/workflows/shared_lint.yml index 4b077eff65..08ade6cb7f 100644 --- a/.github/workflows/shared_lint.yml +++ b/.github/workflows/shared_lint.yml @@ -28,8 +28,9 @@ jobs: - name: Restore cached node_modules uses: actions/cache@v4 with: - path: "**/node_modules" - key: shared-lint-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: shared-lint-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -48,8 +49,9 @@ jobs: - name: Restore cached node_modules uses: actions/cache@v4 with: - path: "**/node_modules" - key: shared-lint-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: shared-lint-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -68,8 +70,9 @@ jobs: - name: Restore cached node_modules uses: actions/cache@v4 with: - path: "**/node_modules" - key: shared-lint-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: shared-lint-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile @@ -88,8 +91,9 @@ jobs: - name: Restore cached node_modules uses: actions/cache@v4 with: - path: "**/node_modules" - key: shared-lint-node_modules-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} + path: | + **/node_modules + key: shared-lint-node_modules-v5-${{ runner.arch }}-${{ runner.os }}-${{ hashFiles('**/yarn.lock') }} - name: Ensure clean build directory run: rm -rf build - run: yarn install --frozen-lockfile From 5398b7115847e87c0053aa719728d8dd1a635ccd Mon Sep 17 00:00:00 2001 From: mofeiZ <34200447+mofeiZ@users.noreply.github.com> Date: Fri, 14 Mar 2025 11:44:49 -0400 Subject: [PATCH 07/12] [compiler] detect and throw on untransformed required features (#32512) Traverse program after running compiler transform to find untransformed references to compiler features (e.g. `inferEffectDeps`, `fire`). Hard error to fail the babel pipeline when the compiler fails to transform these features to give predictable runtime semantics. Untransformed calls to functions like `fire` will throw at runtime anyways, so let's fail the build to catch these earlier. Note that with this fails the build *regardless of panicThreshold* --- .../src/Babel/BabelPlugin.ts | 10 +- .../src/Entrypoint/Program.ts | 23 +- .../ValidateNoUntransformedReferences.ts | 275 ++++++++++++++++++ .../src/HIR/Environment.ts | 1 + ...e-in-non-react-fn-default-import.expect.md | 26 ++ ...callsite-in-non-react-fn-default-import.js | 6 + .../error.callsite-in-non-react-fn.expect.md | 26 ++ .../error.callsite-in-non-react-fn.js | 6 + .../error.non-inlined-effect-fn.expect.md | 41 +++ .../error.non-inlined-effect-fn.js | 21 ++ ...mport-default-property-useEffect.expect.md | 27 ++ ...todo-import-default-property-useEffect.js} | 2 +- .../bailout-retry/error.todo-syntax.expect.md | 51 ++++ .../bailout-retry/error.todo-syntax.js | 18 ++ .../bailout-retry/error.use-no-memo.expect.md | 27 ++ .../bailout-retry/error.use-no-memo.js | 7 + .../infer-effect-dependencies.expect.md | 26 +- .../infer-effect-dependencies.js | 7 - ...mport-default-property-useEffect.expect.md | 44 --- .../error.todo-infer-deps-on-retry.expect.md | 42 +++ .../error.todo-infer-deps-on-retry.js | 17 ++ .../bailout-retry/error.todo-syntax.expect.md | 20 +- .../bailout-retry/error.todo-syntax.js | 2 +- ...ror.untransformed-fire-reference.expect.md | 23 ++ .../error.untransformed-fire-reference.js | 4 + .../bailout-retry/error.use-no-memo.expect.md | 42 +++ ...do-use-no-memo.js => error.use-no-memo.js} | 7 +- ...-fire-todo-syntax-shouldnt-throw.expect.md | 95 ++++++ .../no-fire-todo-syntax-shouldnt-throw.js | 35 +++ .../bailout-retry/todo-use-no-memo.expect.md | 47 --- .../__tests__/ReactCompilerRule-test.ts | 32 ++ 31 files changed, 864 insertions(+), 146 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/Entrypoint/ValidateNoUntransformedReferences.ts create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn-default-import.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn-default-import.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.non-inlined-effect-fn.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.non-inlined-effect-fn.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-import-default-property-useEffect.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/{todo.import-default-property-useEffect.js => bailout-retry/error.todo-import-default-property-useEffect.js} (73%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-syntax.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-syntax.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.use-no-memo.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.use-no-memo.js delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/todo.import-default-property-useEffect.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.todo-infer-deps-on-retry.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.todo-infer-deps-on-retry.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.untransformed-fire-reference.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.untransformed-fire-reference.js create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.use-no-memo.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/{todo-use-no-memo.js => error.use-no-memo.js} (51%) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/no-fire-todo-syntax-shouldnt-throw.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/no-fire-todo-syntax-shouldnt-throw.js delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/todo-use-no-memo.expect.md diff --git a/compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts b/compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts index aa49bda22b..ff9817380f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts @@ -11,6 +11,7 @@ import { injectReanimatedFlag, pipelineUsesReanimatedPlugin, } from '../Entrypoint/Reanimated'; +import validateNoUntransformedReferences from '../Entrypoint/ValidateNoUntransformedReferences'; const ENABLE_REACT_COMPILER_TIMINGS = process.env['ENABLE_REACT_COMPILER_TIMINGS'] === '1'; @@ -61,12 +62,19 @@ export default function BabelPluginReactCompiler( }, }; } - compileProgram(prog, { + const result = compileProgram(prog, { opts, filename: pass.filename ?? null, comments: pass.file.ast.comments ?? [], code: pass.file.code, }); + validateNoUntransformedReferences( + prog, + pass.filename ?? null, + opts.logger, + opts.environment, + result?.retryErrors ?? [], + ); if (ENABLE_REACT_COMPILER_TIMINGS === true) { performance.mark(`${filename}:end`, { detail: 'BabelPlugin:Program:end', diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts index 0865b50f84..72fa101260 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts @@ -271,6 +271,9 @@ function isFilePartOfSources( return false; } +type CompileProgramResult = { + retryErrors: Array<{fn: BabelFn; error: CompilerError}>; +}; /** * `compileProgram` is directly invoked by the react-compiler babel plugin, so * exceptions thrown by this function will fail the babel build. @@ -285,16 +288,16 @@ function isFilePartOfSources( export function compileProgram( program: NodePath, pass: CompilerPass, -): void { +): CompileProgramResult | null { if (shouldSkipCompilation(program, pass)) { - return; + return null; } const environment = pass.opts.environment; const restrictedImportsErr = validateRestrictedImports(program, environment); if (restrictedImportsErr) { handleError(restrictedImportsErr, pass, null); - return; + return null; } const useMemoCacheIdentifier = program.scope.generateUidIdentifier('c'); @@ -365,7 +368,7 @@ export function compileProgram( filename: pass.filename ?? null, }, ); - + const retryErrors: Array<{fn: BabelFn; error: CompilerError}> = []; const processFn = ( fn: BabelFn, fnType: ReactFunctionType, @@ -429,7 +432,9 @@ export function compileProgram( handleError(compileResult.error, pass, fn.node.loc ?? null); } // If non-memoization features are enabled, retry regardless of error kind - if (!environment.enableFire) { + if ( + !(environment.enableFire || environment.inferEffectDependencies != null) + ) { return null; } try { @@ -448,6 +453,9 @@ export function compileProgram( }; } catch (err) { // TODO: we might want to log error here, but this will also result in duplicate logging + if (err instanceof CompilerError) { + retryErrors.push({fn, error: err}); + } return null; } } @@ -538,7 +546,7 @@ export function compileProgram( program.node.directives, ); if (moduleScopeOptOutDirectives.length > 0) { - return; + return null; } let gating: null | { gatingFn: ExternalFunction; @@ -596,7 +604,7 @@ export function compileProgram( } } catch (err) { handleError(err, pass, null); - return; + return null; } /* @@ -638,6 +646,7 @@ export function compileProgram( } addImportsToProgram(program, externalFunctions); } + return {retryErrors}; } function shouldSkipCompilation( diff --git a/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/ValidateNoUntransformedReferences.ts b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/ValidateNoUntransformedReferences.ts new file mode 100644 index 0000000000..07ab3b2b6a --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/ValidateNoUntransformedReferences.ts @@ -0,0 +1,275 @@ +import {NodePath} from '@babel/core'; +import * as t from '@babel/types'; + +import { + CompilerError, + CompilerErrorDetailOptions, + EnvironmentConfig, + ErrorSeverity, + Logger, +} from '..'; +import {getOrInsertWith} from '../Utils/utils'; +import {Environment} from '../HIR'; +import {DEFAULT_EXPORT} from '../HIR/Environment'; + +function throwInvalidReact( + options: Omit, + {logger, filename}: TraversalState, +): never { + const detail: CompilerErrorDetailOptions = { + ...options, + severity: ErrorSeverity.InvalidReact, + }; + logger?.logEvent(filename, { + kind: 'CompileError', + fnLoc: null, + detail, + }); + CompilerError.throw(detail); +} +function assertValidEffectImportReference( + numArgs: number, + paths: Array>, + context: TraversalState, +): void { + for (const path of paths) { + const parent = path.parentPath; + if (parent != null && parent.isCallExpression()) { + const args = parent.get('arguments'); + /** + * Only error on untransformed references of the form `useMyEffect(...)` + * or `moduleNamespace.useMyEffect(...)`, with matching argument counts. + * TODO: do we also want a mode to also hard error on non-call references? + */ + if (args.length === numArgs) { + const maybeErrorDiagnostic = matchCompilerDiagnostic( + path, + context.transformErrors, + ); + /** + * Note that we cannot easily check the type of the first argument here, + * as it may have already been transformed by the compiler (and not + * memoized). + */ + throwInvalidReact( + { + reason: + '[InferEffectDependencies] React Compiler is unable to infer dependencies of this effect. ' + + 'This will break your build! ' + + 'To resolve, either pass your own dependency array or fix reported compiler bailout diagnostics.', + description: maybeErrorDiagnostic + ? `(Bailout reason: ${maybeErrorDiagnostic})` + : null, + loc: parent.node.loc ?? null, + }, + context, + ); + } + } + } +} + +function assertValidFireImportReference( + paths: Array>, + context: TraversalState, +): void { + if (paths.length > 0) { + const maybeErrorDiagnostic = matchCompilerDiagnostic( + paths[0], + context.transformErrors, + ); + throwInvalidReact( + { + reason: + '[Fire] Untransformed reference to compiler-required feature. ' + + 'Either remove this `fire` call or ensure it is successfully transformed by the compiler', + description: maybeErrorDiagnostic + ? `(Bailout reason: ${maybeErrorDiagnostic})` + : null, + loc: paths[0].node.loc ?? null, + }, + context, + ); + } +} +export default function validateNoUntransformedReferences( + path: NodePath, + filename: string | null, + logger: Logger | null, + env: EnvironmentConfig, + transformErrors: Array<{fn: NodePath; error: CompilerError}>, +): void { + const moduleLoadChecks = new Map< + string, + Map + >(); + if (env.enableFire) { + /** + * Error on any untransformed references to `fire` (e.g. including non-call + * expressions) + */ + for (const module of Environment.knownReactModules) { + const react = getOrInsertWith(moduleLoadChecks, module, () => new Map()); + react.set('fire', assertValidFireImportReference); + } + } + if (env.inferEffectDependencies) { + for (const { + function: {source, importSpecifierName}, + numRequiredArgs, + } of env.inferEffectDependencies) { + const module = getOrInsertWith(moduleLoadChecks, source, () => new Map()); + module.set( + importSpecifierName, + assertValidEffectImportReference.bind(null, numRequiredArgs), + ); + } + } + if (moduleLoadChecks.size > 0) { + transformProgram(path, moduleLoadChecks, filename, logger, transformErrors); + } +} + +type TraversalState = { + shouldInvalidateScopes: boolean; + program: NodePath; + logger: Logger | null; + filename: string | null; + transformErrors: Array<{fn: NodePath; error: CompilerError}>; +}; +type CheckInvalidReferenceFn = ( + paths: Array>, + context: TraversalState, +) => void; + +function validateImportSpecifier( + specifier: NodePath, + importSpecifierChecks: Map, + state: TraversalState, +): void { + const imported = specifier.get('imported'); + const specifierName: string = + imported.node.type === 'Identifier' + ? imported.node.name + : imported.node.value; + const checkFn = importSpecifierChecks.get(specifierName); + if (checkFn == null) { + return; + } + if (state.shouldInvalidateScopes) { + state.shouldInvalidateScopes = false; + state.program.scope.crawl(); + } + + const local = specifier.get('local'); + const binding = local.scope.getBinding(local.node.name); + CompilerError.invariant(binding != null, { + reason: 'Expected binding to be found for import specifier', + loc: local.node.loc ?? null, + }); + checkFn(binding.referencePaths, state); +} + +function validateNamespacedImport( + specifier: NodePath, + importSpecifierChecks: Map, + state: TraversalState, +): void { + if (state.shouldInvalidateScopes) { + state.shouldInvalidateScopes = false; + state.program.scope.crawl(); + } + const local = specifier.get('local'); + const binding = local.scope.getBinding(local.node.name); + const defaultCheckFn = importSpecifierChecks.get(DEFAULT_EXPORT); + + CompilerError.invariant(binding != null, { + reason: 'Expected binding to be found for import specifier', + loc: local.node.loc ?? null, + }); + const filteredReferences = new Map< + CheckInvalidReferenceFn, + Array> + >(); + for (const reference of binding.referencePaths) { + if (defaultCheckFn != null) { + getOrInsertWith(filteredReferences, defaultCheckFn, () => []).push( + reference, + ); + } + const parent = reference.parentPath; + if ( + parent != null && + parent.isMemberExpression() && + parent.get('object') === reference + ) { + if (parent.node.computed || parent.node.property.type !== 'Identifier') { + continue; + } + const checkFn = importSpecifierChecks.get(parent.node.property.name); + if (checkFn != null) { + getOrInsertWith(filteredReferences, checkFn, () => []).push(parent); + } + } + } + + for (const [checkFn, references] of filteredReferences) { + checkFn(references, state); + } +} +function transformProgram( + path: NodePath, + + moduleLoadChecks: Map>, + filename: string | null, + logger: Logger | null, + transformErrors: Array<{fn: NodePath; error: CompilerError}>, +): void { + const traversalState: TraversalState = { + shouldInvalidateScopes: true, + program: path, + filename, + logger, + transformErrors, + }; + path.traverse({ + ImportDeclaration(path: NodePath) { + const importSpecifierChecks = moduleLoadChecks.get( + path.node.source.value, + ); + if (importSpecifierChecks == null) { + return; + } + const specifiers = path.get('specifiers'); + for (const specifier of specifiers) { + if (specifier.isImportSpecifier()) { + validateImportSpecifier( + specifier, + importSpecifierChecks, + traversalState, + ); + } else { + validateNamespacedImport( + specifier as NodePath< + t.ImportNamespaceSpecifier | t.ImportDefaultSpecifier + >, + importSpecifierChecks, + traversalState, + ); + } + } + }, + }); +} + +function matchCompilerDiagnostic( + badReference: NodePath, + transformErrors: Array<{fn: NodePath; error: CompilerError}>, +): string | null { + for (const {fn, error} of transformErrors) { + if (fn.isAncestor(badReference)) { + return error.toString(); + } + } + return null; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts index 4fce273b7a..1f64452e4b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/Environment.ts @@ -1121,6 +1121,7 @@ export class Environment { moduleName.toLowerCase() === 'react-dom' ); } + static knownReactModules: ReadonlyArray = ['react', 'react-dom']; getFallthroughPropertyType( receiver: Type, diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn-default-import.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn-default-import.expect.md new file mode 100644 index 0000000000..cd977ae834 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn-default-import.expect.md @@ -0,0 +1,26 @@ + +## Input + +```javascript +// @inferEffectDependencies @compilationMode(infer) @panicThreshold(none) +import useMyEffect from 'useEffectWrapper'; + +function nonReactFn(arg) { + useMyEffect(() => [1, 2, arg]); +} + +``` + + +## Error + +``` + 3 | + 4 | function nonReactFn(arg) { +> 5 | useMyEffect(() => [1, 2, arg]); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: [InferEffectDependencies] React Compiler is unable to infer dependencies of this effect. This will break your build! To resolve, either pass your own dependency array or fix reported compiler bailout diagnostics. (5:5) + 6 | } + 7 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn-default-import.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn-default-import.js new file mode 100644 index 0000000000..8061b44a3d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn-default-import.js @@ -0,0 +1,6 @@ +// @inferEffectDependencies @compilationMode(infer) @panicThreshold(none) +import useMyEffect from 'useEffectWrapper'; + +function nonReactFn(arg) { + useMyEffect(() => [1, 2, arg]); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn.expect.md new file mode 100644 index 0000000000..e36e9b3c0e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn.expect.md @@ -0,0 +1,26 @@ + +## Input + +```javascript +// @inferEffectDependencies @compilationMode(infer) @panicThreshold(none) +import {useEffect} from 'react'; + +function nonReactFn(arg) { + useEffect(() => [1, 2, arg]); +} + +``` + + +## Error + +``` + 3 | + 4 | function nonReactFn(arg) { +> 5 | useEffect(() => [1, 2, arg]); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: [InferEffectDependencies] React Compiler is unable to infer dependencies of this effect. This will break your build! To resolve, either pass your own dependency array or fix reported compiler bailout diagnostics. (5:5) + 6 | } + 7 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn.js new file mode 100644 index 0000000000..813c3b6c3d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.callsite-in-non-react-fn.js @@ -0,0 +1,6 @@ +// @inferEffectDependencies @compilationMode(infer) @panicThreshold(none) +import {useEffect} from 'react'; + +function nonReactFn(arg) { + useEffect(() => [1, 2, arg]); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.non-inlined-effect-fn.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.non-inlined-effect-fn.expect.md new file mode 100644 index 0000000000..ee17fb048e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.non-inlined-effect-fn.expect.md @@ -0,0 +1,41 @@ + +## Input + +```javascript +// @inferEffectDependencies @panicThreshold(none) +import {useEffect} from 'react'; + +/** + * Error on non-inlined effect functions: + * 1. From the effect hook callee's perspective, it only makes sense + * to either + * (a) never hard error (i.e. failing to infer deps is acceptable) or + * (b) always hard error, + * regardless of whether the callback function is an inline fn. + * 2. (Technical detail) it's harder to support detecting cases in which + * function (pre-Forget transform) was inline but becomes memoized + */ +function Component({foo}) { + function f() { + console.log(foo); + } + + // No inferred dep array, the argument is not a lambda + useEffect(f); +} + +``` + + +## Error + +``` + 18 | + 19 | // No inferred dep array, the argument is not a lambda +> 20 | useEffect(f); + | ^^^^^^^^^^^^ InvalidReact: [InferEffectDependencies] React Compiler is unable to infer dependencies of this effect. This will break your build! To resolve, either pass your own dependency array or fix reported compiler bailout diagnostics. (20:20) + 21 | } + 22 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.non-inlined-effect-fn.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.non-inlined-effect-fn.js new file mode 100644 index 0000000000..a011e3bf14 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.non-inlined-effect-fn.js @@ -0,0 +1,21 @@ +// @inferEffectDependencies @panicThreshold(none) +import {useEffect} from 'react'; + +/** + * Error on non-inlined effect functions: + * 1. From the effect hook callee's perspective, it only makes sense + * to either + * (a) never hard error (i.e. failing to infer deps is acceptable) or + * (b) always hard error, + * regardless of whether the callback function is an inline fn. + * 2. (Technical detail) it's harder to support detecting cases in which + * function (pre-Forget transform) was inline but becomes memoized + */ +function Component({foo}) { + function f() { + console.log(foo); + } + + // No inferred dep array, the argument is not a lambda + useEffect(f); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-import-default-property-useEffect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-import-default-property-useEffect.expect.md new file mode 100644 index 0000000000..71ecefeeea --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-import-default-property-useEffect.expect.md @@ -0,0 +1,27 @@ + +## Input + +```javascript +// @inferEffectDependencies @panicThreshold(none) +import React from 'react'; + +function NonReactiveDepInEffect() { + const obj = makeObject_Primitives(); + React.useEffect(() => print(obj)); +} + +``` + + +## Error + +``` + 4 | function NonReactiveDepInEffect() { + 5 | const obj = makeObject_Primitives(); +> 6 | React.useEffect(() => print(obj)); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: [InferEffectDependencies] React Compiler is unable to infer dependencies of this effect. This will break your build! To resolve, either pass your own dependency array or fix reported compiler bailout diagnostics. (6:6) + 7 | } + 8 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/todo.import-default-property-useEffect.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-import-default-property-useEffect.js similarity index 73% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/todo.import-default-property-useEffect.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-import-default-property-useEffect.js index 0dbae754ec..237535fe5b 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/todo.import-default-property-useEffect.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-import-default-property-useEffect.js @@ -1,4 +1,4 @@ -// @inferEffectDependencies +// @inferEffectDependencies @panicThreshold(none) import React from 'react'; function NonReactiveDepInEffect() { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-syntax.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-syntax.expect.md new file mode 100644 index 0000000000..575fd2101f --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-syntax.expect.md @@ -0,0 +1,51 @@ + +## Input + +```javascript +// @inferEffectDependencies @panicThreshold(none) +import {useSpecialEffect} from 'shared-runtime'; + +/** + * Note that a react compiler-based transform still has limitations on JS syntax. + * We should surface these as actionable lint / build errors to devs. + */ +function Component({prop1}) { + 'use memo'; + useSpecialEffect(() => { + try { + console.log(prop1); + } finally { + console.log('exiting'); + } + }, [prop1]); + return
{prop1}
; +} + +``` + + +## Error + +``` + 8 | function Component({prop1}) { + 9 | 'use memo'; +> 10 | useSpecialEffect(() => { + | ^^^^^^^^^^^^^^^^^^^^^^^^ +> 11 | try { + | ^^^^^^^^^ +> 12 | console.log(prop1); + | ^^^^^^^^^ +> 13 | } finally { + | ^^^^^^^^^ +> 14 | console.log('exiting'); + | ^^^^^^^^^ +> 15 | } + | ^^^^^^^^^ +> 16 | }, [prop1]); + | ^^^^^^^^^^^^^^ InvalidReact: [InferEffectDependencies] React Compiler is unable to infer dependencies of this effect. This will break your build! To resolve, either pass your own dependency array or fix reported compiler bailout diagnostics.. (Bailout reason: Todo: (BuildHIR::lowerStatement) Handle TryStatement without a catch clause (11:15)) (10:16) + 17 | return
{prop1}
; + 18 | } + 19 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-syntax.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-syntax.js new file mode 100644 index 0000000000..d3c83c2562 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.todo-syntax.js @@ -0,0 +1,18 @@ +// @inferEffectDependencies @panicThreshold(none) +import {useSpecialEffect} from 'shared-runtime'; + +/** + * Note that a react compiler-based transform still has limitations on JS syntax. + * We should surface these as actionable lint / build errors to devs. + */ +function Component({prop1}) { + 'use memo'; + useSpecialEffect(() => { + try { + console.log(prop1); + } finally { + console.log('exiting'); + } + }, [prop1]); + return
{prop1}
; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.use-no-memo.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.use-no-memo.expect.md new file mode 100644 index 0000000000..52b99a393e --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.use-no-memo.expect.md @@ -0,0 +1,27 @@ + +## Input + +```javascript +// @inferEffectDependencies @panicThreshold(none) +import {useEffect} from 'react'; + +function Component({propVal}) { + 'use no memo'; + useEffect(() => [propVal]); +} + +``` + + +## Error + +``` + 4 | function Component({propVal}) { + 5 | 'use no memo'; +> 6 | useEffect(() => [propVal]); + | ^^^^^^^^^^^^^^^^^^^^^^^^^^ InvalidReact: [InferEffectDependencies] React Compiler is unable to infer dependencies of this effect. This will break your build! To resolve, either pass your own dependency array or fix reported compiler bailout diagnostics. (6:6) + 7 | } + 8 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.use-no-memo.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.use-no-memo.js new file mode 100644 index 0000000000..53eeda5050 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/bailout-retry/error.use-no-memo.js @@ -0,0 +1,7 @@ +// @inferEffectDependencies @panicThreshold(none) +import {useEffect} from 'react'; + +function Component({propVal}) { + 'use no memo'; + useEffect(() => [propVal]); +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/infer-effect-dependencies.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/infer-effect-dependencies.expect.md index 89da346a72..d0745f9bd3 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/infer-effect-dependencies.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/infer-effect-dependencies.expect.md @@ -34,13 +34,6 @@ function Component({foo, bar}) { console.log(bar.qux); }); - function f() { - console.log(foo); - } - - // No inferred dep array, the argument is not a lambda - useEffect(f); - useEffectWrapper(() => { console.log(foo); }); @@ -58,7 +51,7 @@ import useEffectWrapper from "useEffectWrapper"; const moduleNonReactive = 0; function Component(t0) { - const $ = _c(14); + const $ = _c(12); const { foo, bar } = t0; const ref = useRef(0); @@ -119,7 +112,7 @@ function Component(t0) { useEffect(t4, [bar.baz, bar.qux]); let t5; if ($[10] !== foo) { - t5 = function f() { + t5 = () => { console.log(foo); }; $[10] = foo; @@ -127,20 +120,7 @@ function Component(t0) { } else { t5 = $[11]; } - const f = t5; - - useEffect(f); - let t6; - if ($[12] !== foo) { - t6 = () => { - console.log(foo); - }; - $[12] = foo; - $[13] = t6; - } else { - t6 = $[13]; - } - useEffectWrapper(t6, [foo]); + useEffectWrapper(t5, [foo]); } ``` diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/infer-effect-dependencies.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/infer-effect-dependencies.js index efdd416478..2bad5ee1cc 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/infer-effect-dependencies.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/infer-effect-dependencies.js @@ -30,13 +30,6 @@ function Component({foo, bar}) { console.log(bar.qux); }); - function f() { - console.log(foo); - } - - // No inferred dep array, the argument is not a lambda - useEffect(f); - useEffectWrapper(() => { console.log(foo); }); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/todo.import-default-property-useEffect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/todo.import-default-property-useEffect.expect.md deleted file mode 100644 index a5a576c830..0000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/infer-effect-dependencies/todo.import-default-property-useEffect.expect.md +++ /dev/null @@ -1,44 +0,0 @@ - -## Input - -```javascript -// @inferEffectDependencies -import React from 'react'; - -function NonReactiveDepInEffect() { - const obj = makeObject_Primitives(); - React.useEffect(() => print(obj)); -} - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; // @inferEffectDependencies -import React from "react"; - -function NonReactiveDepInEffect() { - const $ = _c(2); - let t0; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t0 = makeObject_Primitives(); - $[0] = t0; - } else { - t0 = $[0]; - } - const obj = t0; - let t1; - if ($[1] === Symbol.for("react.memo_cache_sentinel")) { - t1 = () => print(obj); - $[1] = t1; - } else { - t1 = $[1]; - } - React.useEffect(t1); -} - -``` - -### Eval output -(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.todo-infer-deps-on-retry.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.todo-infer-deps-on-retry.expect.md new file mode 100644 index 0000000000..0384d3335c --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.todo-infer-deps-on-retry.expect.md @@ -0,0 +1,42 @@ + +## Input + +```javascript +// @inferEffectDependencies @panicThreshold(none) +import {useRef} from 'react'; +import {useSpecialEffect} from 'shared-runtime'; + +/** + * The retry pipeline disables memoization features, which means we need to + * provide an alternate implementation of effect dependencies which does not + * rely on memoization. + */ +function useFoo({cond}) { + const ref = useRef(); + const derived = cond ? ref.current : makeObject(); + useSpecialEffect(() => { + log(derived); + }, [derived]); + return ref; +} + +``` + + +## Error + +``` + 11 | const ref = useRef(); + 12 | const derived = cond ? ref.current : makeObject(); +> 13 | useSpecialEffect(() => { + | ^^^^^^^^^^^^^^^^^^^^^^^^ +> 14 | log(derived); + | ^^^^^^^^^^^^^^^^^ +> 15 | }, [derived]); + | ^^^^^^^^^^^^^^^^ InvalidReact: [InferEffectDependencies] React Compiler is unable to infer dependencies of this effect. This will break your build! To resolve, either pass your own dependency array or fix reported compiler bailout diagnostics.. (Bailout reason: Invariant: Expected function expression scope to exist (13:15)) (13:15) + 16 | return ref; + 17 | } + 18 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.todo-infer-deps-on-retry.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.todo-infer-deps-on-retry.js new file mode 100644 index 0000000000..f3a57bb912 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.todo-infer-deps-on-retry.js @@ -0,0 +1,17 @@ +// @inferEffectDependencies @panicThreshold(none) +import {useRef} from 'react'; +import {useSpecialEffect} from 'shared-runtime'; + +/** + * The retry pipeline disables memoization features, which means we need to + * provide an alternate implementation of effect dependencies which does not + * rely on memoization. + */ +function useFoo({cond}) { + const ref = useRef(); + const derived = cond ? ref.current : makeObject(); + useSpecialEffect(() => { + log(derived); + }, [derived]); + return ref; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.todo-syntax.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.todo-syntax.expect.md index 86836d86b6..c5d7456d65 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.todo-syntax.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.todo-syntax.expect.md @@ -2,7 +2,7 @@ ## Input ```javascript -// @enableFire +// @enableFire @panicThreshold(none) import {fire} from 'react'; /** @@ -29,21 +29,13 @@ function Component({prop1}) { ## Error ``` - 9 | function Component({prop1}) { - 10 | const foo = () => { -> 11 | try { - | ^^^^^ -> 12 | console.log(prop1); - | ^^^^^^^^^^^^^^^^^^^^^^^^^ -> 13 | } finally { - | ^^^^^^^^^^^^^^^^^^^^^^^^^ -> 14 | console.log('jbrown215'); - | ^^^^^^^^^^^^^^^^^^^^^^^^^ -> 15 | } - | ^^^^^^ Todo: (BuildHIR::lowerStatement) Handle TryStatement without a catch clause (11:15) 16 | }; 17 | useEffect(() => { - 18 | fire(foo()); +> 18 | fire(foo()); + | ^^^^ InvalidReact: [Fire] Untransformed reference to compiler-required feature. Either remove this `fire` call or ensure it is successfully transformed by the compiler. (Bailout reason: Todo: (BuildHIR::lowerStatement) Handle TryStatement without a catch clause (11:15)) (18:18) + 19 | }); + 20 | } + 21 | ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.todo-syntax.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.todo-syntax.js index ec50ed84c5..b1ee459177 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.todo-syntax.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.todo-syntax.js @@ -1,4 +1,4 @@ -// @enableFire +// @enableFire @panicThreshold(none) import {fire} from 'react'; /** diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.untransformed-fire-reference.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.untransformed-fire-reference.expect.md new file mode 100644 index 0000000000..ddcc86ff00 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.untransformed-fire-reference.expect.md @@ -0,0 +1,23 @@ + +## Input + +```javascript +// @enableFire @panicThreshold(none) +import {fire} from 'react'; + +console.log(fire == null); + +``` + + +## Error + +``` + 2 | import {fire} from 'react'; + 3 | +> 4 | console.log(fire == null); + | ^^^^ InvalidReact: [Fire] Untransformed reference to compiler-required feature. Either remove this `fire` call or ensure it is successfully transformed by the compiler (4:4) + 5 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.untransformed-fire-reference.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.untransformed-fire-reference.js new file mode 100644 index 0000000000..25a60a9716 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.untransformed-fire-reference.js @@ -0,0 +1,4 @@ +// @enableFire @panicThreshold(none) +import {fire} from 'react'; + +console.log(fire == null); diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.use-no-memo.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.use-no-memo.expect.md new file mode 100644 index 0000000000..84a27b43b8 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.use-no-memo.expect.md @@ -0,0 +1,42 @@ + +## Input + +```javascript +// @enableFire @panicThreshold(none) +import {fire} from 'react'; + +/** + * TODO: we should eventually distinguish between `use no memo` and `use no + * compiler` directives. The former should be used to *only* disable memoization + * features. + */ +function Component({props, bar}) { + 'use no memo'; + const foo = () => { + console.log(props); + }; + useEffect(() => { + fire(foo(props)); + fire(foo()); + fire(bar()); + }); + + return null; +} + +``` + + +## Error + +``` + 13 | }; + 14 | useEffect(() => { +> 15 | fire(foo(props)); + | ^^^^ InvalidReact: [Fire] Untransformed reference to compiler-required feature. Either remove this `fire` call or ensure it is successfully transformed by the compiler (15:15) + 16 | fire(foo()); + 17 | fire(bar()); + 18 | }); +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/todo-use-no-memo.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.use-no-memo.js similarity index 51% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/todo-use-no-memo.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.use-no-memo.js index 2587e24ee1..92e1ff211a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/todo-use-no-memo.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/error.use-no-memo.js @@ -1,6 +1,11 @@ -// @enableFire +// @enableFire @panicThreshold(none) import {fire} from 'react'; +/** + * TODO: we should eventually distinguish between `use no memo` and `use no + * compiler` directives. The former should be used to *only* disable memoization + * features. + */ function Component({props, bar}) { 'use no memo'; const foo = () => { diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/no-fire-todo-syntax-shouldnt-throw.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/no-fire-todo-syntax-shouldnt-throw.expect.md new file mode 100644 index 0000000000..fecc28bb00 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/no-fire-todo-syntax-shouldnt-throw.expect.md @@ -0,0 +1,95 @@ + +## Input + +```javascript +// @enableFire @panicThreshold(none) +import {fire} from 'react'; + +/** + * Compilation of this file should succeed. + */ +function NonFireComponent({prop1}) { + /** + * This component bails out but does not use fire + */ + const foo = () => { + try { + console.log(prop1); + } finally { + console.log('jbrown215'); + } + }; + useEffect(() => { + foo(); + }); +} + +function FireComponent(props) { + /** + * This component uses fire and compiles successfully + */ + const foo = props => { + console.log(props); + }; + useEffect(() => { + fire(foo(props)); + }); + + return null; +} + +``` + +## Code + +```javascript +import { useFire } from "react/compiler-runtime"; +import { c as _c } from "react/compiler-runtime"; // @enableFire @panicThreshold(none) +import { fire } from "react"; + +/** + * Compilation of this file should succeed. + */ +function NonFireComponent({ prop1 }) { + /** + * This component bails out but does not use fire + */ + const foo = () => { + try { + console.log(prop1); + } finally { + console.log("jbrown215"); + } + }; + useEffect(() => { + foo(); + }); +} + +function FireComponent(props) { + const $ = _c(3); + + const foo = _temp; + const t0 = useFire(foo); + let t1; + if ($[0] !== props || $[1] !== t0) { + t1 = () => { + t0(props); + }; + $[0] = props; + $[1] = t0; + $[2] = t1; + } else { + t1 = $[2]; + } + useEffect(t1); + return null; +} +function _temp(props_0) { + console.log(props_0); +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/no-fire-todo-syntax-shouldnt-throw.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/no-fire-todo-syntax-shouldnt-throw.js new file mode 100644 index 0000000000..899fa33376 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/no-fire-todo-syntax-shouldnt-throw.js @@ -0,0 +1,35 @@ +// @enableFire @panicThreshold(none) +import {fire} from 'react'; + +/** + * Compilation of this file should succeed. + */ +function NonFireComponent({prop1}) { + /** + * This component bails out but does not use fire + */ + const foo = () => { + try { + console.log(prop1); + } finally { + console.log('jbrown215'); + } + }; + useEffect(() => { + foo(); + }); +} + +function FireComponent(props) { + /** + * This component uses fire and compiles successfully + */ + const foo = props => { + console.log(props); + }; + useEffect(() => { + fire(foo(props)); + }); + + return null; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/todo-use-no-memo.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/todo-use-no-memo.expect.md deleted file mode 100644 index 907501228b..0000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/transform-fire/bailout-retry/todo-use-no-memo.expect.md +++ /dev/null @@ -1,47 +0,0 @@ - -## Input - -```javascript -// @enableFire -import {fire} from 'react'; - -function Component({props, bar}) { - 'use no memo'; - const foo = () => { - console.log(props); - }; - useEffect(() => { - fire(foo(props)); - fire(foo()); - fire(bar()); - }); - - return null; -} - -``` - -## Code - -```javascript -// @enableFire -import { fire } from "react"; - -function Component({ props, bar }) { - "use no memo"; - const foo = () => { - console.log(props); - }; - useEffect(() => { - fire(foo(props)); - fire(foo()); - fire(bar()); - }); - - return null; -} - -``` - -### Eval output -(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/eslint-plugin-react-compiler/__tests__/ReactCompilerRule-test.ts b/compiler/packages/eslint-plugin-react-compiler/__tests__/ReactCompilerRule-test.ts index 71be6b6622..ea5c30d5b2 100644 --- a/compiler/packages/eslint-plugin-react-compiler/__tests__/ReactCompilerRule-test.ts +++ b/compiler/packages/eslint-plugin-react-compiler/__tests__/ReactCompilerRule-test.ts @@ -274,6 +274,38 @@ const tests: CompilerTestCases = { }, ], }, + { + name: 'Pipeline errors are reported', + code: normalizeIndent` + import useMyEffect from 'useMyEffect'; + function Component({a}) { + 'use no memo'; + useMyEffect(() => console.log(a.b)); + return
Hello world
; + } + `, + options: [ + { + environment: { + inferEffectDependencies: [ + { + function: { + source: 'useMyEffect', + importSpecifierName: 'default', + }, + numRequiredArgs: 1, + }, + ], + }, + }, + ], + errors: [ + { + message: + '[InferEffectDependencies] React Compiler is unable to infer dependencies of this effect. This will break your build! To resolve, either pass your own dependency array or fix reported compiler bailout diagnostics.', + }, + ], + }, ], }; From 1b6e3dd985c0cfaa4f15fae6fda26f7b57bd25da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 14 Mar 2025 13:16:20 -0400 Subject: [PATCH 08/12] Merge restoreEnterViewTransitions and restoreExitViewTransitions (#32585) This is the exact same code in both cases. It's just general clean up. By unifying them it becomes less confusing to reuse these helpers in the Apply Gesture path where the naming is reversed. --- .../src/ReactFiberCommitViewTransitions.js | 35 +++++-------------- .../src/ReactFiberCommitWork.js | 9 +++-- 2 files changed, 13 insertions(+), 31 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitViewTransitions.js b/packages/react-reconciler/src/ReactFiberCommitViewTransitions.js index 22b9d11ac6..5dcf1498ed 100644 --- a/packages/react-reconciler/src/ReactFiberCommitViewTransitions.js +++ b/packages/react-reconciler/src/ReactFiberCommitViewTransitions.js @@ -514,37 +514,20 @@ function restorePairedViewTransitions(parent: Fiber): void { } } -export function restoreEnterViewTransitions(placement: Fiber): void { - if (placement.tag === ViewTransitionComponent) { - const instance: ViewTransitionState = placement.stateNode; +export function restoreEnterOrExitViewTransitions(fiber: Fiber): void { + if (fiber.tag === ViewTransitionComponent) { + const instance: ViewTransitionState = fiber.stateNode; instance.paired = null; - restoreViewTransitionOnHostInstances(placement.child, false); - restorePairedViewTransitions(placement); - } else if ((placement.subtreeFlags & ViewTransitionStatic) !== NoFlags) { - let child = placement.child; + restoreViewTransitionOnHostInstances(fiber.child, false); + restorePairedViewTransitions(fiber); + } else if ((fiber.subtreeFlags & ViewTransitionStatic) !== NoFlags) { + let child = fiber.child; while (child !== null) { - restoreEnterViewTransitions(child); + restoreEnterOrExitViewTransitions(child); child = child.sibling; } } else { - restorePairedViewTransitions(placement); - } -} - -export function restoreExitViewTransitions(deletion: Fiber): void { - if (deletion.tag === ViewTransitionComponent) { - const instance: ViewTransitionState = deletion.stateNode; - instance.paired = null; - restoreViewTransitionOnHostInstances(deletion.child, false); - restorePairedViewTransitions(deletion); - } else if ((deletion.subtreeFlags & ViewTransitionStatic) !== NoFlags) { - let child = deletion.child; - while (child !== null) { - restoreExitViewTransitions(child); - child = child.sibling; - } - } else { - restorePairedViewTransitions(deletion); + restorePairedViewTransitions(fiber); } } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index 8b82e6a7e7..bcf7fc188e 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -246,8 +246,7 @@ import { commitExitViewTransitions, commitBeforeUpdateViewTransition, commitNestedViewTransitions, - restoreEnterViewTransitions, - restoreExitViewTransitions, + restoreEnterOrExitViewTransitions, restoreUpdateViewTransition, restoreNestedViewTransitions, measureUpdateViewTransition, @@ -3228,7 +3227,7 @@ function commitPassiveMountOnFiber( // This was a new mount. This means we could've triggered an enter animation on // the content. Restore the view transitions if there were any assigned in the // snapshot phase. - restoreEnterViewTransitions(finishedWork); + restoreEnterOrExitViewTransitions(finishedWork); } // When updating this function, also update reconnectPassiveEffects, which does @@ -3529,7 +3528,7 @@ function commitPassiveMountOnFiber( // Content is now hidden but wasn't before. This means we could've // triggered an exit animation on the content. Restore the view // transitions if there were any assigned in the snapshot phase. - restoreExitViewTransitions(current); + restoreEnterOrExitViewTransitions(current); } if (instance._visibility & OffscreenPassiveEffectsConnected) { // The effects are currently connected. Update them. @@ -3576,7 +3575,7 @@ function commitPassiveMountOnFiber( // Content is now visible but wasn't before. This means we could've // triggered an enter animation on the content. Restore the view // transitions if there were any assigned in the snapshot phase. - restoreEnterViewTransitions(finishedWork); + restoreEnterOrExitViewTransitions(finishedWork); } if (instance._visibility & OffscreenPassiveEffectsConnected) { // The effects are currently connected. Update them. From 3e956805e899bff7aea7b19c56e6adaf362cdc2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 14 Mar 2025 13:16:30 -0400 Subject: [PATCH 09/12] Gate flushGestureMutations and flushGestureAnimations (#32605) Normally these are gated by the whole commitGestureOnRoot path but in the case of an early commit these phases may need to be invoked. Earlier. Those paths weren't gated which I noticed when I started adding code to them. --- packages/react-reconciler/src/ReactFiberWorkLoop.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.js b/packages/react-reconciler/src/ReactFiberWorkLoop.js index d7fe089363..380fa20c3d 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.js @@ -3927,6 +3927,9 @@ function commitGestureOnRoot( } function flushGestureMutations(): void { + if (!enableSwipeTransition) { + return; + } if (pendingEffectsStatus !== PENDING_GESTURE_MUTATION_PHASE) { return; } @@ -3953,6 +3956,9 @@ function flushGestureMutations(): void { } function flushGestureAnimations(): void { + if (!enableSwipeTransition) { + return; + } // If we get canceled before we start we might not have applied // mutations yet. We need to apply them first. flushGestureMutations(); From 6daef4e7c8eea99c1b7eb7e451e028c6d7358321 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 14 Mar 2025 13:16:42 -0400 Subject: [PATCH 10/12] Make xViewTransitionToHostInstances helpers reusable (#32611) This prepares from being able to reuse some this in ApplyGesture. These all start with resetting a counter but it's tricky to have to remember to do this and tricky to do from the outside of this module. So we make an exported helper that does the resetting. Ideally it gets inlined. We also stop passing "current" to measureViewTransitionHostInstances. Same thing for cancelViewTransitionHostInstances. This doesn't make sense for "nested" which has not updated and so might not have an alternate. Instead we pass in the old and new name if they might be different. --- .../src/ReactFiberCommitViewTransitions.js | 163 +++++++++++------- .../src/ReactFiberCommitWork.js | 14 +- 2 files changed, 110 insertions(+), 67 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitViewTransitions.js b/packages/react-reconciler/src/ReactFiberCommitViewTransitions.js index 5dcf1498ed..09f749bd22 100644 --- a/packages/react-reconciler/src/ReactFiberCommitViewTransitions.js +++ b/packages/react-reconciler/src/ReactFiberCommitViewTransitions.js @@ -71,15 +71,40 @@ export let viewTransitionCancelableChildren: null | Array< Instance | string | Props, > = null; // tupled array where each entry is [instance: Instance, oldName: string, props: Props] -export function setViewTransitionCancelableChildren( - children: null | Array, +export function pushViewTransitionCancelableScope(): null | Array< + Instance | string | Props, +> { + const prevChildren = viewTransitionCancelableChildren; + viewTransitionCancelableChildren = null; + return prevChildren; +} + +export function popViewTransitionCancelableScope( + prevChildren: null | Array, ): void { - viewTransitionCancelableChildren = children; + viewTransitionCancelableChildren = prevChildren; } let viewTransitionHostInstanceIdx = 0; -function applyViewTransitionToHostInstances( +export function applyViewTransitionToHostInstances( + child: null | Fiber, + name: string, + className: ?string, + collectMeasurements: null | Array, + stopAtNestedViewTransitions: boolean, +): boolean { + viewTransitionHostInstanceIdx = 0; + return applyViewTransitionToHostInstancesRecursive( + child, + name, + className, + collectMeasurements, + stopAtNestedViewTransitions, + ); +} + +function applyViewTransitionToHostInstancesRecursive( child: null | Fiber, name: string, className: ?string, @@ -128,7 +153,7 @@ function applyViewTransitionToHostInstances( // inner most one is the one that handles the update. } else { if ( - applyViewTransitionToHostInstances( + applyViewTransitionToHostInstancesRecursive( child.child, name, className, @@ -207,7 +232,6 @@ function commitAppearingPairViewTransitions(placement: Fiber): void { if (className !== 'none') { // We found a new appearing view transition with the same name as this deletion. // We'll transition between them. - viewTransitionHostInstanceIdx = 0; const inViewport = applyViewTransitionToHostInstances( child.child, name, @@ -242,7 +266,6 @@ export function commitEnterViewTransitions(placement: Fiber): void { state.paired ? props.share : props.enter, ); if (className !== 'none') { - viewTransitionHostInstanceIdx = 0; const inViewport = applyViewTransitionToHostInstances( placement.child, name, @@ -310,7 +333,6 @@ function commitDeletedPairViewTransitions(deletion: Fiber): void { ); if (className !== 'none') { // We found a new appearing view transition with the same name as this deletion. - viewTransitionHostInstanceIdx = 0; const inViewport = applyViewTransitionToHostInstances( child.child, name, @@ -361,7 +383,6 @@ export function commitExitViewTransitions(deletion: Fiber): void { pair !== undefined ? props.share : props.exit, ); if (className !== 'none') { - viewTransitionHostInstanceIdx = 0; const inViewport = applyViewTransitionToHostInstances( deletion.child, name, @@ -449,7 +470,6 @@ export function commitBeforeUpdateViewTransition( return; } } - viewTransitionHostInstanceIdx = 0; applyViewTransitionToHostInstances( current.child, oldName, @@ -472,7 +492,6 @@ export function commitNestedViewTransitions(changedParent: Fiber): void { props.layout, ); if (className !== 'none') { - viewTransitionHostInstanceIdx = 0; applyViewTransitionToHostInstances( child.child, name, @@ -553,9 +572,22 @@ export function restoreNestedViewTransitions(changedParent: Fiber): void { } } -function cancelViewTransitionHostInstances( - currentViewTransition: Fiber, +export function cancelViewTransitionHostInstances( child: null | Fiber, + oldName: string, + stopAtNestedViewTransitions: boolean, +): void { + viewTransitionHostInstanceIdx = 0; + cancelViewTransitionHostInstancesRecursive( + child, + oldName, + stopAtNestedViewTransitions, + ); +} + +function cancelViewTransitionHostInstancesRecursive( + child: null | Fiber, + oldName: string, stopAtNestedViewTransitions: boolean, ): void { if (!supportsMutation) { @@ -564,10 +596,6 @@ function cancelViewTransitionHostInstances( while (child !== null) { if (child.tag === HostComponent) { const instance: Instance = child.stateNode; - const oldName = getViewTransitionName( - currentViewTransition.memoizedProps, - currentViewTransition.stateNode, - ); if (viewTransitionCancelableChildren === null) { viewTransitionCancelableChildren = []; } @@ -589,9 +617,9 @@ function cancelViewTransitionHostInstances( // Skip any nested view transitions for updates since in that case the // inner most one is the one that handles the update. } else { - cancelViewTransitionHostInstances( - currentViewTransition, + cancelViewTransitionHostInstancesRecursive( child.child, + oldName, stopAtNestedViewTransitions, ); } @@ -599,11 +627,32 @@ function cancelViewTransitionHostInstances( } } -function measureViewTransitionHostInstances( - currentViewTransition: Fiber, +export function measureViewTransitionHostInstances( parentViewTransition: Fiber, child: null | Fiber, - name: string, + newName: string, + oldName: string, + className: ?string, + previousMeasurements: null | Array, + stopAtNestedViewTransitions: boolean, +): boolean { + viewTransitionHostInstanceIdx = 0; + return measureViewTransitionHostInstancesRecursive( + parentViewTransition, + child, + newName, + oldName, + className, + previousMeasurements, + stopAtNestedViewTransitions, + ); +} + +function measureViewTransitionHostInstancesRecursive( + parentViewTransition: Fiber, + child: null | Fiber, + newName: string, + oldName: string, className: ?string, previousMeasurements: null | Array, stopAtNestedViewTransitions: boolean, @@ -654,10 +703,10 @@ function measureViewTransitionHostInstances( applyViewTransitionName( instance, viewTransitionHostInstanceIdx === 0 - ? name + ? newName : // If we have multiple Host Instances below, we add a suffix to the name to give // each one a unique name. - name + '_' + viewTransitionHostInstanceIdx, + newName + '_' + viewTransitionHostInstanceIdx, className, ); } @@ -667,10 +716,6 @@ function measureViewTransitionHostInstances( // animating it. However, in the current model this only works if the parent also // doesn't animate. So we have to queue these and wait until we complete the parent // to cancel them. - const oldName = getViewTransitionName( - currentViewTransition.memoizedProps, - currentViewTransition.stateNode, - ); if (viewTransitionCancelableChildren === null) { viewTransitionCancelableChildren = []; } @@ -696,11 +741,11 @@ function measureViewTransitionHostInstances( parentViewTransition.flags |= child.flags & AffectedParentLayout; } else { if ( - measureViewTransitionHostInstances( - currentViewTransition, + measureViewTransitionHostInstancesRecursive( parentViewTransition, child.child, - name, + newName, + oldName, className, previousMeasurements, stopAtNestedViewTransitions, @@ -719,6 +764,11 @@ export function measureUpdateViewTransition( finishedWork: Fiber, ): boolean { const props: ViewTransitionProps = finishedWork.memoizedProps; + const newName = getViewTransitionName(props, finishedWork.stateNode); + const oldName = getViewTransitionName( + current.memoizedProps, + current.stateNode, + ); const updateClassName: ?string = getViewTransitionClassName( props.className, props.update, @@ -745,24 +795,21 @@ export function measureUpdateViewTransition( if (layoutClassName === 'none') { // If we did not update, then all changes are considered a layout. We'll // attempt to cancel. - viewTransitionHostInstanceIdx = 0; - cancelViewTransitionHostInstances(current, finishedWork.child, true); + cancelViewTransitionHostInstances(finishedWork.child, oldName, true); return false; } // We didn't update but we might still apply layout so we measure each // instance to see if it moved or resized. className = layoutClassName; } - const name = getViewTransitionName(props, finishedWork.stateNode); // If nothing changed due to a mutation, or children changing size // and the measurements end up unchanged, we should restore it to not animate. - viewTransitionHostInstanceIdx = 0; const previousMeasurements = current.memoizedState; const inViewport = measureViewTransitionHostInstances( - current, finishedWork, finishedWork.child, - name, + newName, + oldName, className, previousMeasurements, true, @@ -782,29 +829,25 @@ export function measureNestedViewTransitions(changedParent: Fiber): void { let child = changedParent.child; while (child !== null) { if (child.tag === ViewTransitionComponent) { - const current = child.alternate; - if (current !== null) { - const props: ViewTransitionProps = child.memoizedProps; - const name = getViewTransitionName(props, child.stateNode); - const className: ?string = getViewTransitionClassName( - props.className, - props.layout, - ); - viewTransitionHostInstanceIdx = 0; - const inViewport = measureViewTransitionHostInstances( - current, - child, - child.child, - name, - className, - child.memoizedState, - false, - ); - if ((child.flags & Update) === NoFlags || !inViewport) { - // Nothing changed. - } else { - scheduleViewTransitionEvent(child, props.onLayout); - } + const props: ViewTransitionProps = child.memoizedProps; + const name = getViewTransitionName(props, child.stateNode); + const className: ?string = getViewTransitionClassName( + props.className, + props.layout, + ); + const inViewport = measureViewTransitionHostInstances( + child, + child.child, + name, + name, // Since this is unchanged, new and old name is the same. + className, + child.memoizedState, + false, + ); + if ((child.flags & Update) === NoFlags || !inViewport) { + // Nothing changed. + } else { + scheduleViewTransitionEvent(child, props.onLayout); } } else if ((child.subtreeFlags & ViewTransitionStatic) !== NoFlags) { measureNestedViewTransitions(child); diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.js b/packages/react-reconciler/src/ReactFiberCommitWork.js index bcf7fc188e..2663c1e087 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.js @@ -254,7 +254,8 @@ import { resetAppearingViewTransitions, trackAppearingViewTransition, viewTransitionCancelableChildren, - setViewTransitionCancelableChildren, + pushViewTransitionCancelableScope, + popViewTransitionCancelableScope, } from './ReactFiberCommitViewTransitions'; import { viewTransitionMutationContext, @@ -2474,14 +2475,14 @@ function commitAfterMutationEffectsOnFiber( switch (finishedWork.tag) { case HostRoot: { viewTransitionContextChanged = false; - setViewTransitionCancelableChildren(null); + pushViewTransitionCancelableScope(); recursivelyTraverseAfterMutationEffects(root, finishedWork, lanes); if (!viewTransitionContextChanged) { // If we didn't leak any resizing out to the root, we don't have to transition // the root itself. This means that we can now safely cancel any cancellations // that bubbled all the way up. const cancelableChildren = viewTransitionCancelableChildren; - setViewTransitionCancelableChildren(null); + popViewTransitionCancelableScope(null); if (cancelableChildren !== null) { for (let i = 0; i < cancelableChildren.length; i += 3) { cancelViewTransitionName( @@ -2532,9 +2533,8 @@ function commitAfterMutationEffectsOnFiber( const wasMutated = (finishedWork.flags & Update) !== NoFlags; const prevContextChanged = viewTransitionContextChanged; - const prevCancelableChildren = viewTransitionCancelableChildren; + const prevCancelableChildren = pushViewTransitionCancelableScope(); viewTransitionContextChanged = false; - setViewTransitionCancelableChildren(null); recursivelyTraverseAfterMutationEffects(root, finishedWork, lanes); if (viewTransitionContextChanged) { @@ -2557,7 +2557,7 @@ function commitAfterMutationEffectsOnFiber( prevCancelableChildren, viewTransitionCancelableChildren, ); - setViewTransitionCancelableChildren(prevCancelableChildren); + popViewTransitionCancelableScope(prevCancelableChildren); } // TODO: If this doesn't end up canceled, because a parent animates, // then we should probably issue an event since this instance is part of it. @@ -2571,7 +2571,7 @@ function commitAfterMutationEffectsOnFiber( ); // If this boundary did update, we cannot cancel its children so those are dropped. - setViewTransitionCancelableChildren(prevCancelableChildren); + popViewTransitionCancelableScope(prevCancelableChildren); } if ((finishedWork.flags & AffectedParentLayout) !== NoFlags) { From c4a3b92e098cf1896939758e7419cbdb0e2f0cf4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Sebastian=20Markb=C3=A5ge?= Date: Fri, 14 Mar 2025 13:20:17 -0400 Subject: [PATCH 11/12] Add more phases to the ReactFiberApplyGesture (#32578) Stacked on #32585 and #32605. This adds more loops for the phases of "Apply Gesture". It doesn't implement the interesting bit yet like adding view-transition-names and measurements. I'll do that in a separate PR to keep reviewing easier. The three phases of this approach is roughly: - Clone and apply names to the "old" state. - Inside startViewTransition: Apply names to the "new" state. Measure both the "old" and "new" state to know whether to cancel some of them. Delete the clones which will include all the "old" names. - After startViewTransition: Restore "new" names back to no view-transition-name. Since we don't have any other Effects in these phases we have a bit more flexibility and we can avoid extra phases that traverse the tree. I've tried to avoid any additional passes. An interesting consequence of this approach is that we could measure both the "old" and "new" state before `startViewTransition`. This would be more efficient because we wouldn't need to take View Transition snapshots of parts of the tree that won't actually animate. However, that would require an extra pass and force layout earlier. It would also have different semantics from the fire-and-forget View Transitions because we could optimize better which can be visible. It would also not account for any late mutations. So I decided to instead let the layout be computed by painting as usual and then measure both "old" and "new" inside the startViewTransition instead. Then canceling anything that doesn't animate to keep it consistent. Unfortunately, though there's not a lot of code sharing possible in these phases because the strategy is so different with the cloning and because the animation is performed in reverse. The "finishedWork" Fiber represents the "old" state and the "current" Fiber represents the "new" state. The most complicated phase is the cloning. I actually ended up having to make a very different pattern from the other phases and CommitWork in general. Because we have to clone as we go and also do other things like apply names and finding pairs, it has more phases. I ended up with an approach that uses three different loops. The outer one for updated trees, one for inserted trees that don't need cloning (doesn't include reappearing offscreen) and one for not updated trees that still need cloning. Inside each loop it can also be in different phases which I track with the `visitPhase` enum - this pattern is kind of new. Additionally, we need to measure the cloned nodes after we've applied mutations to them and we have to wait until the whole tree is inserted. We don't have a reference to these DOM elements in the Fiber tree since that still refers to the original ones. We need to store the cloned elements somewhere. So I added a temporary field on the ViewTransitionState to keep track of any clones owned by that ViewTransition. When we deep clone an unchanged subtree we don't have DOM element instances. It wouldn't be quite safe to try to find them from the tree structure. So we need to avoid the deep clones if we might need DOM elements. Therefore we keep traversing in the case where we need to find nested ViewTransition boundaries that are either potentially affected by layout or a "pair". For the other two phases the pattern there's a lot of code duplication since it's slightly different from the commit ones but they at least follow the same pattern. For the restore phase I was actually able to reuse most of the code. I don't love how much code this is. --- packages/react-reconciler/src/ReactFiber.js | 1 + .../src/ReactFiberApplyGesture.js | 829 +++++++++++++++--- .../src/ReactFiberViewTransitionComponent.js | 3 +- 3 files changed, 721 insertions(+), 112 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiber.js b/packages/react-reconciler/src/ReactFiber.js index 70cb3ed9de..f6011a3ba1 100644 --- a/packages/react-reconciler/src/ReactFiber.js +++ b/packages/react-reconciler/src/ReactFiber.js @@ -878,6 +878,7 @@ export function createFiberFromViewTransition( const instance: ViewTransitionState = { autoName: null, paired: null, + clones: null, ref: null, }; fiber.stateNode = instance; diff --git a/packages/react-reconciler/src/ReactFiberApplyGesture.js b/packages/react-reconciler/src/ReactFiberApplyGesture.js index 6847a2426c..a83c52c57c 100644 --- a/packages/react-reconciler/src/ReactFiberApplyGesture.js +++ b/packages/react-reconciler/src/ReactFiberApplyGesture.js @@ -13,6 +13,11 @@ import type {Instance, TextInstance} from './ReactFiberConfig'; import type {OffscreenState} from './ReactFiberActivityComponent'; +import type { + ViewTransitionState, + ViewTransitionProps, +} from './ReactFiberViewTransitionComponent'; + import { cloneMutableInstance, cloneMutableTextInstance, @@ -40,6 +45,8 @@ import { ContentReset, NoFlags, Visibility, + ViewTransitionNamedStatic, + ViewTransitionStatic, } from './ReactFiberFlags'; import { HostComponent, @@ -50,6 +57,10 @@ import { OffscreenComponent, ViewTransitionComponent, } from './ReactWorkTags'; +import { + restoreEnterOrExitViewTransitions, + restoreNestedViewTransitions, +} from './ReactFiberCommitViewTransitions'; let didWarnForRootClone = false; @@ -57,26 +68,284 @@ function detectMutationOrInsertClones(finishedWork: Fiber): boolean { return true; } -let unhideHostChildren = false; +const CLONE_UPDATE = 0; // Mutations in this subtree or potentially affected by layout. +const CLONE_EXIT = 1; // Inside a reappearing offscreen before the next ViewTransition or HostComponent. +const CLONE_UNHIDE = 2; // Inside a reappearing offscreen before the next HostComponent. +const CLONE_APPEARING_PAIR = 3; // Like UNHIDE but we're already inside the first Host Component only finding pairs. +const CLONE_UNCHANGED = 4; // Nothing in this tree was changed but we're still walking to clone it. +const INSERT_EXIT = 5; // Inside a newly mounted tree before the next ViewTransition or HostComponent. +const INSERT_APPEND = 6; // Inside a newly mounted tree before the next HostComponent. +const INSERT_APPEARING_PAIR = 7; // Inside a newly mounted tree only finding pairs. +type VisitPhase = 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7; + +function trackDeletedPairViewTransitions(deletion: Fiber): void { + if ((deletion.subtreeFlags & ViewTransitionNamedStatic) === NoFlags) { + // This has no named view transitions in its subtree. + return; + } + let child = deletion.child; + while (child !== null) { + if (child.tag === OffscreenComponent && child.memoizedState === null) { + // This tree was already hidden so we skip it. + } else { + if ( + child.tag === ViewTransitionComponent && + (child.flags & ViewTransitionNamedStatic) !== NoFlags + ) { + const props: ViewTransitionProps = child.memoizedProps; + const name = props.name; + if (name != null && name !== 'auto') { + // TODO: Find a pair + } + } + trackDeletedPairViewTransitions(child); + } + child = child.sibling; + } +} + +function trackEnterViewTransitions(deletion: Fiber): void { + if (deletion.tag === ViewTransitionComponent) { + const props: ViewTransitionProps = deletion.memoizedProps; + const name = props.name; + if (name != null && name !== 'auto') { + // TODO: Find a pair + } + // Look for more pairs deeper in the tree. + trackDeletedPairViewTransitions(deletion); + } else if ((deletion.subtreeFlags & ViewTransitionStatic) !== NoFlags) { + let child = deletion.child; + while (child !== null) { + trackEnterViewTransitions(child); + child = child.sibling; + } + } else { + trackDeletedPairViewTransitions(deletion); + } +} + +function recursivelyInsertNew( + parentFiber: Fiber, + hostParentClone: Instance, + parentViewTransition: null | ViewTransitionState, + visitPhase: VisitPhase, +): void { + if ( + visitPhase === INSERT_APPEARING_PAIR && + parentViewTransition === null && + (parentFiber.subtreeFlags & ViewTransitionNamedStatic) === NoFlags + ) { + // We're just searching for pairs but we have reached the end. + return; + } + let child = parentFiber.child; + while (child !== null) { + recursivelyInsertNewFiber( + child, + hostParentClone, + parentViewTransition, + visitPhase, + ); + child = child.sibling; + } +} + +function recursivelyInsertNewFiber( + finishedWork: Fiber, + hostParentClone: Instance, + parentViewTransition: null | ViewTransitionState, + visitPhase: VisitPhase, +): void { + switch (finishedWork.tag) { + case HostHoistable: { + if (supportsResources) { + // TODO: Hoistables should get optimistically inserted and then removed. + recursivelyInsertNew( + finishedWork, + hostParentClone, + parentViewTransition, + visitPhase, + ); + break; + } + // Fall through + } + case HostSingleton: { + if (supportsSingletons) { + recursivelyInsertNew( + finishedWork, + hostParentClone, + parentViewTransition, + visitPhase, + ); + + if (__DEV__) { + // We cannot apply mutations to Host Singletons since by definition + // they cannot be cloned. Therefore we warn in DEV if this commit + // had any effect. + if (finishedWork.flags & Update) { + console.error( + 'useSwipeTransition() caused something to render a new <%s>. ' + + 'This is not possible in the current implementation. ' + + "Make sure that the swipe doesn't mount any new <%s> elements.", + finishedWork.type, + finishedWork.type, + ); + } + } + break; + } + // Fall through + } + case HostComponent: { + const instance: Instance = finishedWork.stateNode; + // For insertions we don't need to clone. It's already new state node. + if (visitPhase !== INSERT_APPEARING_PAIR) { + appendChild(hostParentClone, instance); + recursivelyInsertNew( + finishedWork, + instance, + null, + INSERT_APPEARING_PAIR, + ); + } else { + recursivelyInsertNew(finishedWork, instance, null, visitPhase); + } + if (parentViewTransition !== null) { + if (parentViewTransition.clones === null) { + parentViewTransition.clones = [instance]; + } else { + parentViewTransition.clones.push(instance); + } + } + break; + } + case HostText: { + const textInstance: TextInstance = finishedWork.stateNode; + if (textInstance === null) { + throw new Error( + 'This should have a text node initialized. This error is likely ' + + 'caused by a bug in React. Please file an issue.', + ); + } + // For insertions we don't need to clone. It's already new state node. + if (visitPhase !== INSERT_APPEARING_PAIR) { + appendChild(hostParentClone, textInstance); + } + break; + } + case HostPortal: { + // TODO: Consider what should happen to Portals. For now we exclude them. + break; + } + case OffscreenComponent: { + const newState: OffscreenState | null = finishedWork.memoizedState; + const isHidden = newState !== null; + if (!isHidden) { + // Only insert nodes if this tree is going to be visible. No need to + // insert invisible content. + // Since there was no mutation to this node, it couldn't have changed + // visibility so we don't need to update visitPhase here. + recursivelyInsertNew( + finishedWork, + hostParentClone, + parentViewTransition, + visitPhase, + ); + } + break; + } + case ViewTransitionComponent: + const prevMutationContext = pushMutationContext(); + const viewTransitionState: ViewTransitionState = finishedWork.stateNode; + // TODO: If this was already cloned by a previous pass we can reuse those clones. + viewTransitionState.clones = null; + let nextPhase; + if (visitPhase === INSERT_EXIT) { + // This was an Enter of a ViewTransition. We now move onto inserting the inner + // HostComponents and finding inner pairs. + nextPhase = INSERT_APPEND; + // TODO: Mark the name and find a pair. + } else { + nextPhase = visitPhase; + } + recursivelyInsertNew( + finishedWork, + hostParentClone, + viewTransitionState, + nextPhase, + ); + popMutationContext(prevMutationContext); + break; + default: { + recursivelyInsertNew( + finishedWork, + hostParentClone, + parentViewTransition, + visitPhase, + ); + break; + } + } +} function recursivelyInsertClonesFromExistingTree( parentFiber: Fiber, hostParentClone: Instance, + parentViewTransition: null | ViewTransitionState, + visitPhase: VisitPhase, ): void { let child = parentFiber.child; while (child !== null) { switch (child.tag) { case HostComponent: { const instance: Instance = child.stateNode; - // If we have no mutations in this subtree, we just need to make a deep clone. - const clone: Instance = cloneMutableInstance(instance, true); + let nextPhase: VisitPhase; + switch (visitPhase) { + case CLONE_EXIT: + case CLONE_UNHIDE: + case CLONE_APPEARING_PAIR: + // If this was an unhide, we need to keep going if there are any named + // pairs in this subtree, since they might need to be marked. + nextPhase = + (child.subtreeFlags & ViewTransitionNamedStatic) !== NoFlags + ? CLONE_APPEARING_PAIR + : CLONE_UNCHANGED; + break; + default: + // We've found any "layout" View Transitions at this point so we can bail. + nextPhase = CLONE_UNCHANGED; + } + let clone: Instance; + if (nextPhase !== CLONE_UNCHANGED) { + // We might need a handle on these clones, so we need to do a shallow clone + // and keep going. + clone = cloneMutableInstance(instance, false); + recursivelyInsertClonesFromExistingTree( + child, + clone, + null, + nextPhase, + ); + } else { + // If we have no mutations in this subtree, and we don't need a handle on the + // clones, then we can do a deep clone instead and bailout. + clone = cloneMutableInstance(instance, true); + // TODO: We may need to transfer some DOM state such as scroll position + // for the deep clones. + // TODO: If there's a manual view-transition-name inside the clone we + // should ideally remove it from the original and then restore it in mutation + // phase. Otherwise it leads to duplicate names. + } appendChild(hostParentClone, clone); - // TODO: We may need to transfer some DOM state such as scroll position - // for the deep clones. - // TODO: If there's a manual view-transition-name inside the clone we - // should ideally remove it from the original and then restore it in mutation - // phase. Otherwise it leads to duplicate names. - if (unhideHostChildren) { + if (parentViewTransition !== null) { + if (parentViewTransition.clones === null) { + parentViewTransition.clones = [clone]; + } else { + parentViewTransition.clones.push(clone); + } + } + if (visitPhase === CLONE_EXIT || visitPhase === CLONE_UNHIDE) { unhideInstance(clone, child.memoizedProps); } break; @@ -91,7 +360,7 @@ function recursivelyInsertClonesFromExistingTree( } const clone = cloneMutableTextInstance(textInstance); appendChild(hostParentClone, clone); - if (unhideHostChildren) { + if (visitPhase === CLONE_EXIT || visitPhase === CLONE_UNHIDE) { unhideTextInstance(clone, child.memoizedProps); } break; @@ -108,21 +377,52 @@ function recursivelyInsertClonesFromExistingTree( // clone invisible content. // TODO: If this is visible but detached it should still be cloned. // Since there was no mutation to this node, it couldn't have changed - // visibility so we don't need to update unhideHostChildren here. - recursivelyInsertClonesFromExistingTree(child, hostParentClone); + // visibility so we don't need to update visitPhase here. + recursivelyInsertClonesFromExistingTree( + child, + hostParentClone, + parentViewTransition, + visitPhase, + ); } break; } case ViewTransitionComponent: const prevMutationContext = pushMutationContext(); + const viewTransitionState: ViewTransitionState = child.stateNode; // TODO: If this was already cloned by a previous pass we can reuse those clones. - recursivelyInsertClonesFromExistingTree(child, hostParentClone); - // TODO: Do we need to track whether this should have a name applied? + viewTransitionState.clones = null; + let nextPhase; + if (visitPhase === CLONE_EXIT) { + // This was an Enter of a ViewTransition. We now move onto unhiding the inner + // HostComponents and finding inner pairs. + nextPhase = CLONE_UNHIDE; + // TODO: Mark the name and find a pair. + } else if (visitPhase === CLONE_UPDATE) { + // If the tree had no mutations and we've found the top most ViewTransition + // then this is the one we might apply the "layout" state too if it has changed + // position. After we've found its HostComponents we can bail out. + nextPhase = CLONE_UNCHANGED; + } else { + nextPhase = visitPhase; + } + recursivelyInsertClonesFromExistingTree( + child, + hostParentClone, + viewTransitionState, + nextPhase, + ); + // TODO: Only the first level should track if this was s // child.flags |= Update; popMutationContext(prevMutationContext); break; default: { - recursivelyInsertClonesFromExistingTree(child, hostParentClone); + recursivelyInsertClonesFromExistingTree( + child, + hostParentClone, + parentViewTransition, + visitPhase, + ); break; } } @@ -133,12 +433,14 @@ function recursivelyInsertClonesFromExistingTree( function recursivelyInsertClones( parentFiber: Fiber, hostParentClone: Instance, + parentViewTransition: null | ViewTransitionState, + visitPhase: VisitPhase, ) { const deletions = parentFiber.deletions; if (deletions !== null) { for (let i = 0; i < deletions.length; i++) { - // const childToDelete = deletions[i]; - // TODO + const childToDelete = deletions[i]; + trackEnterViewTransitions(childToDelete); } } @@ -149,21 +451,45 @@ function recursivelyInsertClones( // If we have mutations or if this is a newly inserted tree, clone as we go. let child = parentFiber.child; while (child !== null) { - insertDestinationClonesOfFiber(child, hostParentClone); + insertDestinationClonesOfFiber( + child, + hostParentClone, + parentViewTransition, + visitPhase, + ); child = child.sibling; } } else { // Once we reach a subtree with no more mutations we can bail out. // However, we must still insert deep clones of the HostComponents. - recursivelyInsertClonesFromExistingTree(parentFiber, hostParentClone); + recursivelyInsertClonesFromExistingTree( + parentFiber, + hostParentClone, + parentViewTransition, + visitPhase, + ); } } function insertDestinationClonesOfFiber( finishedWork: Fiber, hostParentClone: Instance, + parentViewTransition: null | ViewTransitionState, + visitPhase: VisitPhase, ) { const current = finishedWork.alternate; + if (current === null) { + // This is a newly mounted subtree. Insert any HostComponents and trigger + // Enter transitions. + recursivelyInsertNewFiber( + finishedWork, + hostParentClone, + parentViewTransition, + INSERT_EXIT, + ); + return; + } + const flags = finishedWork.flags; // The effect flag should be checked *after* we refine the type of fiber, // because the fiber tag is more specific. An exception is any flag related @@ -172,55 +498,55 @@ function insertDestinationClonesOfFiber( case HostHoistable: { if (supportsResources) { // TODO: Hoistables should get optimistically inserted and then removed. - recursivelyInsertClones(finishedWork, hostParentClone); + recursivelyInsertClones( + finishedWork, + hostParentClone, + parentViewTransition, + visitPhase, + ); break; } // Fall through } case HostSingleton: { if (supportsSingletons) { - recursivelyInsertClones(finishedWork, hostParentClone); + recursivelyInsertClones( + finishedWork, + hostParentClone, + parentViewTransition, + visitPhase, + ); if (__DEV__) { // We cannot apply mutations to Host Singletons since by definition // they cannot be cloned. Therefore we warn in DEV if this commit // had any effect. if (flags & Update) { - if (current === null) { - console.error( - 'useSwipeTransition() caused something to render a new <%s>. ' + - 'This is not possible in the current implementation. ' + - "Make sure that the swipe doesn't mount any new <%s> elements.", - finishedWork.type, - finishedWork.type, - ); - } else { - const newProps = finishedWork.memoizedProps; - const oldProps = current.memoizedProps; - const instance = finishedWork.stateNode; - const type = finishedWork.type; - const prev = pushMutationContext(); + const newProps = finishedWork.memoizedProps; + const oldProps = current.memoizedProps; + const instance = finishedWork.stateNode; + const type = finishedWork.type; + const prev = pushMutationContext(); - try { - // Since we currently don't have a separate diffing algorithm for - // individual properties, the Update flag can be a false positive. - // We have to apply the new props first o detect any mutations and - // then revert them. - commitUpdate(instance, type, oldProps, newProps, finishedWork); - if (viewTransitionMutationContext) { - console.error( - 'useSwipeTransition() caused something to mutate <%s>. ' + - 'This is not possible in the current implementation. ' + - "Make sure that the swipe doesn't update any state which " + - 'causes <%s> to change.', - finishedWork.type, - finishedWork.type, - ); - } - // Revert - commitUpdate(instance, type, newProps, oldProps, finishedWork); - } finally { - popMutationContext(prev); + try { + // Since we currently don't have a separate diffing algorithm for + // individual properties, the Update flag can be a false positive. + // We have to apply the new props first o detect any mutations and + // then revert them. + commitUpdate(instance, type, oldProps, newProps, finishedWork); + if (viewTransitionMutationContext) { + console.error( + 'useSwipeTransition() caused something to mutate <%s>. ' + + 'This is not possible in the current implementation. ' + + "Make sure that the swipe doesn't update any state which " + + 'causes <%s> to change.', + finishedWork.type, + finishedWork.type, + ); } + // Revert + commitUpdate(instance, type, newProps, oldProps, finishedWork); + } finally { + popMutationContext(prev); } } } @@ -230,42 +556,46 @@ function insertDestinationClonesOfFiber( } case HostComponent: { const instance: Instance = finishedWork.stateNode; - if (current === null) { - // For insertions we don't need to clone. It's already new state node. - // TODO: Do we need to visit it for ViewTransitions though? - appendChild(hostParentClone, instance); + let clone: Instance; + if (finishedWork.child === null) { + // This node is terminal. We still do a deep clone in case this has user + // inserted content, text content or dangerouslySetInnerHTML. + clone = cloneMutableInstance(instance, true); + if (finishedWork.flags & ContentReset) { + resetTextContent(clone); + } } else { - let clone: Instance; - if (finishedWork.child === null) { - // This node is terminal. We still do a deep clone in case this has user - // inserted content, text content or dangerouslySetInnerHTML. - clone = cloneMutableInstance(instance, true); - if (finishedWork.flags & ContentReset) { - resetTextContent(clone); - } - } else { - // If we have children we'll clone them as we walk the tree so we just - // do a shallow clone here. - clone = cloneMutableInstance(instance, false); - } + // If we have children we'll clone them as we walk the tree so we just + // do a shallow clone here. + clone = cloneMutableInstance(instance, false); + } - if (flags & Update) { - const newProps = finishedWork.memoizedProps; - const oldProps = current.memoizedProps; - const type = finishedWork.type; - // Apply the delta to the clone. - commitUpdate(clone, type, oldProps, newProps, finishedWork); - } + if (flags & Update) { + const newProps = finishedWork.memoizedProps; + const oldProps = current.memoizedProps; + const type = finishedWork.type; + // Apply the delta to the clone. + commitUpdate(clone, type, oldProps, newProps, finishedWork); + } - if (unhideHostChildren) { - unhideHostChildren = false; - recursivelyInsertClones(finishedWork, clone); - appendChild(hostParentClone, clone); - unhideHostChildren = true; - unhideInstance(clone, finishedWork.memoizedProps); + if (visitPhase === CLONE_EXIT || visitPhase === CLONE_UNHIDE) { + recursivelyInsertClones( + finishedWork, + clone, + null, + CLONE_APPEARING_PAIR, + ); + appendChild(hostParentClone, clone); + unhideInstance(clone, finishedWork.memoizedProps); + } else { + recursivelyInsertClones(finishedWork, clone, null, visitPhase); + appendChild(hostParentClone, clone); + } + if (parentViewTransition !== null) { + if (parentViewTransition.clones === null) { + parentViewTransition.clones = [clone]; } else { - recursivelyInsertClones(finishedWork, clone); - appendChild(hostParentClone, clone); + parentViewTransition.clones.push(clone); } } break; @@ -278,20 +608,15 @@ function insertDestinationClonesOfFiber( 'caused by a bug in React. Please file an issue.', ); } - if (current === null) { - // For insertions we don't need to clone. It's already new state node. - appendChild(hostParentClone, textInstance); - } else { - const clone = cloneMutableTextInstance(textInstance); - if (flags & Update) { - const newText: string = finishedWork.memoizedProps; - const oldText: string = current.memoizedProps; - commitTextUpdate(clone, newText, oldText); - } - appendChild(hostParentClone, clone); - if (unhideHostChildren) { - unhideTextInstance(clone, finishedWork.memoizedProps); - } + const clone = cloneMutableTextInstance(textInstance); + if (flags & Update) { + const newText: string = finishedWork.memoizedProps; + const oldText: string = current.memoizedProps; + commitTextUpdate(clone, newText, oldText); + } + appendChild(hostParentClone, clone); + if (visitPhase === CLONE_EXIT || visitPhase === CLONE_UNHIDE) { + unhideTextInstance(clone, finishedWork.memoizedProps); } break; } @@ -306,17 +631,45 @@ function insertDestinationClonesOfFiber( // Only insert clones if this tree is going to be visible. No need to // clone invisible content. // TODO: If this is visible but detached it should still be cloned. - const prevUnhide = unhideHostChildren; - unhideHostChildren = prevUnhide || (flags & Visibility) !== NoFlags; - recursivelyInsertClones(finishedWork, hostParentClone); - unhideHostChildren = prevUnhide; + let nextPhase; + if (visitPhase === CLONE_UPDATE && (flags & Visibility) !== NoFlags) { + // This is the root of an appear. We need to trigger Enter transitions. + nextPhase = CLONE_EXIT; + } else { + nextPhase = visitPhase; + } + recursivelyInsertClones( + finishedWork, + hostParentClone, + parentViewTransition, + nextPhase, + ); + } else if (current !== null && current.memoizedState === null) { + // Was previously mounted as visible but is now hidden. + trackEnterViewTransitions(current); } break; } case ViewTransitionComponent: const prevMutationContext = pushMutationContext(); + const viewTransitionState: ViewTransitionState = finishedWork.stateNode; // TODO: If this was already cloned by a previous pass we can reuse those clones. - recursivelyInsertClones(finishedWork, hostParentClone); + viewTransitionState.clones = null; + let nextPhase; + if (visitPhase === CLONE_EXIT) { + // This was an Enter of a ViewTransition. We now move onto unhiding the inner + // HostComponents and finding inner pairs. + nextPhase = CLONE_UNHIDE; + // TODO: Mark the name and find a pair. + } else { + nextPhase = visitPhase; + } + recursivelyInsertClones( + finishedWork, + hostParentClone, + viewTransitionState, + nextPhase, + ); if (viewTransitionMutationContext) { // Track that this boundary had a mutation and therefore needs to animate // whether it resized or not. @@ -325,7 +678,12 @@ function insertDestinationClonesOfFiber( popMutationContext(prevMutationContext); break; default: { - recursivelyInsertClones(finishedWork, hostParentClone); + recursivelyInsertClones( + finishedWork, + hostParentClone, + parentViewTransition, + visitPhase, + ); break; } } @@ -337,7 +695,6 @@ export function insertDestinationClones( root: FiberRoot, finishedWork: Fiber, ): void { - unhideHostChildren = false; // We'll either not transition the root, or we'll transition the clone. Regardless // we cancel the root view transition name. const needsClone = detectMutationOrInsertClones(finishedWork); @@ -356,24 +713,274 @@ export function insertDestinationClones( // Clone the whole root const rootClone = cloneRootViewTransitionContainer(root.containerInfo); root.gestureClone = rootClone; - recursivelyInsertClones(finishedWork, rootClone); + recursivelyInsertClones(finishedWork, rootClone, null, CLONE_UPDATE); } else { root.gestureClone = null; cancelRootViewTransitionName(root.containerInfo); } } +function applyDeletedPairViewTransitions(deletion: Fiber): void { + if ((deletion.subtreeFlags & ViewTransitionNamedStatic) === NoFlags) { + // This has no named view transitions in its subtree. + return; + } + let child = deletion.child; + while (child !== null) { + if (child.tag === OffscreenComponent && child.memoizedState === null) { + // This tree was already hidden so we skip it. + } else { + if ( + child.tag === ViewTransitionComponent && + (child.flags & ViewTransitionNamedStatic) !== NoFlags + ) { + const props: ViewTransitionProps = child.memoizedProps; + const name = props.name; + if (name != null && name !== 'auto') { + // TODO: Find a pair + } + } + applyDeletedPairViewTransitions(child); + } + child = child.sibling; + } +} + +function applyEnterViewTransitions(deletion: Fiber): void { + if (deletion.tag === ViewTransitionComponent) { + const props: ViewTransitionProps = deletion.memoizedProps; + const name = props.name; + if (name != null && name !== 'auto') { + // TODO: Find a pair + } + // Look for more pairs deeper in the tree. + applyDeletedPairViewTransitions(deletion); + } else if ((deletion.subtreeFlags & ViewTransitionStatic) !== NoFlags) { + // TODO: Check if this is a hidden Offscreen or a Portal. + let child = deletion.child; + while (child !== null) { + applyEnterViewTransitions(child); + child = child.sibling; + } + } else { + applyDeletedPairViewTransitions(deletion); + } +} + +function measureExitViewTransitions(placement: Fiber): void { + if (placement.tag === ViewTransitionComponent) { + // const state: ViewTransitionState = placement.stateNode; + const props: ViewTransitionProps = placement.memoizedProps; + const name = props.name; + if (name != null && name !== 'auto') { + // TODO: Find a pair + } + } else if ((placement.subtreeFlags & ViewTransitionStatic) !== NoFlags) { + // TODO: Check if this is a hidden Offscreen or a Portal. + let child = placement.child; + while (child !== null) { + measureExitViewTransitions(child); + child = child.sibling; + } + } else { + // We don't need to find pairs here because we would've already found and + // measured the pairs inside the deletion phase. + } +} + +function measureNestedViewTransitions(changedParent: Fiber): void { + let child = changedParent.child; + while (child !== null) { + if (child.tag === ViewTransitionComponent) { + const current = child.alternate; + if (current !== null) { + // const props: ViewTransitionProps = child.memoizedProps; + // const name = getViewTransitionName(props, child.stateNode); + // TODO: Measure both the old and new state and see if they're different. + } + } else if ((child.subtreeFlags & ViewTransitionStatic) !== NoFlags) { + // TODO: Check if this is a hidden Offscreen or a Portal. + measureNestedViewTransitions(child); + } + child = child.sibling; + } +} + +function measureUpdateViewTransition( + current: Fiber, + finishedWork: Fiber, +): void { + // TODO +} + +function recursivelyApplyViewTransitions(parentFiber: Fiber) { + const deletions = parentFiber.deletions; + if (deletions !== null) { + for (let i = 0; i < deletions.length; i++) { + const childToDelete = deletions[i]; + applyEnterViewTransitions(childToDelete); + } + } + + if ( + parentFiber.alternate === null || + (parentFiber.subtreeFlags & MutationMask) !== NoFlags + ) { + // If we have mutations or if this is a newly inserted tree, clone as we go. + let child = parentFiber.child; + while (child !== null) { + applyViewTransitionsOnFiber(child); + child = child.sibling; + } + } else { + // Nothing has changed in this subtree, but the parent may have still affected + // its size and position. We need to measure the old and new state to see if + // we should animate its size and position. + measureNestedViewTransitions(parentFiber); + } +} + +function applyViewTransitionsOnFiber(finishedWork: Fiber) { + const current = finishedWork.alternate; + if (current === null) { + measureExitViewTransitions(finishedWork); + return; + } + + const flags = finishedWork.flags; + // The effect flag should be checked *after* we refine the type of fiber, + // because the fiber tag is more specific. An exception is any flag related + // to reconciliation, because those can be set on all fiber types. + switch (finishedWork.tag) { + case HostComponent: { + // const instance: Instance = finishedWork.stateNode; + // TODO: Apply name and measure. + recursivelyApplyViewTransitions(finishedWork); + break; + } + case HostText: { + break; + } + case HostPortal: { + // TODO: Consider what should happen to Portals. For now we exclude them. + break; + } + case OffscreenComponent: { + if (flags & Visibility) { + const newState: OffscreenState | null = finishedWork.memoizedState; + const isHidden = newState !== null; + if (!isHidden) { + measureExitViewTransitions(finishedWork); + } else if (current !== null && current.memoizedState === null) { + // Was previously mounted as visible but is now hidden. + applyEnterViewTransitions(current); + } + } + break; + } + case ViewTransitionComponent: + measureUpdateViewTransition(current, finishedWork); + const viewTransitionState: ViewTransitionState = finishedWork.stateNode; + viewTransitionState.clones = null; // Reset + recursivelyApplyViewTransitions(finishedWork); + break; + default: { + recursivelyApplyViewTransitions(finishedWork); + break; + } + } +} + // Revert insertions and apply view transition names to the "new" (current) state. export function applyDepartureTransitions( root: FiberRoot, finishedWork: Fiber, ): void { + // First measure and apply view-transition-names to the "new" states. + recursivelyApplyViewTransitions(finishedWork); + // Then remove the clones. const rootClone = root.gestureClone; if (rootClone !== null) { root.gestureClone = null; removeRootViewTransitionClone(root.containerInfo, rootClone); } - // TODO +} + +function recursivelyRestoreViewTransitions(parentFiber: Fiber) { + const deletions = parentFiber.deletions; + if (deletions !== null) { + for (let i = 0; i < deletions.length; i++) { + const childToDelete = deletions[i]; + restoreEnterOrExitViewTransitions(childToDelete); + } + } + + if ( + parentFiber.alternate === null || + (parentFiber.subtreeFlags & MutationMask) !== NoFlags + ) { + // If we have mutations or if this is a newly inserted tree, clone as we go. + let child = parentFiber.child; + while (child !== null) { + restoreViewTransitionsOnFiber(child); + child = child.sibling; + } + } else { + // Nothing has changed in this subtree, but the parent may have still affected + // its size and position. We need to measure the old and new state to see if + // we should animate its size and position. + restoreNestedViewTransitions(parentFiber); + } +} + +function restoreViewTransitionsOnFiber(finishedWork: Fiber) { + const current = finishedWork.alternate; + if (current === null) { + restoreEnterOrExitViewTransitions(finishedWork); + return; + } + + const flags = finishedWork.flags; + // The effect flag should be checked *after* we refine the type of fiber, + // because the fiber tag is more specific. An exception is any flag related + // to reconciliation, because those can be set on all fiber types. + switch (finishedWork.tag) { + case HostComponent: { + // const instance: Instance = finishedWork.stateNode; + // TODO: Restore the name. + recursivelyRestoreViewTransitions(finishedWork); + break; + } + case HostText: { + break; + } + case HostPortal: { + // TODO: Consider what should happen to Portals. For now we exclude them. + break; + } + case OffscreenComponent: { + if (flags & Visibility) { + const newState: OffscreenState | null = finishedWork.memoizedState; + const isHidden = newState !== null; + if (!isHidden) { + restoreEnterOrExitViewTransitions(finishedWork); + } else if (current !== null && current.memoizedState === null) { + // Was previously mounted as visible but is now hidden. + restoreEnterOrExitViewTransitions(current); + } + } + break; + } + case ViewTransitionComponent: + const viewTransitionState: ViewTransitionState = finishedWork.stateNode; + viewTransitionState.clones = null; // Reset + recursivelyRestoreViewTransitions(finishedWork); + break; + default: { + recursivelyRestoreViewTransitions(finishedWork); + break; + } + } } // Revert transition names and start/adjust animations on the started View Transition. @@ -381,6 +988,6 @@ export function startGestureAnimations( root: FiberRoot, finishedWork: Fiber, ): void { - // TODO + restoreViewTransitionsOnFiber(finishedWork); restoreRootViewTransitionName(root.containerInfo); } diff --git a/packages/react-reconciler/src/ReactFiberViewTransitionComponent.js b/packages/react-reconciler/src/ReactFiberViewTransitionComponent.js index 16b5ef3049..ecacf2a439 100644 --- a/packages/react-reconciler/src/ReactFiberViewTransitionComponent.js +++ b/packages/react-reconciler/src/ReactFiberViewTransitionComponent.js @@ -9,7 +9,7 @@ import type {ReactNodeList} from 'shared/ReactTypes'; import type {FiberRoot} from './ReactInternalTypes'; -import type {ViewTransitionInstance} from './ReactFiberConfig'; +import type {ViewTransitionInstance, Instance} from './ReactFiberConfig'; import { getWorkInProgressRoot, @@ -45,6 +45,7 @@ export type ViewTransitionProps = { export type ViewTransitionState = { autoName: null | string, // the view-transition-name to use when an explicit one is not specified paired: null | ViewTransitionState, // a temporary state during the commit phase if we have paired this with another instance + clones: null | Array, // a temporary state during the apply gesture phase if we cloned this boundary ref: null | ViewTransitionInstance, // the current ref instance. This can change through the lifetime of the instance. }; From cc03226eabcbf6e4ebc28a9c25e439b0302d2016 Mon Sep 17 00:00:00 2001 From: Mofei Zhang Date: Fri, 14 Mar 2025 13:38:01 -0400 Subject: [PATCH 12/12] [compiler] (needs cleanup) Hoist dependencies from functions more conservatively Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: --- .../src/HIR/CollectHoistablePropertyLoads.ts | 228 +++++++++++++++--- ...ap-operand-conditionally-invoked.expect.md | 53 ++++ ...array-map-operand-conditionally-invoked.js | 9 + .../bug-aliased-capture-mutate.expect.md | 4 +- ...nction-expression-prototype-call.expect.md | 4 +- ...be-invalid-useMemo-read-maybeRef.expect.md | 32 --- ...be-invalid-useMemo-read-maybeRef.expect.md | 40 +++ ...=> maybe-invalid-useMemo-read-maybeRef.ts} | 0 ...function-uncond-access-local-var.expect.md | 4 +- ...nfer-object-method-uncond-access.expect.md | 4 +- ...l-dependency-on-context-variable.expect.md | 4 +- .../context-var-granular-dep.expect.md | 4 +- ...e-variables-nested-object-method.expect.md | 4 +- .../useEffect-nested-lambdas.expect.md | 25 +- 14 files changed, 324 insertions(+), 91 deletions(-) create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-map-operand-conditionally-invoked.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-map-operand-conditionally-invoked.js delete mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.maybe-invalid-useMemo-read-maybeRef.expect.md create mode 100644 compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/maybe-invalid-useMemo-read-maybeRef.expect.md rename compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/{error.maybe-invalid-useMemo-read-maybeRef.ts => maybe-invalid-useMemo-read-maybeRef.ts} (100%) diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts index 7b35269686..b8e4760cc9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/CollectHoistablePropertyLoads.ts @@ -7,21 +7,27 @@ import { Set_intersect, Set_union, getOrInsertDefault, + getOrInsertWith, } from '../Utils/utils'; import { BasicBlock, BlockId, DependencyPathEntry, GeneratedSource, + getHookKind, HIRFunction, Identifier, IdentifierId, InstructionId, InstructionValue, + LoweredFunction, + Place, PropertyLiteral, ReactiveScopeDependency, ScopeId, + ValidatedIdentifier, } from './HIR'; +import {eachInstructionOperand, eachInstructionValueOperand} from './visitors'; const DEBUG_PRINT = false; @@ -112,6 +118,9 @@ export function collectHoistablePropertyLoads( hoistableFromOptionals, registry, nestedFnImmutableContext: null, + fnUsages: fn.env.config.enableTreatFunctionDepsAsConditional + ? new Map() + : mapFunctionExpressionsToEscapingBlocks(fn), }); } @@ -127,6 +136,11 @@ type CollectHoistablePropertyLoadsContext = { * but are currently kept separate for readability. */ nestedFnImmutableContext: ReadonlySet | null; + /** + * Mapping of functions declared within a traversal context to their + * (valid) usage sites, which will have hoistable property loads added + */ + fnUsages: ReadonlyMap>; }; function collectHoistablePropertyLoadsImpl( fn: HIRFunction, @@ -338,7 +352,13 @@ function collectNonNullsInBlocks( context.registry.getOrCreateIdentifier(identifier), ); } - const nodes = new Map(); + const nodes = new Map< + BlockId, + { + block: BasicBlock; + assumedNonNullObjects: Set; + } + >(); for (const [_, block] of fn.body.blocks) { const assumedNonNullObjects = new Set( knownNonNullIdentifiers, @@ -358,40 +378,68 @@ function collectNonNullsInBlocks( ) { assumedNonNullObjects.add(maybeNonNull); } - if ( - (instr.value.kind === 'FunctionExpression' || - instr.value.kind === 'ObjectMethod') && - !fn.env.config.enableTreatFunctionDepsAsConditional - ) { + if (instr.value.kind === 'FunctionExpression') { + /** + * What are reasonable semantics here? + * Risky + * - only treat named fns as unconditionally hoistable + * + * Conservative + */ const innerFn = instr.value.loweredFunc; - const innerHoistableMap = collectHoistablePropertyLoadsImpl( - innerFn.func, - { - ...context, - nestedFnImmutableContext: - context.nestedFnImmutableContext ?? - new Set( - innerFn.func.context - .filter(place => - isImmutableAtInstr(place.identifier, instr.id, context), - ) - .map(place => place.identifier.id), - ), - }, - ); - const innerHoistables = assertNonNull( - innerHoistableMap.get(innerFn.func.body.entry), - ); - for (const entry of innerHoistables.assumedNonNullObjects) { - assumedNonNullObjects.add(entry); + const resultBlocks = context.fnUsages.get(innerFn); + if (resultBlocks != null) { + const innerHoistableMap = collectHoistablePropertyLoadsImpl( + innerFn.func, + { + ...context, + nestedFnImmutableContext: + context.nestedFnImmutableContext ?? + new Set( + innerFn.func.context + .filter(place => + isImmutableAtInstr(place.identifier, instr.id, context), + ) + .map(place => place.identifier.id), + ), + fnUsages: fn.env.config.enableTreatFunctionDepsAsConditional + ? new Map() + : mapFunctionExpressionsToEscapingBlocks(innerFn.func), + }, + ); + const innerHoistables = assertNonNull( + innerHoistableMap.get(innerFn.func.body.entry), + ); + for (const innerBlock of resultBlocks) { + let innerNonNulls; + if (innerBlock === block.id) { + innerNonNulls = assumedNonNullObjects; + } else { + innerNonNulls = getOrInsertWith(nodes, innerBlock, () => ({ + block: assertNonNull(fn.body.blocks.get(innerBlock)), + assumedNonNullObjects: new Set(), + })).assumedNonNullObjects; + } + for (const entry of innerHoistables.assumedNonNullObjects) { + innerNonNulls.add(entry); + } + } } } } - nodes.set(block.id, { - block, - assumedNonNullObjects, - }); + const maybeNode = nodes.get(block.id); + if (maybeNode != null) { + // merge + for (const entry of assumedNonNullObjects) { + maybeNode.assumedNonNullObjects.add(entry); + } + } else { + nodes.set(block.id, { + block, + assumedNonNullObjects, + }); + } } return nodes; } @@ -591,3 +639,123 @@ function reduceMaybeOptionalChains( } } while (changed); } + +/** + * + * const foo = function() { ... } // this matches + * arr.map(function() { ... }) // this does not match + * + * What about function expressions that just escape to other functions? + * + * For both below examples, cb1 should be hoistable only to if-cond block + * ```js + * function useFoo(...) { + * const cb1 = function() { ... }; + * const cb2 = function() { if (cond) cb1() }; + * return cb2; + * } + * ``` + * ```js + * function useFoo(...) { + * const cb1 = function() { ... }; + * const cb2 = function() { if (cond) return cb1; }; + * return cb2; + * } + * ``` + */ +function mapFunctionExpressionsToEscapingBlocks( + fn: HIRFunction, +): ReadonlyMap> { + /** + * Step 1: gather all function expressions and known ssa'd aliases + */ + const temporaries = new Map< + IdentifierId, + {fn: LoweredFunction; usage: Set} + >(); + const validUsages = new Set(); + + for (const block of fn.body.blocks.values()) { + for (const {lvalue, value} of block.instructions) { + /** + * Only match function expressions which can have guaranteed ssa. + */ + if (value.kind === 'FunctionExpression') { + temporaries.set(lvalue.identifier.id, { + fn: value.loweredFunc, + usage: new Set(), + }); + } else if (value.kind === 'StoreLocal') { + const lvalue = value.lvalue.place.identifier; + const maybeLoweredFunc = temporaries.get(value.value.identifier.id); + if ( + lvalue.name != null && + lvalue.name.kind === 'named' && + maybeLoweredFunc != null + ) { + temporaries.set(lvalue.id, maybeLoweredFunc); + validUsages.add(value.value); + } + } else if (value.kind === 'LoadLocal') { + const maybeLoweredFunc = temporaries.get(value.place.identifier.id); + if (maybeLoweredFunc != null) { + temporaries.set(lvalue.identifier.id, maybeLoweredFunc); + validUsages.add(value.place); + } + } + } + } + /** + * Step 2: Forward pass to do best-effort "escape analysis" + */ + for (const block of fn.body.blocks.values()) { + for (const {value} of block.instructions) { + if (value.kind === 'CallExpression') { + const callee = value.callee; + const maybeHook = getHookKind(fn.env, callee.identifier); + const maybeLoweredFunc = temporaries.get(callee.identifier.id); + if (maybeLoweredFunc != null) { + // Direct calls + maybeLoweredFunc.usage.add(block.id); + } else if (maybeHook != null) { + // Arguments to hooks + for (const arg of value.args.filter( + arg => arg.kind === 'Identifier', + ) as Array) { + const maybeLoweredFunc = temporaries.get(arg.identifier.id); + if (maybeLoweredFunc != null) { + maybeLoweredFunc.usage.add(block.id); + } + } + } + } else if (value.kind === 'JsxExpression') { + /* Match jsx attributes */ + for (const attr of value.props) { + if (attr.kind === 'JsxSpreadAttribute') { + continue; + } + const maybeLoweredFunc = temporaries.get(attr.place.identifier.id); + if (maybeLoweredFunc != null) { + maybeLoweredFunc.usage.add(block.id); + } + } + } + if (block.terminal.kind === 'return') { + const maybeLoweredFunc = temporaries.get( + block.terminal.value.identifier.id, + ); + if (maybeLoweredFunc != null) { + maybeLoweredFunc.usage.add(block.id); + } + } + } + } + + const map = new Map>(); + for (const {fn, usage} of temporaries.values()) { + if (!map.has(fn)) { + map.set(fn, usage); + } + } + return map; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-map-operand-conditionally-invoked.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-map-operand-conditionally-invoked.expect.md new file mode 100644 index 0000000000..8df552c108 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-map-operand-conditionally-invoked.expect.md @@ -0,0 +1,53 @@ + +## Input + +```javascript +function useFoo({arr}) { + return arr.map(e => arr[0].value + e.value); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{arr: []}], + sequentialRenders: [{arr: []}, {arr: [{value: 1}, {value: 2}]}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function useFoo(t0) { + const $ = _c(4); + const { arr } = t0; + let t1; + if ($[0] !== arr) { + let t2; + if ($[2] !== arr[0]) { + t2 = (e) => arr[0].value + e.value; + $[2] = arr[0]; + $[3] = t2; + } else { + t2 = $[3]; + } + t1 = arr.map(t2); + $[0] = arr; + $[1] = t1; + } else { + t1 = $[1]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{ arr: [] }], + sequentialRenders: [{ arr: [] }, { arr: [{ value: 1 }, { value: 2 }] }], +}; + +``` + +### Eval output +(kind: ok) [] +[2,3] \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-map-operand-conditionally-invoked.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-map-operand-conditionally-invoked.js new file mode 100644 index 0000000000..e7d5f59046 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/array-map-operand-conditionally-invoked.js @@ -0,0 +1,9 @@ +function useFoo({arr}) { + return arr.map(e => arr[0].value + e.value); +} + +export const FIXTURE_ENTRYPOINT = { + fn: useFoo, + params: [{arr: []}], + sequentialRenders: [{arr: []}, {arr: [{value: 1}, {value: 2}]}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-mutate.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-mutate.expect.md index c35efe6a16..ae60114253 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-mutate.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/bug-aliased-capture-mutate.expect.md @@ -56,9 +56,9 @@ function useFoo(t0) { setPropertyByKey(obj, "arr", arr); const obj_alias = obj; let t2; - if ($[2] !== obj_alias.arr.length) { + if ($[2] !== obj_alias) { t2 = () => obj_alias.arr.length; - $[2] = obj_alias.arr.length; + $[2] = obj_alias; $[3] = t2; } else { t2 = $[3]; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/function-expression-prototype-call.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/function-expression-prototype-call.expect.md index 5666876f00..2df5b90890 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/function-expression-prototype-call.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/function-expression-prototype-call.expect.md @@ -23,11 +23,11 @@ import { c as _c } from "react/compiler-runtime"; function Component(props) { const $ = _c(4); let t0; - if ($[0] !== props.name) { + if ($[0] !== props) { t0 = function () { return
{props.name}
; }; - $[0] = props.name; + $[0] = props; $[1] = t0; } else { t0 = $[1]; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.maybe-invalid-useMemo-read-maybeRef.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.maybe-invalid-useMemo-read-maybeRef.expect.md deleted file mode 100644 index ce0b751851..0000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.maybe-invalid-useMemo-read-maybeRef.expect.md +++ /dev/null @@ -1,32 +0,0 @@ - -## Input - -```javascript -// @validatePreserveExistingMemoizationGuarantees -import {useMemo} from 'react'; - -function useHook(maybeRef, shouldRead) { - return useMemo(() => { - return () => [maybeRef.current]; - }, [shouldRead, maybeRef]); -} - -``` - - -## Error - -``` - 3 | - 4 | function useHook(maybeRef, shouldRead) { -> 5 | return useMemo(() => { - | ^^^^^^^ -> 6 | return () => [maybeRef.current]; - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ -> 7 | }, [shouldRead, maybeRef]); - | ^^^^ CannotPreserveMemoization: React Compiler has skipped optimizing this component because the existing manual memoization could not be preserved. The inferred dependencies did not match the manually specified dependencies, which could cause the value to change more or less frequently than expected (5:7) - 8 | } - 9 | -``` - - \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/maybe-invalid-useMemo-read-maybeRef.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/maybe-invalid-useMemo-read-maybeRef.expect.md new file mode 100644 index 0000000000..b65fb89782 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/maybe-invalid-useMemo-read-maybeRef.expect.md @@ -0,0 +1,40 @@ + +## Input + +```javascript +// @validatePreserveExistingMemoizationGuarantees +import {useMemo} from 'react'; + +function useHook(maybeRef, shouldRead) { + return useMemo(() => { + return () => [maybeRef.current]; + }, [shouldRead, maybeRef]); +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; // @validatePreserveExistingMemoizationGuarantees +import { useMemo } from "react"; + +function useHook(maybeRef, shouldRead) { + const $ = _c(2); + let t0; + let t1; + if ($[0] !== maybeRef) { + t1 = () => [maybeRef.current]; + $[0] = maybeRef; + $[1] = t1; + } else { + t1 = $[1]; + } + t0 = t1; + return t0; +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.maybe-invalid-useMemo-read-maybeRef.ts b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/maybe-invalid-useMemo-read-maybeRef.ts similarity index 100% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/error.maybe-invalid-useMemo-read-maybeRef.ts rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/preserve-memo-validation/maybe-invalid-useMemo-read-maybeRef.ts diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-nested-function-uncond-access-local-var.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-nested-function-uncond-access-local-var.expect.md index ca65ce72bc..53d3d04531 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-nested-function-uncond-access-local-var.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-nested-function-uncond-access-local-var.expect.md @@ -41,9 +41,9 @@ function useFoo(t0) { local = $[1]; } let t1; - if ($[2] !== local.b.c) { + if ($[2] !== local) { t1 = () => [() => local.b.c]; - $[2] = local.b.c; + $[2] = local; $[3] = t1; } else { t1 = $[3]; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-object-method-uncond-access.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-object-method-uncond-access.expect.md index 7d75470550..f8a8af1fd4 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-object-method-uncond-access.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/propagate-scope-deps-hir-fork/reduce-reactive-deps/infer-object-method-uncond-access.expect.md @@ -34,13 +34,13 @@ function useFoo(t0) { const $ = _c(4); const { a } = t0; let t1; - if ($[0] !== a.b.c) { + if ($[0] !== a) { t1 = { fn() { return identity(a.b.c); }, }; - $[0] = a.b.c; + $[0] = a; $[1] = t1; } else { t1 = $[1]; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-control-dependency-on-context-variable.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-control-dependency-on-context-variable.expect.md index ceaa350012..963024e887 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-control-dependency-on-context-variable.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reactive-control-dependency-on-context-variable.expect.md @@ -51,7 +51,7 @@ import { identity } from "shared-runtime"; function Component(props) { const $ = _c(4); let x; - if ($[0] !== props.cond) { + if ($[0] !== props) { const f = () => { if (props.cond) { x = 1; @@ -62,7 +62,7 @@ function Component(props) { const f2 = identity(f); f2(); - $[0] = props.cond; + $[0] = props; $[1] = x; } else { x = $[1]; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/context-var-granular-dep.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/context-var-granular-dep.expect.md index d72f34b4fd..f887870197 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/context-var-granular-dep.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/reduce-reactive-deps/context-var-granular-dep.expect.md @@ -82,9 +82,9 @@ function Component(t0) { contextVar = $[2]; } let t1; - if ($[3] !== contextVar.val) { + if ($[3] !== contextVar) { t1 = { cb: () => contextVar.val * 4 }; - $[3] = contextVar.val; + $[3] = contextVar; $[4] = t1; } else { t1 = $[4]; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rename-source-variables-nested-object-method.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rename-source-variables-nested-object-method.expect.md index d0f3d5dcfe..e406f3a7d7 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rename-source-variables-nested-object-method.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/rename-source-variables-nested-object-method.expect.md @@ -43,7 +43,7 @@ const t0 = "module_t0"; const c_0 = "module_c_0"; function useFoo(props) { const $0 = _c(2); - const c_00 = $0[0] !== props.value; + const c_00 = $0[0] !== props; let t1; if (c_00) { const a = { @@ -61,7 +61,7 @@ function useFoo(props) { }; t1 = a.foo().bar(); - $0[0] = props.value; + $0[0] = props; $0[1] = t1; } else { t1 = $0[1]; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect-nested-lambdas.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect-nested-lambdas.expect.md index c3e115fa0d..0cce42e97a 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect-nested-lambdas.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useEffect-nested-lambdas.expect.md @@ -35,7 +35,7 @@ function Component(props) { import { c as _c } from "react/compiler-runtime"; // @enableTransitivelyFreezeFunctionExpressions:false function Component(props) { - const $ = _c(9); + const $ = _c(7); const item = useMutable(props.itemId); const dispatch = useDispatch(); useFreeze(dispatch); @@ -51,7 +51,8 @@ function Component(props) { } const exit = t0; let t1; - if ($[2] !== exit || $[3] !== item.value) { + let t2; + if ($[2] !== exit || $[3] !== item) { t1 = () => { const cleanup = GlobalEventEmitter.addListener("onInput", () => { if (item.value) { @@ -60,30 +61,24 @@ function Component(props) { }); return () => cleanup.remove(); }; + t2 = [exit, item]; $[2] = exit; - $[3] = item.value; + $[3] = item; $[4] = t1; + $[5] = t2; } else { t1 = $[4]; - } - let t2; - if ($[5] !== exit || $[6] !== item) { - t2 = [exit, item]; - $[5] = exit; - $[6] = item; - $[7] = t2; - } else { - t2 = $[7]; + t2 = $[5]; } useEffect(t1, t2); maybeMutate(item); let t3; - if ($[8] === Symbol.for("react.memo_cache_sentinel")) { + if ($[6] === Symbol.for("react.memo_cache_sentinel")) { t3 =
; - $[8] = t3; + $[6] = t3; } else { - t3 = $[8]; + t3 = $[6]; } return t3; }