From 6630c2de2a578aba0b61cb2e61e58a1466e80ca2 Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 4 Jan 2021 06:32:03 -0800 Subject: [PATCH 01/16] Add rudimentary support for Cache to DevTools (#20458) --- .../__snapshots__/store-test.js.snap | 1 + .../src/__tests__/store-test.js | 1 + .../src/backend/renderer.js | 19 ++++++++++++++--- .../src/backend/types.js | 1 + .../src/app/ElementTypes/index.js | 21 +++++++++++-------- 5 files changed, 31 insertions(+), 12 deletions(-) diff --git a/packages/react-devtools-shared/src/__tests__/__snapshots__/store-test.js.snap b/packages/react-devtools-shared/src/__tests__/__snapshots__/store-test.js.snap index cc3c9b6c3cc2d..5ff48f317f479 100644 --- a/packages/react-devtools-shared/src/__tests__/__snapshots__/store-test.js.snap +++ b/packages/react-devtools-shared/src/__tests__/__snapshots__/store-test.js.snap @@ -685,4 +685,5 @@ exports[`Store should show the right display names for special component types 1 [withFoo][withBar] [Memo][withFoo][withBar] [ForwardRef][withFoo][withBar] + `; diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index 33d3bc0a072a3..d219f3cba7def 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -888,6 +888,7 @@ describe('Store', () => { + ); diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index e44b58f71009e..b942b7561b7b8 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -176,6 +176,7 @@ export function getInternalReactConstants( // Currently the version in Git is 17.0.2 (but that version has not been/may not end up being released). if (gt(version, '17.0.1')) { ReactTypeOfWork = { + CacheComponent: 24, // Experimental ClassComponent: 1, ContextConsumer: 9, ContextProvider: 10, @@ -205,6 +206,7 @@ export function getInternalReactConstants( }; } else if (gte(version, '17.0.0-alpha')) { ReactTypeOfWork = { + CacheComponent: -1, // Doesn't exist yet ClassComponent: 1, ContextConsumer: 9, ContextProvider: 10, @@ -234,6 +236,7 @@ export function getInternalReactConstants( }; } else if (gte(version, '16.6.0-beta.0')) { ReactTypeOfWork = { + CacheComponent: -1, // Doens't exist yet ClassComponent: 1, ContextConsumer: 9, ContextProvider: 10, @@ -263,6 +266,7 @@ export function getInternalReactConstants( }; } else if (gte(version, '16.4.3-alpha')) { ReactTypeOfWork = { + CacheComponent: -1, // Doens't exist yet ClassComponent: 2, ContextConsumer: 11, ContextProvider: 12, @@ -292,6 +296,7 @@ export function getInternalReactConstants( }; } else { ReactTypeOfWork = { + CacheComponent: -1, // Doens't exist yet ClassComponent: 2, ContextConsumer: 12, ContextProvider: 13, @@ -335,6 +340,7 @@ export function getInternalReactConstants( } const { + CacheComponent, ClassComponent, IncompleteClassComponent, FunctionComponent, @@ -382,6 +388,8 @@ export function getInternalReactConstants( let resolvedContext: any = null; switch (tag) { + case CacheComponent: + return 'Cache'; case ClassComponent: case IncompleteClassComponent: return getDisplayName(resolvedType); @@ -486,12 +494,13 @@ export function attach( } = getInternalReactConstants(renderer.version); const {Incomplete, NoFlags, PerformedWork, Placement} = ReactTypeOfSideEffect; const { - FunctionComponent, + CacheComponent, ClassComponent, ContextConsumer, DehydratedSuspenseComponent, - Fragment, ForwardRef, + Fragment, + FunctionComponent, HostRoot, HostPortal, HostComponent, @@ -2518,6 +2527,10 @@ export function attach( tag === ForwardRef) && (!!memoizedState || !!dependencies); + // TODO Show custom UI for Cache like we do for Suspense + // For now, just hide state data entirely since it's not meant to be inspected. + const showState = !usesHooks && tag !== CacheComponent; + const typeSymbol = getTypeSymbol(type); let canViewSource = false; @@ -2687,7 +2700,7 @@ export function attach( context, hooks, props: memoizedProps, - state: usesHooks ? null : memoizedState, + state: showState ? memoizedState : null, errors: Array.from(errors.entries()), warnings: Array.from(warnings.entries()), diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index 93ac3c7827e8b..4cecf59df2983 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -26,6 +26,7 @@ export type WorkFlags = number; export type ExpirationTime = number; export type WorkTagMap = {| + CacheComponent: WorkTag, ClassComponent: WorkTag, ContextConsumer: WorkTag, ContextProvider: WorkTag, diff --git a/packages/react-devtools-shell/src/app/ElementTypes/index.js b/packages/react-devtools-shell/src/app/ElementTypes/index.js index cc3905457a996..4b44a50a6653e 100644 --- a/packages/react-devtools-shell/src/app/ElementTypes/index.js +++ b/packages/react-devtools-shell/src/app/ElementTypes/index.js @@ -19,6 +19,7 @@ import { Profiler, StrictMode, Suspense, + unstable_Cache as Cache, } from 'react'; const Context = createContext('abc'); @@ -61,15 +62,17 @@ export default function ElementTypes() { {value => null} - Loading...}> - - - - - - - - + + Loading...}> + + + + + + + + + From beb38aba3e5617cb915a8e979245b2996e1764e8 Mon Sep 17 00:00:00 2001 From: Jaiwanth Date: Mon, 4 Jan 2021 21:08:11 +0530 Subject: [PATCH 02/16] [devtools] Bump electron version from 9.1.0 to 11.1.0 for darwin-arm64 builds (#20496) --- packages/react-devtools/package.json | 2 +- yarn.lock | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/packages/react-devtools/package.json b/packages/react-devtools/package.json index 3a22f5c622f20..ad5d429fd1841 100644 --- a/packages/react-devtools/package.json +++ b/packages/react-devtools/package.json @@ -24,7 +24,7 @@ }, "dependencies": { "cross-spawn": "^5.0.1", - "electron": "^9.1.0", + "electron": "^11.1.0", "ip": "^1.1.4", "minimist": "^1.2.3", "react-devtools-core": "4.10.1", diff --git a/yarn.lock b/yarn.lock index 7b6cb08909628..9daccaf78bb36 100644 --- a/yarn.lock +++ b/yarn.lock @@ -5413,10 +5413,10 @@ electron-to-chromium@^1.3.523: resolved "https://registry.yarnpkg.com/electron-to-chromium/-/electron-to-chromium-1.3.538.tgz#15226638ee9db5d8e74f4c860cef6078d8e1e871" integrity sha512-rlyYXLlOoZkJuvY4AJXUpP7CHRVtwZz311HPVoEO1UHo/kqDCsP1pNas0A9paZuPEiYGdLwrjllF2hs69NEaTw== -electron@^9.1.0: - version "9.1.0" - resolved "https://registry.yarnpkg.com/electron/-/electron-9.1.0.tgz#ca77600c9e4cd591298c340e013384114d3d8d05" - integrity sha512-VRAF8KX1m0py9I9sf0kw1kWfeC87mlscfFcbcRdLBsNJ44/GrJhi3+E8rKbpHUeZNQxsPaVA5Zu5Lxb6dV/scQ== +electron@^11.1.0: + version "11.1.1" + resolved "https://registry.yarnpkg.com/electron/-/electron-11.1.1.tgz#188f036f8282798398dca9513e9bb3b10213e3aa" + integrity sha512-tlbex3xosJgfileN6BAQRotevPRXB/wQIq48QeQ08tUJJrXwE72c8smsM/hbHx5eDgnbfJ2G3a60PmRjHU2NhA== dependencies: "@electron/get" "^1.0.1" "@types/node" "^12.0.12" From 27659559ebfd6b7119bfc0ff02ecb851c135020c Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Mon, 4 Jan 2021 07:46:20 -0800 Subject: [PATCH 03/16] Add useRefresh hook to react-debug-tools (#20460) --- packages/react-debug-tools/src/ReactDebugHooks.js | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/packages/react-debug-tools/src/ReactDebugHooks.js b/packages/react-debug-tools/src/ReactDebugHooks.js index ce62b5ce8b9ef..eb7a8f6d4ef14 100644 --- a/packages/react-debug-tools/src/ReactDebugHooks.js +++ b/packages/react-debug-tools/src/ReactDebugHooks.js @@ -73,6 +73,10 @@ function getPrimitiveStackCache(): Map> { Dispatcher.useState(null); Dispatcher.useReducer((s, a) => s, null); Dispatcher.useRef(null); + if (typeof Dispatcher.useCacheRefresh === 'function') { + // This type check is for Flow only. + Dispatcher.useCacheRefresh(); + } Dispatcher.useLayoutEffect(() => {}); Dispatcher.useEffect(() => {}); Dispatcher.useImperativeHandle(undefined, () => null); @@ -171,6 +175,16 @@ function useRef(initialValue: T): {|current: T|} { return ref; } +function useCacheRefresh(): () => void { + const hook = nextHook(); + hookLog.push({ + primitive: 'CacheRefresh', + stackError: new Error(), + value: hook !== null ? hook.memoizedState : function refresh() {}, + }); + return () => {}; +} + function useLayoutEffect( create: () => (() => void) | void, inputs: Array | void | null, @@ -305,6 +319,7 @@ function useOpaqueIdentifier(): OpaqueIDType | void { const Dispatcher: DispatcherType = { getCacheForType, readContext, + useCacheRefresh, useCallback, useContext, useEffect, From e8eff119e036485b74b2acb6f57045390703f6fb Mon Sep 17 00:00:00 2001 From: Christian Ruigrok Date: Mon, 11 Jan 2021 21:18:45 +0100 Subject: [PATCH 04/16] Fix ESLint crash on empty react effect hook (#20385) * Fix ESLint crash on empty react effect hook * Add layout effect to test * Improve wording in comment * Improve lint warning wording * Reword missing effect callback message --- .../ESLintRuleExhaustiveDeps-test.js | 36 +++++++++++++++++++ .../src/ExhaustiveDeps.js | 13 +++++++ 2 files changed, 49 insertions(+) diff --git a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js index 0c782dc25ae02..2a2786dca8850 100644 --- a/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js +++ b/packages/eslint-plugin-react-hooks/__tests__/ESLintRuleExhaustiveDeps-test.js @@ -1692,6 +1692,42 @@ const tests = { }, ], }, + { + code: normalizeIndent` + function MyComponent() { + useEffect() + useLayoutEffect() + useCallback() + useMemo() + } + `, + errors: [ + { + message: + 'React Hook useEffect requires an effect callback. ' + + 'Did you forget to pass a callback to the hook?', + suggestions: undefined, + }, + { + message: + 'React Hook useLayoutEffect requires an effect callback. ' + + 'Did you forget to pass a callback to the hook?', + suggestions: undefined, + }, + { + message: + 'React Hook useCallback requires an effect callback. ' + + 'Did you forget to pass a callback to the hook?', + suggestions: undefined, + }, + { + message: + 'React Hook useMemo requires an effect callback. ' + + 'Did you forget to pass a callback to the hook?', + suggestions: undefined, + }, + ], + }, { // Regression test code: normalizeIndent` diff --git a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js index 7c970f7c8f6ac..ee259b2d37cd6 100644 --- a/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js +++ b/packages/eslint-plugin-react-hooks/src/ExhaustiveDeps.js @@ -1119,6 +1119,19 @@ export default { const declaredDependenciesNode = node.arguments[callbackIndex + 1]; const isEffect = /Effect($|[^a-z])/g.test(reactiveHookName); + // Check whether a callback is supplied. If there is no callback supplied + // then the hook will not work and React will throw a TypeError. + // So no need to check for dependency inclusion. + if (!callback) { + reportProblem({ + node: reactiveHook, + message: + `React Hook ${reactiveHookName} requires an effect callback. ` + + `Did you forget to pass a callback to the hook?`, + }); + return; + } + // Check the declared dependencies for this reactive hook. If there is no // second argument then the reactive callback will re-run on every render. // So no need to check for dependency inclusion. From eb0fb382308f18460844d646dada2e564f82353d Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 12 Jan 2021 11:32:32 -0600 Subject: [PATCH 05/16] Build stable and experimental with same command (#20573) The goal is to simplify our CI pipeline so that all configurations are built and tested in a single workflow. As a first step, this adds a new build script entry point that builds both the experimental and stable release channels into a single artifacts directory. The script works by wrapping the existing build script (which only builds a single release channel at a time), then post-processing the results to match the desired filesystem layout. A future version of the build script would output the files directly without post-processing. Because many parts of our infra depend on the existing layout of the build artifacts directory, I have left the old workflows untouched. We can incremental migrate to the new layout, then delete the old workflows after we've finished. --- .circleci/config.yml | 50 ++++++++ .gitignore | 1 + package.json | 1 + scripts/rollup/build-all-release-channels.js | 123 +++++++++++++++++++ 4 files changed, 175 insertions(+) create mode 100644 scripts/rollup/build-all-release-channels.js diff --git a/.circleci/config.yml b/.circleci/config.yml index f859b2c59a309..a532fb2e067a7 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -293,6 +293,45 @@ jobs: - dist - sizes/*.json + yarn_build_combined: + docker: *docker + environment: *environment + parallelism: 40 + steps: + - checkout + - run: yarn workspaces info | head -n -1 > workspace_info.txt + - *restore_node_modules + - run: + command: | + ./scripts/circleci/add_build_info_json.sh + ./scripts/circleci/update_package_versions.sh + yarn build-combined + - persist_to_workspace: + root: build2 + paths: + - facebook-www + - facebook-react-native + - facebook-relay + - oss-stable + - oss-experimental + - react-native + - dist + - sizes/*.json + + process_artifacts_combined: + docker: *docker + environment: *environment + steps: + - checkout + - attach_workspace: + at: build2 + - run: yarn workspaces info | head -n -1 > workspace_info.txt + - *restore_node_modules + # Compress build directory into a single tarball for easy download + - run: tar -zcvf ./build2.tgz ./build2 + - store_artifacts: + path: ./build2.tgz + build_devtools_and_process_artifacts: docker: *docker environment: *environment @@ -611,6 +650,17 @@ workflows: only: - master + # New workflow that will replace "stable" and "experimental" + combined: + jobs: + - setup + - yarn_build_combined: + requires: + - setup + - process_artifacts_combined: + requires: + - yarn_build_combined + fuzz_tests: triggers: - schedule: diff --git a/.gitignore b/.gitignore index 5a913ac6e5278..fed2a08aa5d9e 100644 --- a/.gitignore +++ b/.gitignore @@ -8,6 +8,7 @@ scripts/flow/*/.flowconfig _SpecRunner.html __benchmarks__ build/ +build2/ remote-repo/ coverage/ .module-cache diff --git a/package.json b/package.json index bf225d9cdc81c..deb7b24882da6 100644 --- a/package.json +++ b/package.json @@ -106,6 +106,7 @@ }, "scripts": { "build": "node ./scripts/rollup/build.js", + "build-combined": "node ./scripts/rollup/build-all-release-channels.js", "build-for-devtools": "cross-env RELEASE_CHANNEL=experimental yarn build react/index,react-dom,react-is,react-debug-tools,scheduler,react-test-renderer,react-refresh", "build-for-devtools-dev": "yarn build-for-devtools --type=NODE_DEV", "build-for-devtools-prod": "yarn build-for-devtools --type=NODE_PROD", diff --git a/scripts/rollup/build-all-release-channels.js b/scripts/rollup/build-all-release-channels.js new file mode 100644 index 0000000000000..774f1011aa287 --- /dev/null +++ b/scripts/rollup/build-all-release-channels.js @@ -0,0 +1,123 @@ +'use strict'; + +/* eslint-disable no-for-of-loops/no-for-of-loops */ + +const fs = require('fs'); +const {spawnSync} = require('child_process'); +const tmp = require('tmp'); + +// Runs the build script for both stable and experimental release channels, +// by configuring an environment variable. + +if (process.env.CIRCLE_NODE_TOTAL) { + // In CI, we use multiple concurrent processes. Allocate half the processes to + // build the stable channel, and the other half for experimental. Override + // the environment variables to "trick" the underlying build script. + const total = parseInt(process.env.CIRCLE_NODE_TOTAL, 10); + const halfTotal = Math.floor(total / 2); + const index = parseInt(process.env.CIRCLE_NODE_INDEX, 10); + if (index < halfTotal) { + const nodeTotal = halfTotal; + const nodeIndex = index; + buildForChannel('stable', nodeTotal, nodeIndex); + processStable('./build'); + } else { + const nodeTotal = total - halfTotal; + const nodeIndex = index - halfTotal; + buildForChannel('experimental', nodeTotal, nodeIndex); + processExperimental('./build'); + } + + // TODO: Currently storing artifacts as `./build2` so that it doesn't conflict + // with old build job. Remove once we migrate rest of build/test pipeline. + fs.renameSync('./build', './build2'); +} else { + // Running locally, no concurrency. Move each channel's build artifacts into + // a temporary directory so that they don't conflict. + buildForChannel('stable', '', ''); + const stableDir = tmp.dirSync().name; + fs.renameSync('./build', stableDir); + processStable(stableDir); + + buildForChannel('experimental', '', ''); + const experimentalDir = tmp.dirSync().name; + fs.renameSync('./build', experimentalDir); + processExperimental(experimentalDir); + + // Then merge the experimental folder into the stable one. processExperimental + // will have already removed conflicting files. + // + // In CI, merging is handled automatically by CircleCI's workspace feature. + spawnSync('rsync', ['-ar', experimentalDir + '/', stableDir + '/']); + + // Now restore the combined directory back to its original name + // TODO: Currently storing artifacts as `./build2` so that it doesn't conflict + // with old build job. Remove once we migrate rest of build/test pipeline. + fs.renameSync(stableDir, './build2'); +} + +function buildForChannel(channel, nodeTotal, nodeIndex) { + spawnSync('node', ['./scripts/rollup/build.js', ...process.argv.slice(2)], { + stdio: ['pipe', process.stdout, process.stderr], + env: { + ...process.env, + RELEASE_CHANNEL: channel, + CIRCLE_NODE_TOTAL: nodeTotal, + CIRCLE_NODE_INDEX: nodeIndex, + }, + }); +} + +function processStable(buildDir) { + if (fs.existsSync(buildDir + '/node_modules')) { + fs.renameSync(buildDir + '/node_modules', buildDir + '/oss-stable'); + } + + if (fs.existsSync(buildDir + '/facebook-www')) { + for (const fileName of fs.readdirSync(buildDir + '/facebook-www')) { + const filePath = buildDir + '/facebook-www/' + fileName; + const stats = fs.statSync(filePath); + if (!stats.isDirectory()) { + fs.renameSync(filePath, filePath.replace('.js', '.classic.js')); + } + } + } + + if (fs.existsSync(buildDir + '/sizes')) { + fs.renameSync(buildDir + '/sizes', buildDir + '/sizes-stable'); + } +} + +function processExperimental(buildDir) { + if (fs.existsSync(buildDir + '/node_modules')) { + fs.renameSync(buildDir + '/node_modules', buildDir + '/oss-experimental'); + } + + if (fs.existsSync(buildDir + '/facebook-www')) { + for (const fileName of fs.readdirSync(buildDir + '/facebook-www')) { + const filePath = buildDir + '/facebook-www/' + fileName; + const stats = fs.statSync(filePath); + if (!stats.isDirectory()) { + fs.renameSync(filePath, filePath.replace('.js', '.modern.js')); + } + } + } + + if (fs.existsSync(buildDir + '/sizes')) { + fs.renameSync(buildDir + '/sizes', buildDir + '/sizes-experimental'); + } + + // Delete all other artifacts that weren't handled above. We assume they are + // duplicates of the corresponding artifacts in the stable channel. Ideally, + // the underlying build script should not have produced these files in the + // first place. + for (const pathName of fs.readdirSync(buildDir)) { + if ( + pathName !== 'oss-experimental' && + pathName !== 'facebook-www' && + pathName !== 'sizes-experimental' + ) { + spawnSync('rm', ['-rm', buildDir + '/' + pathName]); + } + } +} From 9a6a41d108e1bb7c4babebebad7e72daa22a2374 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 12 Jan 2021 11:50:38 -0600 Subject: [PATCH 06/16] Migrate build tests to combined workflow (#20574) --- .circleci/config.yml | 94 ++++++++++----------------- scripts/jest/config.build-devtools.js | 11 +++- scripts/jest/config.build.js | 11 +++- scripts/jest/jest-cli.js | 4 +- 4 files changed, 51 insertions(+), 69 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index a532fb2e067a7..75d8f945e9918 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -1,4 +1,4 @@ -version: 2 +version: 2.1 aliases: - &docker @@ -455,37 +455,20 @@ jobs: command: yarn lint-build - run: scripts/circleci/check_minified_errors.sh - yarn_test-stable_build: - docker: *docker - environment: *environment - parallelism: *TEST_PARALLELISM - steps: - - checkout - - attach_workspace: *attach_workspace - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - - run: yarn test-stable --build --ci - yarn_test_build: docker: *docker environment: *environment parallelism: *TEST_PARALLELISM + parameters: + args: + type: string steps: - checkout - - attach_workspace: *attach_workspace - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - - run: yarn test --build --ci - - yarn_test_build_devtools: - docker: *docker - environment: *environment - steps: - - checkout - - attach_workspace: *attach_workspace + - attach_workspace: + at: build2 - run: yarn workspaces info | head -n -1 > workspace_info.txt - *restore_node_modules - - run: yarn test --project=devtools --build --ci + - run: yarn test --build <> --ci RELEASE_CHANNEL_stable_yarn_test_dom_fixtures: docker: *docker @@ -518,28 +501,6 @@ jobs: FUZZ_TEST_SEED=$RANDOM yarn test fuzz --ci FUZZ_TEST_SEED=$RANDOM yarn test --prod fuzz --ci - yarn_test-stable_build_prod: - docker: *docker - environment: *environment - parallelism: *TEST_PARALLELISM - steps: - - checkout - - attach_workspace: *attach_workspace - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - - run: yarn test-stable --build --prod --ci - - yarn_test_build_prod: - docker: *docker - environment: *environment - parallelism: *TEST_PARALLELISM - steps: - - checkout - - attach_workspace: *attach_workspace - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - - run: yarn test --build --prod --ci - workflows: version: 2 stable: @@ -584,12 +545,6 @@ workflows: - RELEASE_CHANNEL_stable_yarn_lint_build: requires: - RELEASE_CHANNEL_stable_yarn_build - - yarn_test-stable_build: - requires: - - RELEASE_CHANNEL_stable_yarn_build - - yarn_test-stable_build_prod: - requires: - - RELEASE_CHANNEL_stable_yarn_build - RELEASE_CHANNEL_stable_yarn_test_dom_fixtures: requires: - RELEASE_CHANNEL_stable_yarn_build @@ -624,18 +579,9 @@ workflows: - sizebot_experimental: requires: - yarn_build - - yarn_test_build: - requires: - - yarn_build - - yarn_test_build_prod: - requires: - - yarn_build - yarn_lint_build: requires: - yarn_build - - yarn_test_build_devtools: - requires: - - yarn_build - build_devtools_and_process_artifacts: requires: - yarn_build @@ -660,6 +606,32 @@ workflows: - process_artifacts_combined: requires: - yarn_build_combined + - yarn_test_build: + requires: + - yarn_build_combined + matrix: + parameters: + args: + # Intentionally passing these as strings instead of creating a + # separate parameter per CLI argument, since it's easier to + # control/see which combinations we want to run. + - "-r=stable --env=development" + - "-r=stable --env=production" + - "-r=experimental --env=development" + - "-r=experimental --env=production" + + # Dev Tools + - "--project=devtools -r=experimental" + + # TODO: Update test config to support www build tests + # - "-r=www-classic --env=development" + # - "-r=www-classic --env=production" + # - "-r=www-classic --env=development --variant" + # - "-r=www-classic --env=production --variant" + # - "-r=www-modern --env=development" + # - "-r=www-modern --env=production" + # - "-r=www-modern --env=development --variant" + # - "-r=www-modern --env=production --variant" fuzz_tests: triggers: diff --git a/scripts/jest/config.build-devtools.js b/scripts/jest/config.build-devtools.js index 39985e7416e18..0d5a6039e414d 100644 --- a/scripts/jest/config.build-devtools.js +++ b/scripts/jest/config.build-devtools.js @@ -4,6 +4,9 @@ const {readdirSync, statSync} = require('fs'); const {join} = require('path'); const baseConfig = require('./config.base'); +const NODE_MODULES_DIR = + process.env.RELEASE_CHANNEL === 'stable' ? 'oss-stable' : 'oss-experimental'; + // Find all folders in packages/* with package.json const packagesRoot = join(__dirname, '..', '..', 'packages'); const packages = readdirSync(packagesRoot).filter(dir => { @@ -32,11 +35,13 @@ moduleNameMapper['react-devtools-feature-flags'] = // Map packages to bundles packages.forEach(name => { // Root entry point - moduleNameMapper[`^${name}$`] = `/build/node_modules/${name}`; + moduleNameMapper[ + `^${name}$` + ] = `/build2/${NODE_MODULES_DIR}/${name}`; // Named entry points moduleNameMapper[ `^${name}\/([^\/]+)$` - ] = `/build/node_modules/${name}/$1`; + ] = `/build2/${NODE_MODULES_DIR}/${name}/$1`; }); // Allow tests to import shared code (e.g. feature flags, getStackByFiberInDevAndProd) @@ -50,7 +55,7 @@ module.exports = Object.assign({}, baseConfig, { // Don't run bundle tests on -test.internal.* files testPathIgnorePatterns: ['/node_modules/', '-test.internal.js$'], // Exclude the build output from transforms - transformIgnorePatterns: ['/node_modules/', '/build/'], + transformIgnorePatterns: ['/node_modules/', '/build2/'], testRegex: 'packages/react-devtools-shared/src/__tests__/[^]+.test.js$', snapshotSerializers: [ require.resolve( diff --git a/scripts/jest/config.build.js b/scripts/jest/config.build.js index fe83740fa4fe7..95bc0e633ce5e 100644 --- a/scripts/jest/config.build.js +++ b/scripts/jest/config.build.js @@ -6,6 +6,9 @@ const baseConfig = require('./config.base'); process.env.IS_BUILD = true; +const NODE_MODULES_DIR = + process.env.RELEASE_CHANNEL === 'stable' ? 'oss-stable' : 'oss-experimental'; + // Find all folders in packages/* with package.json const packagesRoot = join(__dirname, '..', '..', 'packages'); const packages = readdirSync(packagesRoot).filter(dir => { @@ -35,11 +38,13 @@ moduleNameMapper[ // Map packages to bundles packages.forEach(name => { // Root entry point - moduleNameMapper[`^${name}$`] = `/build/node_modules/${name}`; + moduleNameMapper[ + `^${name}$` + ] = `/build2/${NODE_MODULES_DIR}/${name}`; // Named entry points moduleNameMapper[ `^${name}\/([^\/]+)$` - ] = `/build/node_modules/${name}/$1`; + ] = `/build2/${NODE_MODULES_DIR}/${name}/$1`; }); module.exports = Object.assign({}, baseConfig, { @@ -52,7 +57,7 @@ module.exports = Object.assign({}, baseConfig, { // Don't run bundle tests on -test.internal.* files testPathIgnorePatterns: ['/node_modules/', '-test.internal.js$'], // Exclude the build output from transforms - transformIgnorePatterns: ['/node_modules/', '/build/'], + transformIgnorePatterns: ['/node_modules/', '/build2/'], setupFiles: [ ...baseConfig.setupFiles, require.resolve('./setupTests.build.js'), diff --git a/scripts/jest/jest-cli.js b/scripts/jest/jest-cli.js index 19e3879c776a8..76fdd7e67581a 100644 --- a/scripts/jest/jest-cli.js +++ b/scripts/jest/jest-cli.js @@ -212,10 +212,10 @@ function validateOptions() { if (argv.build) { // TODO: We could build this if it hasn't been built yet. - const buildDir = path.resolve('./build'); + const buildDir = path.resolve('./build2'); if (!fs.existsSync(buildDir)) { logError( - 'Build directory does not exist, please run `yarn build` or remove the --build option.' + 'Build directory does not exist, please run `yarn build-combined` or remove the --build option.' ); success = false; } else if (Date.now() - fs.statSync(buildDir).mtimeMs > 1000 * 60 * 15) { From b99ac3d6dffbe57f94d368cc4f4e0ddf089e4f53 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Tue, 12 Jan 2021 12:58:49 -0600 Subject: [PATCH 07/16] Migrate remaining tests to combined workflow (#20577) --- .circleci/config.yml | 212 ++++++++----------------------------------- 1 file changed, 39 insertions(+), 173 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index 75d8f945e9918..c413bae823ceb 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -104,139 +104,6 @@ jobs: - *restore_node_modules - run: node ./scripts/tasks/flow-ci - yarn_test-stable: - docker: *docker - environment: *environment - parallelism: *TEST_PARALLELISM - - steps: - - checkout - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - - run: yarn test-stable --ci - - yarn_test: - docker: *docker - environment: *environment - parallelism: *TEST_PARALLELISM - steps: - - checkout - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - - run: yarn test --ci - - yarn_test-classic: - docker: *docker - environment: *environment - parallelism: *TEST_PARALLELISM - steps: - - checkout - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - - run: yarn test-classic --ci - - yarn_test-classic_variant: - docker: *docker - environment: *environment - parallelism: *TEST_PARALLELISM - steps: - - checkout - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - - run: yarn test-classic --variant --ci - - yarn_test-classic_prod: - docker: *docker - environment: *environment - parallelism: *TEST_PARALLELISM - steps: - - checkout - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - - run: yarn test-classic --prod --ci - - yarn_test-classic_prod_variant: - docker: *docker - environment: *environment - parallelism: *TEST_PARALLELISM - steps: - - checkout - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - - run: yarn test-classic --prod --variant --ci - - yarn_test-www: - docker: *docker - environment: *environment - parallelism: *TEST_PARALLELISM - steps: - - checkout - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - - run: yarn test-www --ci - - yarn_test-www_variant: - docker: *docker - environment: *environment - parallelism: *TEST_PARALLELISM - steps: - - checkout - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - - run: yarn test-www --variant --ci - - yarn_test-www_prod: - docker: *docker - environment: *environment - parallelism: *TEST_PARALLELISM - steps: - - checkout - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - - run: yarn test-www --prod --ci - - yarn_test-www_prod_variant: - docker: *docker - environment: *environment - parallelism: *TEST_PARALLELISM - steps: - - checkout - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - - run: yarn test-www --prod --variant --ci - - yarn_test-stable_persistent: - docker: *docker - environment: *environment - parallelism: *TEST_PARALLELISM - - steps: - - checkout - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - - run: yarn test-stable --persistent --ci - - yarn_test-stable_prod: - docker: *docker - environment: *environment - parallelism: *TEST_PARALLELISM - - steps: - - checkout - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - - run: yarn test-stable --prod --ci - - yarn_test_prod: - docker: *docker - environment: *environment - parallelism: *TEST_PARALLELISM - steps: - - checkout - - run: yarn workspaces info | head -n -1 > workspace_info.txt - - *restore_node_modules - - run: yarn test --prod --ci - RELEASE_CHANNEL_stable_yarn_build: docker: *docker environment: *environment @@ -455,6 +322,19 @@ jobs: command: yarn lint-build - run: scripts/circleci/check_minified_errors.sh + yarn_test: + docker: *docker + environment: *environment + parallelism: *TEST_PARALLELISM + parameters: + args: + type: string + steps: + - checkout + - run: yarn workspaces info | head -n -1 > workspace_info.txt + - *restore_node_modules + - run: yarn test <> --ci + yarn_test_build: docker: *docker environment: *environment @@ -512,27 +392,6 @@ workflows: - yarn_flow: requires: - setup - - yarn_test-stable: - requires: - - setup - - yarn_test-stable_prod: - requires: - - setup - - yarn_test-stable_persistent: - requires: - - setup - - yarn_test-classic: - requires: - - setup - - yarn_test-classic_variant: - requires: - - setup - - yarn_test-classic_prod: - requires: - - setup - - yarn_test-classic_prod_variant: - requires: - - setup - RELEASE_CHANNEL_stable_yarn_build: requires: - setup @@ -552,24 +411,6 @@ workflows: experimental: jobs: - setup - - yarn_test: - requires: - - setup - - yarn_test_prod: - requires: - - setup - - yarn_test-www: - requires: - - setup - - yarn_test-www_variant: - requires: - - setup - - yarn_test-www_prod: - requires: - - setup - - yarn_test-www_prod_variant: - requires: - - setup - yarn_build: requires: - setup @@ -597,9 +438,33 @@ workflows: - master # New workflow that will replace "stable" and "experimental" - combined: + build_and_test: jobs: - setup + - yarn_test: + requires: + - setup + matrix: + parameters: + args: + # Intentionally passing these as strings instead of creating a + # separate parameter per CLI argument, since it's easier to + # control/see which combinations we want to run. + - "-r=stable --env=development" + - "-r=stable --env=production" + - "-r=experimental --env=development" + - "-r=experimental --env=production" + - "-r=www-classic --env=development" + - "-r=www-classic --env=production" + - "-r=www-classic --env=development --variant" + - "-r=www-classic --env=production --variant" + - "-r=www-modern --env=development" + - "-r=www-modern --env=production" + - "-r=www-modern --env=development --variant" + - "-r=www-modern --env=production --variant" + + # TODO: Test more persistent configurations? + - '-r=stable --env=development --persistent' - yarn_build_combined: requires: - setup @@ -633,6 +498,7 @@ workflows: # - "-r=www-modern --env=development --variant" # - "-r=www-modern --env=production --variant" + # TODO: Test more persistent configurations? fuzz_tests: triggers: - schedule: From e6ed2bcf424d0a25a8f628f7bb9962a29ec7d88f Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 13 Jan 2021 11:54:03 -0600 Subject: [PATCH 08/16] Update package.json versions as part of build step (#20579) Fixes issue in the new build workflow where the experimental packages do not include "experimental" in the version string. This was because the previous approach relied on the RELEASE_CHANNEL environment variable, which we are no longer setting in the outer CI job, since we use the same job to build both channels. To solve, I moved the version post-processing into the build script itself. Only affects the new build workflow. Old workflow is unchanged. Longer term, I would like to remove version numbers from the source entirely, including the package.jsons. We should use a placeholder instead; that's mostly how it already works, since the release script swaps out the versions before we publish to stable. --- .circleci/config.yml | 6 +- scripts/rollup/build-all-release-channels.js | 72 ++++++++++++++++++-- 2 files changed, 67 insertions(+), 11 deletions(-) diff --git a/.circleci/config.yml b/.circleci/config.yml index c413bae823ceb..b5f7604059020 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -168,11 +168,7 @@ jobs: - checkout - run: yarn workspaces info | head -n -1 > workspace_info.txt - *restore_node_modules - - run: - command: | - ./scripts/circleci/add_build_info_json.sh - ./scripts/circleci/update_package_versions.sh - yarn build-combined + - run: yarn build-combined - persist_to_workspace: root: build2 paths: diff --git a/scripts/rollup/build-all-release-channels.js b/scripts/rollup/build-all-release-channels.js index 774f1011aa287..0b74e4c42d8e9 100644 --- a/scripts/rollup/build-all-release-channels.js +++ b/scripts/rollup/build-all-release-channels.js @@ -4,11 +4,18 @@ const fs = require('fs'); const {spawnSync} = require('child_process'); +const path = require('path'); const tmp = require('tmp'); // Runs the build script for both stable and experimental release channels, // by configuring an environment variable. +const sha = ( + spawnSync('git', ['show', '-s', '--format=%h']).stdout + '' +).trim(); +const ReactVersion = JSON.parse(fs.readFileSync('packages/react/package.json')) + .version; + if (process.env.CIRCLE_NODE_TOTAL) { // In CI, we use multiple concurrent processes. Allocate half the processes to // build the stable channel, and the other half for experimental. Override @@ -19,13 +26,19 @@ if (process.env.CIRCLE_NODE_TOTAL) { if (index < halfTotal) { const nodeTotal = halfTotal; const nodeIndex = index; + const version = '0.0.0-' + sha; + updateTheReactVersionThatDevToolsReads(ReactVersion + '-' + sha); buildForChannel('stable', nodeTotal, nodeIndex); - processStable('./build'); + processStable('./build', version); } else { const nodeTotal = total - halfTotal; const nodeIndex = index - halfTotal; + const version = '0.0.0-experimental-' + sha; + updateTheReactVersionThatDevToolsReads( + ReactVersion + '-experimental-' + sha + ); buildForChannel('experimental', nodeTotal, nodeIndex); - processExperimental('./build'); + processExperimental('./build', version); } // TODO: Currently storing artifacts as `./build2` so that it doesn't conflict @@ -34,15 +47,17 @@ if (process.env.CIRCLE_NODE_TOTAL) { } else { // Running locally, no concurrency. Move each channel's build artifacts into // a temporary directory so that they don't conflict. + const stableVersion = '0.0.0-' + sha; buildForChannel('stable', '', ''); const stableDir = tmp.dirSync().name; fs.renameSync('./build', stableDir); - processStable(stableDir); + processStable(stableDir, stableVersion); + const experimentalVersion = '0.0.0-experimental-' + sha; buildForChannel('experimental', '', ''); const experimentalDir = tmp.dirSync().name; fs.renameSync('./build', experimentalDir); - processExperimental(experimentalDir); + processExperimental(experimentalDir, experimentalVersion); // Then merge the experimental folder into the stable one. processExperimental // will have already removed conflicting files. @@ -68,8 +83,9 @@ function buildForChannel(channel, nodeTotal, nodeIndex) { }); } -function processStable(buildDir) { +function processStable(buildDir, version) { if (fs.existsSync(buildDir + '/node_modules')) { + updatePackageVersions(buildDir + '/node_modules', version); fs.renameSync(buildDir + '/node_modules', buildDir + '/oss-stable'); } @@ -88,8 +104,9 @@ function processStable(buildDir) { } } -function processExperimental(buildDir) { +function processExperimental(buildDir, version) { if (fs.existsSync(buildDir + '/node_modules')) { + updatePackageVersions(buildDir + '/node_modules', version); fs.renameSync(buildDir + '/node_modules', buildDir + '/oss-experimental'); } @@ -121,3 +138,46 @@ function processExperimental(buildDir) { } } } + +function updatePackageVersions(modulesDir, version) { + const allReactModuleNames = fs.readdirSync('packages'); + for (const moduleName of fs.readdirSync(modulesDir)) { + const packageJSONPath = path.join(modulesDir, moduleName, 'package.json'); + const stats = fs.statSync(packageJSONPath); + if (stats.isFile()) { + const packageInfo = JSON.parse(fs.readFileSync(packageJSONPath)); + + // Update version + packageInfo.version = version; + + // Update dependency versions + if (packageInfo.dependencies) { + for (const dep of Object.keys(packageInfo.dependencies)) { + if (allReactModuleNames.includes(dep)) { + packageInfo.dependencies[dep] = version; + } + } + } + if (packageInfo.peerDependencies) { + for (const dep of Object.keys(packageInfo.peerDependencies)) { + if (allReactModuleNames.includes(dep)) { + packageInfo.peerDependencies[dep] = version; + } + } + } + + // Write out updated package.json + fs.writeFileSync(packageJSONPath, JSON.stringify(packageInfo, null, 2)); + } + } +} + +function updateTheReactVersionThatDevToolsReads(version) { + // Overwrite the ReactVersion module before the build script runs so that it + // is included in the final bundles. This only runs in CI, so it's fine to + // edit the source file. + fs.writeFileSync( + './packages/shared/ReactVersion.js', + `export default '${version}';\n` + ); +} From fc07b070a09396e60126dd75641472f0c7504edf Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 13 Jan 2021 13:54:56 -0600 Subject: [PATCH 09/16] Retry with fresh otp if publish fails (#20582) Currently, if publishing a package fails, the script crashes, and the user must start it again from the beginning. Usually this happens because the one-time password has timed out. With this change, the user will be prompted for a fresh otp, and the script will resume publishing. --- scripts/release/publish.js | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/scripts/release/publish.js b/scripts/release/publish.js index e51df5c45f8ab..9ec9330a0973e 100755 --- a/scripts/release/publish.js +++ b/scripts/release/publish.js @@ -48,8 +48,22 @@ const run = async () => { await confirmVersionAndTags(params); await validateSkipPackages(params); await checkNPMPermissions(params); - const otp = await promptForOTP(params); - await publishToNPM(params, otp); + + while (true) { + try { + const otp = await promptForOTP(params); + await publishToNPM(params, otp); + break; + } catch (error) { + console.error(error.message); + console.log(); + console.log( + theme.error`Publish failed. Enter a fresh otp code to retry.` + ); + continue; + } + } + await updateStableVersionNumbers(params); await printFollowUpInstructions(params); } catch (error) { From a656ace8da6d005e49a750da3d9a3fd0fdcac76c Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Wed, 13 Jan 2021 18:18:55 -0600 Subject: [PATCH 10/16] Deletion effects should fire parent -> child (#20584) * Test: Deletion effects should fire parent -> child Regression in new effect implementation * Fix passive deletion effect ordering --- .../src/ReactFiberCommitWork.new.js | 12 +-- .../src/ReactFiberCommitWork.old.js | 12 +-- .../src/__tests__/ReactEffectOrdering-test.js | 86 +++++++++++++++++++ 3 files changed, 100 insertions(+), 10 deletions(-) create mode 100644 packages/react-reconciler/src/__tests__/ReactEffectOrdering-test.js diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index d3082d5f785b8..a048500e9ddf3 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -2005,6 +2005,13 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_begin( ) { while (nextEffect !== null) { const fiber = nextEffect; + + // Deletion effects fire in parent -> child order + // TODO: Check if fiber has a PassiveStatic flag + setCurrentDebugFiberInDEV(fiber); + commitPassiveUnmountInsideDeletedTreeOnFiber(fiber); + resetCurrentDebugFiberInDEV(); + const child = fiber.child; // TODO: Only traverse subtree if it has a PassiveStatic flag if (child !== null) { @@ -2023,11 +2030,6 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_complete( ) { while (nextEffect !== null) { const fiber = nextEffect; - // TODO: Check if fiber has a PassiveStatic flag - setCurrentDebugFiberInDEV(fiber); - commitPassiveUnmountInsideDeletedTreeOnFiber(fiber); - resetCurrentDebugFiberInDEV(); - if (fiber === deletedSubtreeRoot) { nextEffect = null; return; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 95e48b1e4ae4f..72f801981d30b 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -2005,6 +2005,13 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_begin( ) { while (nextEffect !== null) { const fiber = nextEffect; + + // Deletion effects fire in parent -> child order + // TODO: Check if fiber has a PassiveStatic flag + setCurrentDebugFiberInDEV(fiber); + commitPassiveUnmountInsideDeletedTreeOnFiber(fiber); + resetCurrentDebugFiberInDEV(); + const child = fiber.child; // TODO: Only traverse subtree if it has a PassiveStatic flag if (child !== null) { @@ -2023,11 +2030,6 @@ function commitPassiveUnmountEffectsInsideOfDeletedTree_complete( ) { while (nextEffect !== null) { const fiber = nextEffect; - // TODO: Check if fiber has a PassiveStatic flag - setCurrentDebugFiberInDEV(fiber); - commitPassiveUnmountInsideDeletedTreeOnFiber(fiber); - resetCurrentDebugFiberInDEV(); - if (fiber === deletedSubtreeRoot) { nextEffect = null; return; diff --git a/packages/react-reconciler/src/__tests__/ReactEffectOrdering-test.js b/packages/react-reconciler/src/__tests__/ReactEffectOrdering-test.js new file mode 100644 index 0000000000000..6e9ffcfb894f5 --- /dev/null +++ b/packages/react-reconciler/src/__tests__/ReactEffectOrdering-test.js @@ -0,0 +1,86 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @emails react-core + * @jest-environment node + */ + +/* eslint-disable no-func-assign */ + +'use strict'; + +let React; +let ReactNoop; +let Scheduler; +let useEffect; +let useLayoutEffect; + +describe('ReactHooksWithNoopRenderer', () => { + beforeEach(() => { + jest.resetModules(); + jest.useFakeTimers(); + + React = require('react'); + ReactNoop = require('react-noop-renderer'); + Scheduler = require('scheduler'); + useEffect = React.useEffect; + useLayoutEffect = React.useLayoutEffect; + }); + + test('layout unmmouts on deletion are fired in parent -> child order', async () => { + const root = ReactNoop.createRoot(); + + function Parent() { + useLayoutEffect(() => { + return () => Scheduler.unstable_yieldValue('Unmount parent'); + }); + return ; + } + + function Child() { + useLayoutEffect(() => { + return () => Scheduler.unstable_yieldValue('Unmount child'); + }); + return 'Child'; + } + + await ReactNoop.act(async () => { + root.render(); + }); + expect(root).toMatchRenderedOutput('Child'); + await ReactNoop.act(async () => { + root.render(null); + }); + expect(Scheduler).toHaveYielded(['Unmount parent', 'Unmount child']); + }); + + test('passive unmmouts on deletion are fired in parent -> child order', async () => { + const root = ReactNoop.createRoot(); + + function Parent() { + useEffect(() => { + return () => Scheduler.unstable_yieldValue('Unmount parent'); + }); + return ; + } + + function Child() { + useEffect(() => { + return () => Scheduler.unstable_yieldValue('Unmount child'); + }); + return 'Child'; + } + + await ReactNoop.act(async () => { + root.render(); + }); + expect(root).toMatchRenderedOutput('Child'); + await ReactNoop.act(async () => { + root.render(null); + }); + expect(Scheduler).toHaveYielded(['Unmount parent', 'Unmount child']); + }); +}); From 42e04b46d19648968537a404f15cebc42b6fab54 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 14 Jan 2021 10:18:55 -0600 Subject: [PATCH 11/16] Fix: Detach deleted fiber's alternate, too (#20587) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We need to call `detachFiberAfterEffects` on the fiber's alternate to clean it up. We're currently not, because the `alternate` field is cleared during `detachFiberMutation`. So I deferred detaching the `alternate` until the passive phase. Only the `return` pointer needs to be detached for semantic purposes. I don't think there's any good way to test this without using reflection. It's not even observable using out our "supported" reflection APIs (`findDOMNode`), or at least not that I can think of. Which is a good thing, in a way. It's not really a memory leak, either, because the only reference to the alternate fiber is from the parent's alternate. Which will be disconnected the next time the parent is updated or deleted. It's really only observable if you mess around with internals in ways you're not supposed to — I found it because a product test in www that uses Enzyme was doing just that. In lieu of a new unit test, I confirmed this patch fixes the broken product test. --- .../src/ReactFiberCommitWork.new.js | 14 +++++++++----- .../src/ReactFiberCommitWork.old.js | 14 +++++++++----- .../react-reconciler/src/ReactFiberWorkLoop.new.js | 4 ++++ .../react-reconciler/src/ReactFiberWorkLoop.old.js | 4 ++++ 4 files changed, 26 insertions(+), 10 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index a048500e9ddf3..32bb1028add82 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -1084,17 +1084,17 @@ function detachFiberMutation(fiber: Fiber) { // Note that we can't clear child or sibling pointers yet. // They're needed for passive effects and for findDOMNode. // We defer those fields, and all other cleanup, to the passive phase (see detachFiberAfterEffects). - const alternate = fiber.alternate; - if (alternate !== null) { - alternate.return = null; - fiber.alternate = null; - } + // + // Don't reset the alternate yet, either. We need that so we can detach the + // alternate's fields in the passive phase. Clearing the return pointer is + // sufficient for findDOMNode semantics. fiber.return = null; } export function detachFiberAfterEffects(fiber: Fiber): void { // Null out fields to improve GC for references that may be lingering (e.g. DevTools). // Note that we already cleared the return pointer in detachFiberMutation(). + fiber.alternate = null; fiber.child = null; fiber.deletions = null; fiber.dependencies = null; @@ -1936,7 +1936,11 @@ function commitPassiveUnmountEffects_begin() { commitPassiveUnmountEffectsInsideOfDeletedTree_begin(fiberToDelete); // Now that passive effects have been processed, it's safe to detach lingering pointers. + const alternate = fiberToDelete.alternate; detachFiberAfterEffects(fiberToDelete); + if (alternate !== null) { + detachFiberAfterEffects(alternate); + } } nextEffect = fiber; } diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.old.js b/packages/react-reconciler/src/ReactFiberCommitWork.old.js index 72f801981d30b..d3bfe78e39479 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.old.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.old.js @@ -1084,17 +1084,17 @@ function detachFiberMutation(fiber: Fiber) { // Note that we can't clear child or sibling pointers yet. // They're needed for passive effects and for findDOMNode. // We defer those fields, and all other cleanup, to the passive phase (see detachFiberAfterEffects). - const alternate = fiber.alternate; - if (alternate !== null) { - alternate.return = null; - fiber.alternate = null; - } + // + // Don't reset the alternate yet, either. We need that so we can detach the + // alternate's fields in the passive phase. Clearing the return pointer is + // sufficient for findDOMNode semantics. fiber.return = null; } export function detachFiberAfterEffects(fiber: Fiber): void { // Null out fields to improve GC for references that may be lingering (e.g. DevTools). // Note that we already cleared the return pointer in detachFiberMutation(). + fiber.alternate = null; fiber.child = null; fiber.deletions = null; fiber.dependencies = null; @@ -1936,7 +1936,11 @@ function commitPassiveUnmountEffects_begin() { commitPassiveUnmountEffectsInsideOfDeletedTree_begin(fiberToDelete); // Now that passive effects have been processed, it's safe to detach lingering pointers. + const alternate = fiberToDelete.alternate; detachFiberAfterEffects(fiberToDelete); + if (alternate !== null) { + detachFiberAfterEffects(alternate); + } } nextEffect = fiber; } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 03831988e2b2b..0c50c804da36b 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -2148,7 +2148,11 @@ function commitRootImpl(root, renderPriorityLevel) { if (deletions !== null) { for (let i = 0; i < deletions.length; i++) { const deletion = deletions[i]; + const alternate = deletion.alternate; detachFiberAfterEffects(deletion); + if (alternate !== null) { + detachFiberAfterEffects(alternate); + } } } } diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js index 6966472e0d16f..caeab7ce67330 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.old.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.old.js @@ -2148,7 +2148,11 @@ function commitRootImpl(root, renderPriorityLevel) { if (deletions !== null) { for (let i = 0; i < deletions.length; i++) { const deletion = deletions[i]; + const alternate = deletion.alternate; detachFiberAfterEffects(deletion); + if (alternate !== null) { + detachFiberAfterEffects(alternate); + } } } } From 98313aaa7ea58d49eb30ecffc0f9eb6fc1ef467a Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Thu, 14 Jan 2021 11:20:20 -0600 Subject: [PATCH 12/16] Migrate prepare-release-from-ci to new workflow (#20581) * Migrate prepare-release-from-ci to new workflow I added a `--releaseChannel (-r)` argument to script. You must choose either "stable" or "experimental", because every build job now includes both channels. The prepare-release-from-npm script is unchanged since those releases are downloaded from npm, nt CI. (As a side note, I think we should start preparing semver releases using the prepare-release-from-ci script, too, and get rid of prepare-release-from-npm. I think that was a neat idea originally but because we already run `npm pack` before storing the artifacts in CI, there's really not much additional safety; the only safeguard it adds is the requirement that a "next" release must have already been published.) * Move validation to parse-params module --- scripts/release/prepare-release-from-ci.js | 8 +-- .../download-build-artifacts.js | 67 ++++++++----------- .../get-latest-master-build-number.js | 8 +-- .../release/shared-commands/parse-params.js | 14 ++++ scripts/release/utils.js | 6 +- 5 files changed, 47 insertions(+), 56 deletions(-) diff --git a/scripts/release/prepare-release-from-ci.js b/scripts/release/prepare-release-from-ci.js index f7222ba8ca8a0..dd8e143453f79 100755 --- a/scripts/release/prepare-release-from-ci.js +++ b/scripts/release/prepare-release-from-ci.js @@ -3,8 +3,7 @@ 'use strict'; const {join} = require('path'); -const {readJsonSync} = require('fs-extra'); -const {getPublicPackages, handleError} = require('./utils'); +const {handleError} = require('./utils'); const checkEnvironmentVariables = require('./shared-commands/check-environment-variables'); const downloadBuildArtifacts = require('./shared-commands/download-build-artifacts'); @@ -26,11 +25,6 @@ const run = async () => { await checkEnvironmentVariables(params); await downloadBuildArtifacts(params); - const version = readJsonSync('./build/node_modules/react/package.json') - .version; - const isExperimental = version.includes('experimental'); - params.packages = await getPublicPackages(isExperimental); - if (!params.skipTests) { await testPackagingFixture(params); await testTracingFixture(params); diff --git a/scripts/release/shared-commands/download-build-artifacts.js b/scripts/release/shared-commands/download-build-artifacts.js index 7158341934407..4d5259e24d1c7 100644 --- a/scripts/release/shared-commands/download-build-artifacts.js +++ b/scripts/release/shared-commands/download-build-artifacts.js @@ -3,65 +3,56 @@ 'use strict'; const {exec} = require('child-process-promise'); -const {existsSync, readdirSync} = require('fs'); -const {readJsonSync} = require('fs-extra'); +const {existsSync} = require('fs'); const {join} = require('path'); const {getArtifactsList, logPromise} = require('../utils'); const theme = require('../theme'); -const run = async ({build, cwd}) => { +const run = async ({build, cwd, releaseChannel}) => { const artifacts = await getArtifactsList(build); - const nodeModulesArtifact = artifacts.find(entry => - entry.path.endsWith('node_modules.tgz') + const buildArtifacts = artifacts.find(entry => + entry.path.endsWith('build2.tgz') ); - if (!nodeModulesArtifact) { + if (!buildArtifacts) { console.log( theme`{error The specified build (${build}) does not contain any build artifacts.}` ); process.exit(1); } - const nodeModulesURL = nodeModulesArtifact.url; + // Download and extract artifact + await exec(`rm -rf ./build2`, {cwd}); + await exec( + `curl -L $(fwdproxy-config curl) ${buildArtifacts.url} | tar -xvz`, + { + cwd, + } + ); + // Copy to staging directory + // TODO: Consider staging the release in a different directory from the CI + // build artifacts: `./build/node_modules` -> `./staged-releases` if (!existsSync(join(cwd, 'build'))) { await exec(`mkdir ./build`, {cwd}); + } else { + await exec(`rm -rf ./build/node_modules`, {cwd}); } - - // Download and extract artifact - await exec(`rm -rf ./build/node_modules*`, {cwd}); - await exec(`curl -L ${nodeModulesURL} --output ./build/node_modules.tgz`, { - cwd, - }); - await exec(`mkdir ./build/node_modules`, {cwd}); - await exec(`tar zxvf ./build/node_modules.tgz -C ./build/node_modules/`, { - cwd, - }); - - // Unpack packages and prepare to publish - const compressedPackages = readdirSync(join(cwd, 'build/node_modules/')); - for (let i = 0; i < compressedPackages.length; i++) { - await exec( - `tar zxvf ./build/node_modules/${compressedPackages[i]} -C ./build/node_modules/`, - {cwd} - ); - const packageJSON = readJsonSync( - join(cwd, `/build/node_modules/package/package.json`) - ); - await exec( - `mv ./build/node_modules/package ./build/node_modules/${packageJSON.name}`, - {cwd} - ); + let sourceDir; + if (releaseChannel === 'stable') { + sourceDir = 'oss-stable'; + } else if (releaseChannel === 'experimental') { + sourceDir = 'oss-experimental'; + } else { + console.error('Internal error: Invalid release channel: ' + releaseChannel); + process.exit(releaseChannel); } - - // Cleanup - await exec(`rm ./build/node_modules.tgz`, {cwd}); - await exec(`rm ./build/node_modules/*.tgz`, {cwd}); + await exec(`cp -r ./build2/${sourceDir} ./build/node_modules`, {cwd}); }; -module.exports = async ({build, cwd}) => { +module.exports = async ({build, cwd, releaseChannel}) => { return logPromise( - run({build, cwd}), + run({build, cwd, releaseChannel}), theme`Downloading artifacts from Circle CI for build {build ${build}}` ); }; diff --git a/scripts/release/shared-commands/get-latest-master-build-number.js b/scripts/release/shared-commands/get-latest-master-build-number.js index 1d1787fad8f04..d0bcd56e2e042 100644 --- a/scripts/release/shared-commands/get-latest-master-build-number.js +++ b/scripts/release/shared-commands/get-latest-master-build-number.js @@ -5,11 +5,7 @@ const http = require('request-promise-json'); const {logPromise} = require('../utils'); -const run = async useExperimentalBuild => { - const targetJobName = useExperimentalBuild - ? 'process_artifacts_experimental' - : 'process_artifacts'; - +const run = async () => { // https://circleci.com/docs/api/#recent-builds-for-a-project-branch const metadataURL = `https://circleci.com/api/v1.1/project/github/facebook/react/tree/master`; const metadata = await http.get(metadataURL, true); @@ -17,7 +13,7 @@ const run = async useExperimentalBuild => { entry => entry.branch === 'master' && entry.status === 'success' && - entry.workflows.job_name === targetJobName + entry.workflows.job_name === 'process_artifacts_combined' ).build_num; return build; diff --git a/scripts/release/shared-commands/parse-params.js b/scripts/release/shared-commands/parse-params.js index 703fc0fb11b40..2ec21020282fa 100644 --- a/scripts/release/shared-commands/parse-params.js +++ b/scripts/release/shared-commands/parse-params.js @@ -17,10 +17,24 @@ const paramDefinitions = [ description: 'Skip automated fixture tests.', defaultValue: false, }, + { + name: 'releaseChannel', + alias: 'r', + type: String, + description: 'Release channel (stable or experimental)', + }, ]; module.exports = () => { const params = commandLineArgs(paramDefinitions); + const channel = params.releaseChannel; + if (channel !== 'experimental' && channel !== 'stable') { + console.error( + `Invalid release channel (-r) "${channel}". Must be "stable" or "experimental".` + ); + process.exit(1); + } + return params; }; diff --git a/scripts/release/utils.js b/scripts/release/utils.js index be191978df3f4..52731d4376e0f 100644 --- a/scripts/release/utils.js +++ b/scripts/release/utils.js @@ -41,11 +41,7 @@ const getArtifactsList = async buildID => { ); process.exit(1); } - const artifactsJobName = buildMetadata.workflows.job_name.endsWith( - '_experimental' - ) - ? 'process_artifacts_experimental' - : 'process_artifacts'; + const artifactsJobName = 'process_artifacts_combined'; const workflowID = buildMetadata.workflows.workflow_id; const workflowMetadataURL = `https://circleci.com/api/v2/workflow/${workflowID}/job?circle-token=${process.env.CIRCLE_CI_API_TOKEN}`; const workflowMetadata = await http.get(workflowMetadataURL, true); From 6132919bf2b8851382547b34a442e7e0c09c5697 Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 15 Jan 2021 14:22:23 -0600 Subject: [PATCH 13/16] Convert layout phase to depth-first traversal (#20595) --- .../src/ReactFiberCommitWork.new.js | 591 ++++++++++-------- .../src/ReactFiberWorkLoop.new.js | 88 +-- .../ReactDOMTracing-test.internal.js | 4 +- .../__tests__/ReactProfiler-test.internal.js | 6 +- scripts/jest/TestFlags.js | 7 +- 5 files changed, 368 insertions(+), 328 deletions(-) diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 32bb1028add82..4f77868463fc5 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -69,8 +69,11 @@ import { ChildDeletion, Snapshot, Update, + Callback, + Ref, Passive, PassiveMask, + LayoutMask, PassiveUnmountPendingDev, } from './ReactFiberFlags'; import getComponentName from 'shared/getComponentName'; @@ -490,95 +493,154 @@ export function commitPassiveEffectDurations( } } -function commitLifeCycles( +function commitLayoutEffectOnFiber( finishedRoot: FiberRoot, current: Fiber | null, finishedWork: Fiber, committedLanes: Lanes, ): void { - switch (finishedWork.tag) { - case FunctionComponent: - case ForwardRef: - case SimpleMemoComponent: { - // At this point layout effects have already been destroyed (during mutation phase). - // This is done to prevent sibling component effects from interfering with each other, - // e.g. a destroy function in one component should never override a ref set - // by a create function in another component during the same commit. - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - finishedWork.mode & ProfileMode - ) { - try { - startLayoutEffectTimer(); + if ((finishedWork.flags & (Update | Callback)) !== NoFlags) { + switch (finishedWork.tag) { + case FunctionComponent: + case ForwardRef: + case SimpleMemoComponent: { + // At this point layout effects have already been destroyed (during mutation phase). + // This is done to prevent sibling component effects from interfering with each other, + // e.g. a destroy function in one component should never override a ref set + // by a create function in another component during the same commit. + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + finishedWork.mode & ProfileMode + ) { + try { + startLayoutEffectTimer(); + commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork); + } finally { + recordLayoutEffectDuration(finishedWork); + } + } else { commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork); - } finally { - recordLayoutEffectDuration(finishedWork); } - } else { - commitHookEffectListMount(HookLayout | HookHasEffect, finishedWork); - } - schedulePassiveEffects(finishedWork); - return; - } - case ClassComponent: { - const instance = finishedWork.stateNode; - if (finishedWork.flags & Update) { - if (current === null) { - // We could update instance props and state here, - // but instead we rely on them being set during last render. - // TODO: revisit this when we implement resuming. - if (__DEV__) { + schedulePassiveEffects(finishedWork); + break; + } + case ClassComponent: { + const instance = finishedWork.stateNode; + if (finishedWork.flags & Update) { + if (current === null) { + // We could update instance props and state here, + // but instead we rely on them being set during last render. + // TODO: revisit this when we implement resuming. + if (__DEV__) { + if ( + finishedWork.type === finishedWork.elementType && + !didWarnAboutReassigningProps + ) { + if (instance.props !== finishedWork.memoizedProps) { + console.error( + 'Expected %s props to match memoized props before ' + + 'componentDidMount. ' + + 'This might either be because of a bug in React, or because ' + + 'a component reassigns its own `this.props`. ' + + 'Please file an issue.', + getComponentName(finishedWork.type) || 'instance', + ); + } + if (instance.state !== finishedWork.memoizedState) { + console.error( + 'Expected %s state to match memoized state before ' + + 'componentDidMount. ' + + 'This might either be because of a bug in React, or because ' + + 'a component reassigns its own `this.state`. ' + + 'Please file an issue.', + getComponentName(finishedWork.type) || 'instance', + ); + } + } + } if ( - finishedWork.type === finishedWork.elementType && - !didWarnAboutReassigningProps + enableProfilerTimer && + enableProfilerCommitHooks && + finishedWork.mode & ProfileMode ) { - if (instance.props !== finishedWork.memoizedProps) { - console.error( - 'Expected %s props to match memoized props before ' + - 'componentDidMount. ' + - 'This might either be because of a bug in React, or because ' + - 'a component reassigns its own `this.props`. ' + - 'Please file an issue.', - getComponentName(finishedWork.type) || 'instance', - ); + try { + startLayoutEffectTimer(); + instance.componentDidMount(); + } finally { + recordLayoutEffectDuration(finishedWork); } - if (instance.state !== finishedWork.memoizedState) { - console.error( - 'Expected %s state to match memoized state before ' + - 'componentDidMount. ' + - 'This might either be because of a bug in React, or because ' + - 'a component reassigns its own `this.state`. ' + - 'Please file an issue.', - getComponentName(finishedWork.type) || 'instance', - ); - } - } - } - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - finishedWork.mode & ProfileMode - ) { - try { - startLayoutEffectTimer(); + } else { instance.componentDidMount(); - } finally { - recordLayoutEffectDuration(finishedWork); } } else { - instance.componentDidMount(); + const prevProps = + finishedWork.elementType === finishedWork.type + ? current.memoizedProps + : resolveDefaultProps(finishedWork.type, current.memoizedProps); + const prevState = current.memoizedState; + // We could update instance props and state here, + // but instead we rely on them being set during last render. + // TODO: revisit this when we implement resuming. + if (__DEV__) { + if ( + finishedWork.type === finishedWork.elementType && + !didWarnAboutReassigningProps + ) { + if (instance.props !== finishedWork.memoizedProps) { + console.error( + 'Expected %s props to match memoized props before ' + + 'componentDidUpdate. ' + + 'This might either be because of a bug in React, or because ' + + 'a component reassigns its own `this.props`. ' + + 'Please file an issue.', + getComponentName(finishedWork.type) || 'instance', + ); + } + if (instance.state !== finishedWork.memoizedState) { + console.error( + 'Expected %s state to match memoized state before ' + + 'componentDidUpdate. ' + + 'This might either be because of a bug in React, or because ' + + 'a component reassigns its own `this.state`. ' + + 'Please file an issue.', + getComponentName(finishedWork.type) || 'instance', + ); + } + } + } + if ( + enableProfilerTimer && + enableProfilerCommitHooks && + finishedWork.mode & ProfileMode + ) { + try { + startLayoutEffectTimer(); + instance.componentDidUpdate( + prevProps, + prevState, + instance.__reactInternalSnapshotBeforeUpdate, + ); + } finally { + recordLayoutEffectDuration(finishedWork); + } + } else { + instance.componentDidUpdate( + prevProps, + prevState, + instance.__reactInternalSnapshotBeforeUpdate, + ); + } } - } else { - const prevProps = - finishedWork.elementType === finishedWork.type - ? current.memoizedProps - : resolveDefaultProps(finishedWork.type, current.memoizedProps); - const prevState = current.memoizedState; - // We could update instance props and state here, - // but instead we rely on them being set during last render. - // TODO: revisit this when we implement resuming. + } + + // TODO: I think this is now always non-null by the time it reaches the + // commit phase. Consider removing the type check. + const updateQueue: UpdateQueue< + *, + > | null = (finishedWork.updateQueue: any); + if (updateQueue !== null) { if (__DEV__) { if ( finishedWork.type === finishedWork.elementType && @@ -587,7 +649,7 @@ function commitLifeCycles( if (instance.props !== finishedWork.memoizedProps) { console.error( 'Expected %s props to match memoized props before ' + - 'componentDidUpdate. ' + + 'processing the update queue. ' + 'This might either be because of a bug in React, or because ' + 'a component reassigns its own `this.props`. ' + 'Please file an issue.', @@ -597,7 +659,7 @@ function commitLifeCycles( if (instance.state !== finishedWork.memoizedState) { console.error( 'Expected %s state to match memoized state before ' + - 'componentDidUpdate. ' + + 'processing the update queue. ' + 'This might either be because of a bug in React, or because ' + 'a component reassigns its own `this.state`. ' + 'Please file an issue.', @@ -606,210 +668,166 @@ function commitLifeCycles( } } } - if ( - enableProfilerTimer && - enableProfilerCommitHooks && - finishedWork.mode & ProfileMode - ) { - try { - startLayoutEffectTimer(); - instance.componentDidUpdate( - prevProps, - prevState, - instance.__reactInternalSnapshotBeforeUpdate, - ); - } finally { - recordLayoutEffectDuration(finishedWork); - } - } else { - instance.componentDidUpdate( - prevProps, - prevState, - instance.__reactInternalSnapshotBeforeUpdate, - ); - } + // We could update instance props and state here, + // but instead we rely on them being set during last render. + // TODO: revisit this when we implement resuming. + commitUpdateQueue(finishedWork, updateQueue, instance); } + break; } - - // TODO: I think this is now always non-null by the time it reaches the - // commit phase. Consider removing the type check. - const updateQueue: UpdateQueue< - *, - > | null = (finishedWork.updateQueue: any); - if (updateQueue !== null) { - if (__DEV__) { - if ( - finishedWork.type === finishedWork.elementType && - !didWarnAboutReassigningProps - ) { - if (instance.props !== finishedWork.memoizedProps) { - console.error( - 'Expected %s props to match memoized props before ' + - 'processing the update queue. ' + - 'This might either be because of a bug in React, or because ' + - 'a component reassigns its own `this.props`. ' + - 'Please file an issue.', - getComponentName(finishedWork.type) || 'instance', - ); - } - if (instance.state !== finishedWork.memoizedState) { - console.error( - 'Expected %s state to match memoized state before ' + - 'processing the update queue. ' + - 'This might either be because of a bug in React, or because ' + - 'a component reassigns its own `this.state`. ' + - 'Please file an issue.', - getComponentName(finishedWork.type) || 'instance', - ); + case HostRoot: { + // TODO: I think this is now always non-null by the time it reaches the + // commit phase. Consider removing the type check. + const updateQueue: UpdateQueue< + *, + > | null = (finishedWork.updateQueue: any); + if (updateQueue !== null) { + let instance = null; + if (finishedWork.child !== null) { + switch (finishedWork.child.tag) { + case HostComponent: + instance = getPublicInstance(finishedWork.child.stateNode); + break; + case ClassComponent: + instance = finishedWork.child.stateNode; + break; } } + commitUpdateQueue(finishedWork, updateQueue, instance); } - // We could update instance props and state here, - // but instead we rely on them being set during last render. - // TODO: revisit this when we implement resuming. - commitUpdateQueue(finishedWork, updateQueue, instance); + break; } - return; - } - case HostRoot: { - // TODO: I think this is now always non-null by the time it reaches the - // commit phase. Consider removing the type check. - const updateQueue: UpdateQueue< - *, - > | null = (finishedWork.updateQueue: any); - if (updateQueue !== null) { - let instance = null; - if (finishedWork.child !== null) { - switch (finishedWork.child.tag) { - case HostComponent: - instance = getPublicInstance(finishedWork.child.stateNode); - break; - case ClassComponent: - instance = finishedWork.child.stateNode; - break; - } + case HostComponent: { + const instance: Instance = finishedWork.stateNode; + + // Renderers may schedule work to be done after host components are mounted + // (eg DOM renderer may schedule auto-focus for inputs and form controls). + // These effects should only be committed when components are first mounted, + // aka when there is no current/alternate. + if (current === null && finishedWork.flags & Update) { + const type = finishedWork.type; + const props = finishedWork.memoizedProps; + commitMount(instance, type, props, finishedWork); } - commitUpdateQueue(finishedWork, updateQueue, instance); - } - return; - } - case HostComponent: { - const instance: Instance = finishedWork.stateNode; - // Renderers may schedule work to be done after host components are mounted - // (eg DOM renderer may schedule auto-focus for inputs and form controls). - // These effects should only be committed when components are first mounted, - // aka when there is no current/alternate. - if (current === null && finishedWork.flags & Update) { - const type = finishedWork.type; - const props = finishedWork.memoizedProps; - commitMount(instance, type, props, finishedWork); + break; } + case HostText: { + // We have no life-cycles associated with text. + break; + } + case HostPortal: { + // We have no life-cycles associated with portals. + break; + } + case Profiler: { + if (enableProfilerTimer) { + const {onCommit, onRender} = finishedWork.memoizedProps; + const {effectDuration} = finishedWork.stateNode; - return; - } - case HostText: { - // We have no life-cycles associated with text. - return; - } - case HostPortal: { - // We have no life-cycles associated with portals. - return; - } - case Profiler: { - if (enableProfilerTimer) { - const {onCommit, onRender} = finishedWork.memoizedProps; - const {effectDuration} = finishedWork.stateNode; - - const commitTime = getCommitTime(); - - let phase = current === null ? 'mount' : 'update'; - if (enableProfilerNestedUpdatePhase) { - if (isCurrentUpdateNested()) { - phase = 'nested-update'; - } - } + const commitTime = getCommitTime(); - if (typeof onRender === 'function') { - if (enableSchedulerTracing) { - onRender( - finishedWork.memoizedProps.id, - phase, - finishedWork.actualDuration, - finishedWork.treeBaseDuration, - finishedWork.actualStartTime, - commitTime, - finishedRoot.memoizedInteractions, - ); - } else { - onRender( - finishedWork.memoizedProps.id, - phase, - finishedWork.actualDuration, - finishedWork.treeBaseDuration, - finishedWork.actualStartTime, - commitTime, - ); + let phase = current === null ? 'mount' : 'update'; + if (enableProfilerNestedUpdatePhase) { + if (isCurrentUpdateNested()) { + phase = 'nested-update'; + } } - } - if (enableProfilerCommitHooks) { - if (typeof onCommit === 'function') { + if (typeof onRender === 'function') { if (enableSchedulerTracing) { - onCommit( + onRender( finishedWork.memoizedProps.id, phase, - effectDuration, + finishedWork.actualDuration, + finishedWork.treeBaseDuration, + finishedWork.actualStartTime, commitTime, finishedRoot.memoizedInteractions, ); } else { - onCommit( + onRender( finishedWork.memoizedProps.id, phase, - effectDuration, + finishedWork.actualDuration, + finishedWork.treeBaseDuration, + finishedWork.actualStartTime, commitTime, ); } } - // Schedule a passive effect for this Profiler to call onPostCommit hooks. - // This effect should be scheduled even if there is no onPostCommit callback for this Profiler, - // because the effect is also where times bubble to parent Profilers. - enqueuePendingPassiveProfilerEffect(finishedWork); + if (enableProfilerCommitHooks) { + if (typeof onCommit === 'function') { + if (enableSchedulerTracing) { + onCommit( + finishedWork.memoizedProps.id, + phase, + effectDuration, + commitTime, + finishedRoot.memoizedInteractions, + ); + } else { + onCommit( + finishedWork.memoizedProps.id, + phase, + effectDuration, + commitTime, + ); + } + } - // Propagate layout effect durations to the next nearest Profiler ancestor. - // Do not reset these values until the next render so DevTools has a chance to read them first. - let parentFiber = finishedWork.return; - while (parentFiber !== null) { - if (parentFiber.tag === Profiler) { - const parentStateNode = parentFiber.stateNode; - parentStateNode.effectDuration += effectDuration; - break; + // Schedule a passive effect for this Profiler to call onPostCommit hooks. + // This effect should be scheduled even if there is no onPostCommit callback for this Profiler, + // because the effect is also where times bubble to parent Profilers. + enqueuePendingPassiveProfilerEffect(finishedWork); + + // Propagate layout effect durations to the next nearest Profiler ancestor. + // Do not reset these values until the next render so DevTools has a chance to read them first. + let parentFiber = finishedWork.return; + while (parentFiber !== null) { + if (parentFiber.tag === Profiler) { + const parentStateNode = parentFiber.stateNode; + parentStateNode.effectDuration += effectDuration; + break; + } + parentFiber = parentFiber.return; } - parentFiber = parentFiber.return; } } + break; } - return; + case SuspenseComponent: { + commitSuspenseHydrationCallbacks(finishedRoot, finishedWork); + break; + } + case SuspenseListComponent: + case IncompleteClassComponent: + case FundamentalComponent: + case ScopeComponent: + case OffscreenComponent: + case LegacyHiddenComponent: + break; + default: + invariant( + false, + 'This unit of work tag should not have side-effects. This error is ' + + 'likely caused by a bug in React. Please file an issue.', + ); } - case SuspenseComponent: { - commitSuspenseHydrationCallbacks(finishedRoot, finishedWork); - return; + } + + if (enableScopeAPI) { + // TODO: This is a temporary solution that allowed us to transition away + // from React Flare on www. + if (finishedWork.flags & Ref && finishedWork.tag !== ScopeComponent) { + commitAttachRef(finishedWork); + } + } else { + if (finishedWork.flags & Ref) { + commitAttachRef(finishedWork); } - case SuspenseListComponent: - case IncompleteClassComponent: - case FundamentalComponent: - case ScopeComponent: - case OffscreenComponent: - case LegacyHiddenComponent: - return; } - invariant( - false, - 'This unit of work tag should not have side-effects. This error is ' + - 'likely caused by a bug in React. Please file an issue.', - ); } function hideOrUnhideAllChildren(finishedWork, isHidden) { @@ -1823,12 +1841,88 @@ function commitResetTextContent(current: Fiber) { resetTextContent(current.stateNode); } +export function commitLayoutEffects( + finishedWork: Fiber, + root: FiberRoot, + committedLanes: Lanes, +): void { + nextEffect = finishedWork; + commitLayoutEffects_begin(finishedWork, root, committedLanes); +} + +function commitLayoutEffects_begin( + subtreeRoot: Fiber, + root: FiberRoot, + committedLanes: Lanes, +) { + while (nextEffect !== null) { + const fiber = nextEffect; + const firstChild = fiber.child; + if ((fiber.subtreeFlags & LayoutMask) !== NoFlags && firstChild !== null) { + ensureCorrectReturnPointer(firstChild, fiber); + nextEffect = firstChild; + } else { + commitLayoutMountEffects_complete(subtreeRoot, root, committedLanes); + } + } +} + +function commitLayoutMountEffects_complete( + subtreeRoot: Fiber, + root: FiberRoot, + committedLanes: Lanes, +) { + while (nextEffect !== null) { + const fiber = nextEffect; + if ((fiber.flags & LayoutMask) !== NoFlags) { + const current = fiber.alternate; + if (__DEV__) { + setCurrentDebugFiberInDEV(fiber); + invokeGuardedCallback( + null, + commitLayoutEffectOnFiber, + null, + root, + current, + fiber, + committedLanes, + ); + if (hasCaughtError()) { + const error = clearCaughtError(); + captureCommitPhaseError(fiber, error); + } + resetCurrentDebugFiberInDEV(); + } else { + try { + commitLayoutEffectOnFiber(root, current, fiber, committedLanes); + } catch (error) { + captureCommitPhaseError(fiber, error); + } + } + } + + if (fiber === subtreeRoot) { + nextEffect = null; + return; + } + + const sibling = fiber.sibling; + if (sibling !== null) { + ensureCorrectReturnPointer(sibling, fiber.return); + nextEffect = sibling; + return; + } + + nextEffect = fiber.return; + } +} + export function commitPassiveMountEffects( root: FiberRoot, - firstChild: Fiber, + finishedWork: Fiber, ): void { - nextEffect = firstChild; - commitPassiveMountEffects_begin(firstChild, root); + nextEffect = finishedWork; + commitPassiveMountEffects_begin(finishedWork, root); } function commitPassiveMountEffects_begin(subtreeRoot: Fiber, root: FiberRoot) { @@ -2094,7 +2188,6 @@ export { commitPlacement, commitDeletion, commitWork, - commitLifeCycles, commitAttachRef, commitDetachRef, }; diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 0c50c804da36b..37f513f29d51f 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -131,7 +131,6 @@ import { Ref, ContentReset, Snapshot, - Callback, Passive, PassiveStatic, Incomplete, @@ -190,7 +189,7 @@ import { } from './ReactFiberThrow.new'; import { commitBeforeMutationLifeCycles as commitBeforeMutationEffectOnFiber, - commitLifeCycles as commitLayoutEffectOnFiber, + commitLayoutEffects, commitPlacement, commitWork, commitDeletion, @@ -2073,28 +2072,24 @@ function commitRootImpl(root, renderPriorityLevel) { // The next phase is the layout phase, where we call effects that read // the host tree after it's been mutated. The idiomatic use case for this is // layout, but class component lifecycles also fire here for legacy reasons. - nextEffect = firstEffect; - do { - if (__DEV__) { - invokeGuardedCallback(null, commitLayoutEffects, null, root, lanes); - if (hasCaughtError()) { - invariant(nextEffect !== null, 'Should be working on an effect.'); - const error = clearCaughtError(); - captureCommitPhaseError(nextEffect, error); - nextEffect = nextEffect.nextEffect; - } - } else { - try { - commitLayoutEffects(root, lanes); - } catch (error) { - invariant(nextEffect !== null, 'Should be working on an effect.'); - captureCommitPhaseError(nextEffect, error); - nextEffect = nextEffect.nextEffect; - } + if (__DEV__) { + if (enableDebugTracing) { + logLayoutEffectsStarted(lanes); } - } while (nextEffect !== null); + } + if (enableSchedulingProfiler) { + markLayoutEffectsStarted(lanes); + } + commitLayoutEffects(finishedWork, root, lanes); + if (__DEV__) { + if (enableDebugTracing) { + logLayoutEffectsStopped(); + } + } - nextEffect = null; + if (enableSchedulingProfiler) { + markLayoutEffectsStopped(); + } if (enableProfilerTimer && enableProfilerNestedUpdateScheduledHook) { rootCommittingMutationOrLayoutEffects = null; @@ -2420,55 +2415,6 @@ function commitMutationEffects( } } -function commitLayoutEffects(root: FiberRoot, committedLanes: Lanes) { - if (__DEV__) { - if (enableDebugTracing) { - logLayoutEffectsStarted(committedLanes); - } - } - - if (enableSchedulingProfiler) { - markLayoutEffectsStarted(committedLanes); - } - - // TODO: Should probably move the bulk of this function to commitWork. - while (nextEffect !== null) { - setCurrentDebugFiberInDEV(nextEffect); - - const flags = nextEffect.flags; - - if (flags & (Update | Callback)) { - const current = nextEffect.alternate; - commitLayoutEffectOnFiber(root, current, nextEffect, committedLanes); - } - - if (enableScopeAPI) { - // TODO: This is a temporary solution that allowed us to transition away - // from React Flare on www. - if (flags & Ref && nextEffect.tag !== ScopeComponent) { - commitAttachRef(nextEffect); - } - } else { - if (flags & Ref) { - commitAttachRef(nextEffect); - } - } - - resetCurrentDebugFiberInDEV(); - nextEffect = nextEffect.nextEffect; - } - - if (__DEV__) { - if (enableDebugTracing) { - logLayoutEffectsStopped(); - } - } - - if (enableSchedulingProfiler) { - markLayoutEffectsStopped(); - } -} - export function flushPassiveEffects(): boolean { // Returns whether passive effects were flushed. if (pendingPassiveEffectsRenderPriority !== NoSchedulerPriority) { diff --git a/packages/react/src/__tests__/ReactDOMTracing-test.internal.js b/packages/react/src/__tests__/ReactDOMTracing-test.internal.js index 5026605672fcb..bd097fea76548 100644 --- a/packages/react/src/__tests__/ReactDOMTracing-test.internal.js +++ b/packages/react/src/__tests__/ReactDOMTracing-test.internal.js @@ -152,7 +152,7 @@ describe('ReactDOMTracing', () => { onInteractionScheduledWorkCompleted, ).toHaveBeenLastNotifiedOfInteraction(interaction); - if (gate(flags => flags.dfsEffectsRefactor)) { + if (gate(flags => flags.enableUseJSStackToTrackPassiveDurations)) { expect(onRender).toHaveBeenCalledTimes(3); } else { // TODO: This is 4 instead of 3 because this update was scheduled at @@ -310,7 +310,7 @@ describe('ReactDOMTracing', () => { expect( onInteractionScheduledWorkCompleted, ).toHaveBeenLastNotifiedOfInteraction(interaction); - if (gate(flags => flags.dfsEffectsRefactor)) { + if (gate(flags => flags.enableUseJSStackToTrackPassiveDurations)) { expect(onRender).toHaveBeenCalledTimes(3); } else { // TODO: This is 4 instead of 3 because this update was scheduled at diff --git a/packages/react/src/__tests__/ReactProfiler-test.internal.js b/packages/react/src/__tests__/ReactProfiler-test.internal.js index eb0c69537df4b..8387319511b55 100644 --- a/packages/react/src/__tests__/ReactProfiler-test.internal.js +++ b/packages/react/src/__tests__/ReactProfiler-test.internal.js @@ -368,7 +368,7 @@ describe('Profiler', () => { renderer.update(); - if (gate(flags => flags.dfsEffectsRefactor)) { + if (gate(flags => flags.enableUseJSStackToTrackPassiveDurations)) { // None of the Profiler's subtree was rendered because App bailed out before the Profiler. // So we expect onRender not to be called. expect(callback).not.toHaveBeenCalled(); @@ -4383,7 +4383,7 @@ describe('Profiler', () => { // because the resolved suspended subtree doesn't contain any passive effects. // If or its decendents had a passive effect, // onPostCommit would be called again. - if (gate(flags => flags.dfsEffectsRefactor)) { + if (gate(flags => flags.enableUseJSStackToTrackPassiveDurations)) { expect(Scheduler).toFlushAndYield([]); } else { expect(Scheduler).toFlushAndYield(['onPostCommit']); @@ -4874,7 +4874,7 @@ describe('Profiler', () => { }); if (__DEV__) { - // @gate dfsEffectsRefactor + // @gate enableUseJSStackToTrackPassiveDurations // @gate enableDoubleInvokingEffects it('double invoking does not disconnect wrapped async work', () => { ReactFeatureFlags.enableDoubleInvokingEffects = true; diff --git a/scripts/jest/TestFlags.js b/scripts/jest/TestFlags.js index f869620f03290..d7e82312dd2a5 100644 --- a/scripts/jest/TestFlags.js +++ b/scripts/jest/TestFlags.js @@ -45,9 +45,10 @@ const environmentFlags = { // Use this for tests that are known to be broken. FIXME: false, - // Turn this flag back on (or delete) once the effect list is removed in favor - // of a depth-first traversal using `subtreeTags`. - dfsEffectsRefactor: false, + // Turn these flags back on (or delete) once the effect list is removed in + // favor of a depth-first traversal using `subtreeTags`. + dfsEffectsRefactor: __VARIANT__, + enableUseJSStackToTrackPassiveDurations: false, }; function getTestFlags() { From 95feb0e701a5ae20996e8cc6c4acd0f504d5985a Mon Sep 17 00:00:00 2001 From: Andrew Clark Date: Fri, 15 Jan 2021 14:24:20 -0600 Subject: [PATCH 14/16] Convert mutation phase to depth-first traversal (#20596) --- .../src/ReactChildFiber.new.js | 13 +- .../src/ReactFiberBeginWork.new.js | 10 +- .../src/ReactFiberCommitWork.new.js | 170 ++++++++++++++++++ .../src/ReactFiberHydrationContext.new.js | 5 +- .../src/ReactFiberWorkLoop.new.js | 152 +--------------- 5 files changed, 180 insertions(+), 170 deletions(-) diff --git a/packages/react-reconciler/src/ReactChildFiber.new.js b/packages/react-reconciler/src/ReactChildFiber.new.js index 7163df34d8568..83362b5e15411 100644 --- a/packages/react-reconciler/src/ReactChildFiber.new.js +++ b/packages/react-reconciler/src/ReactChildFiber.new.js @@ -282,22 +282,13 @@ function ChildReconciler(shouldTrackSideEffects) { childToDelete.nextEffect = null; childToDelete.flags = (childToDelete.flags & StaticMask) | Deletion; - let deletions = returnFiber.deletions; + const deletions = returnFiber.deletions; if (deletions === null) { - deletions = returnFiber.deletions = [childToDelete]; + returnFiber.deletions = [childToDelete]; returnFiber.flags |= ChildDeletion; } else { deletions.push(childToDelete); } - // Stash a reference to the return fiber's deletion array on each of the - // deleted children. This is really weird, but it's a temporary workaround - // while we're still using the effect list to traverse effect fibers. A - // better workaround would be to follow the `.return` pointer in the commit - // phase, but unfortunately we can't assume that `.return` points to the - // correct fiber, even in the commit phase, because `findDOMNode` might - // mutate it. - // TODO: Remove this line. - childToDelete.deletions = deletions; } function deleteRemainingChildren( diff --git a/packages/react-reconciler/src/ReactFiberBeginWork.new.js b/packages/react-reconciler/src/ReactFiberBeginWork.new.js index 52e6216d442eb..9684efafc409c 100644 --- a/packages/react-reconciler/src/ReactFiberBeginWork.new.js +++ b/packages/react-reconciler/src/ReactFiberBeginWork.new.js @@ -2203,14 +2203,13 @@ function updateSuspensePrimaryChildren( currentFallbackChildFragment.flags = (currentFallbackChildFragment.flags & StaticMask) | Deletion; workInProgress.firstEffect = workInProgress.lastEffect = currentFallbackChildFragment; - let deletions = workInProgress.deletions; + const deletions = workInProgress.deletions; if (deletions === null) { - deletions = workInProgress.deletions = [currentFallbackChildFragment]; + workInProgress.deletions = [currentFallbackChildFragment]; workInProgress.flags |= ChildDeletion; } else { deletions.push(currentFallbackChildFragment); } - currentFallbackChildFragment.deletions = deletions; } workInProgress.child = primaryChildFragment; @@ -3194,14 +3193,13 @@ function remountFiber( current.nextEffect = null; current.flags = (current.flags & StaticMask) | Deletion; - let deletions = returnFiber.deletions; + const deletions = returnFiber.deletions; if (deletions === null) { - deletions = returnFiber.deletions = [current]; + returnFiber.deletions = [current]; returnFiber.flags |= ChildDeletion; } else { deletions.push(current); } - current.deletions = deletions; newWorkInProgress.flags |= Placement; diff --git a/packages/react-reconciler/src/ReactFiberCommitWork.new.js b/packages/react-reconciler/src/ReactFiberCommitWork.new.js index 4f77868463fc5..35115140041c7 100644 --- a/packages/react-reconciler/src/ReactFiberCommitWork.new.js +++ b/packages/react-reconciler/src/ReactFiberCommitWork.new.js @@ -66,12 +66,16 @@ import { NoFlags, ContentReset, Placement, + PlacementAndUpdate, ChildDeletion, Snapshot, Update, Callback, Ref, + Hydrating, + HydratingAndUpdate, Passive, + MutationMask, PassiveMask, LayoutMask, PassiveUnmountPendingDev, @@ -1841,6 +1845,172 @@ function commitResetTextContent(current: Fiber) { resetTextContent(current.stateNode); } +export function commitMutationEffects( + root: FiberRoot, + renderPriorityLevel: ReactPriorityLevel, + firstChild: Fiber, +) { + nextEffect = firstChild; + commitMutationEffects_begin(root, renderPriorityLevel); +} + +function commitMutationEffects_begin( + root: FiberRoot, + renderPriorityLevel: ReactPriorityLevel, +) { + while (nextEffect !== null) { + const fiber = nextEffect; + + // TODO: Should wrap this in flags check, too, as optimization + const deletions = fiber.deletions; + if (deletions !== null) { + for (let i = 0; i < deletions.length; i++) { + const childToDelete = deletions[i]; + if (__DEV__) { + invokeGuardedCallback( + null, + commitDeletion, + null, + root, + childToDelete, + renderPriorityLevel, + ); + if (hasCaughtError()) { + const error = clearCaughtError(); + captureCommitPhaseError(childToDelete, error); + } + } else { + try { + commitDeletion(root, childToDelete, renderPriorityLevel); + } catch (error) { + captureCommitPhaseError(childToDelete, error); + } + } + } + } + + const child = fiber.child; + if ((fiber.subtreeFlags & MutationMask) !== NoFlags && child !== null) { + ensureCorrectReturnPointer(child, fiber); + nextEffect = child; + } else { + commitMutationEffects_complete(root, renderPriorityLevel); + } + } +} + +function commitMutationEffects_complete( + root: FiberRoot, + renderPriorityLevel: ReactPriorityLevel, +) { + while (nextEffect !== null) { + const fiber = nextEffect; + if (__DEV__) { + setCurrentDebugFiberInDEV(fiber); + invokeGuardedCallback( + null, + commitMutationEffectsOnFiber, + null, + fiber, + root, + renderPriorityLevel, + ); + if (hasCaughtError()) { + const error = clearCaughtError(); + captureCommitPhaseError(fiber, error); + } + resetCurrentDebugFiberInDEV(); + } else { + try { + commitMutationEffectsOnFiber(fiber, root, renderPriorityLevel); + } catch (error) { + captureCommitPhaseError(fiber, error); + } + } + + const sibling = fiber.sibling; + if (sibling !== null) { + ensureCorrectReturnPointer(sibling, fiber.return); + nextEffect = sibling; + return; + } + + nextEffect = fiber.return; + } +} + +function commitMutationEffectsOnFiber( + finishedWork: Fiber, + root: FiberRoot, + renderPriorityLevel: ReactPriorityLevel, +) { + const flags = finishedWork.flags; + + if (flags & ContentReset) { + commitResetTextContent(finishedWork); + } + + if (flags & Ref) { + const current = finishedWork.alternate; + if (current !== null) { + commitDetachRef(current); + } + if (enableScopeAPI) { + // TODO: This is a temporary solution that allowed us to transition away + // from React Flare on www. + if (finishedWork.tag === ScopeComponent) { + commitAttachRef(finishedWork); + } + } + } + + // The following switch statement is only concerned about placement, + // updates, and deletions. To avoid needing to add a case for every possible + // bitmap value, we remove the secondary effects from the effect tag and + // switch on that value. + const primaryFlags = flags & (Placement | Update | Hydrating); + outer: switch (primaryFlags) { + case Placement: { + commitPlacement(finishedWork); + // Clear the "placement" from effect tag so that we know that this is + // inserted, before any life-cycles like componentDidMount gets called. + // TODO: findDOMNode doesn't rely on this any more but isMounted does + // and isMounted is deprecated anyway so we should be able to kill this. + finishedWork.flags &= ~Placement; + break; + } + case PlacementAndUpdate: { + // Placement + commitPlacement(finishedWork); + // Clear the "placement" from effect tag so that we know that this is + // inserted, before any life-cycles like componentDidMount gets called. + finishedWork.flags &= ~Placement; + + // Update + const current = finishedWork.alternate; + commitWork(current, finishedWork); + break; + } + case Hydrating: { + finishedWork.flags &= ~Hydrating; + break; + } + case HydratingAndUpdate: { + finishedWork.flags &= ~Hydrating; + + // Update + const current = finishedWork.alternate; + commitWork(current, finishedWork); + break; + } + case Update: { + const current = finishedWork.alternate; + commitWork(current, finishedWork); + break; + } + } +} + export function commitLayoutEffects( finishedWork: Fiber, root: FiberRoot, diff --git a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js index 37be28be6628c..f416c33b21f1b 100644 --- a/packages/react-reconciler/src/ReactFiberHydrationContext.new.js +++ b/packages/react-reconciler/src/ReactFiberHydrationContext.new.js @@ -144,14 +144,13 @@ function deleteHydratableInstance( returnFiber.firstEffect = returnFiber.lastEffect = childToDelete; } - let deletions = returnFiber.deletions; + const deletions = returnFiber.deletions; if (deletions === null) { - deletions = returnFiber.deletions = [childToDelete]; + returnFiber.deletions = [childToDelete]; returnFiber.flags |= ChildDeletion; } else { deletions.push(childToDelete); } - childToDelete.deletions = deletions; } function insertNonHydratedInstance(returnFiber: Fiber, fiber: Fiber) { diff --git a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js index 37f513f29d51f..4eafe42585043 100644 --- a/packages/react-reconciler/src/ReactFiberWorkLoop.new.js +++ b/packages/react-reconciler/src/ReactFiberWorkLoop.new.js @@ -31,7 +31,6 @@ import { decoupleUpdatePriorityFromScheduler, enableDebugTracing, enableSchedulingProfiler, - enableScopeAPI, disableSchedulerTimeoutInWorkLoop, } from 'shared/ReactFeatureFlags'; import ReactSharedInternals from 'shared/ReactSharedInternals'; @@ -116,7 +115,6 @@ import { ForwardRef, MemoComponent, SimpleMemoComponent, - ScopeComponent, Profiler, } from './ReactWorkTags'; import {LegacyRoot} from './ReactRootTags'; @@ -124,19 +122,14 @@ import { NoFlags, PerformedWork, Placement, - Update, - PlacementAndUpdate, Deletion, ChildDeletion, - Ref, - ContentReset, Snapshot, Passive, PassiveStatic, Incomplete, HostEffectMask, Hydrating, - HydratingAndUpdate, StaticMask, } from './ReactFiberFlags'; import { @@ -190,13 +183,8 @@ import { import { commitBeforeMutationLifeCycles as commitBeforeMutationEffectOnFiber, commitLayoutEffects, - commitPlacement, - commitWork, - commitDeletion, - commitDetachRef, - commitAttachRef, + commitMutationEffects, commitPassiveEffectDurations, - commitResetTextContent, isSuspenseBoundaryBeingHidden, commitPassiveMountEffects, commitPassiveUnmountEffects, @@ -2031,32 +2019,7 @@ function commitRootImpl(root, renderPriorityLevel) { } // The next phase is the mutation phase, where we mutate the host tree. - nextEffect = firstEffect; - do { - if (__DEV__) { - invokeGuardedCallback( - null, - commitMutationEffects, - null, - root, - renderPriorityLevel, - ); - if (hasCaughtError()) { - invariant(nextEffect !== null, 'Should be working on an effect.'); - const error = clearCaughtError(); - captureCommitPhaseError(nextEffect, error); - nextEffect = nextEffect.nextEffect; - } - } else { - try { - commitMutationEffects(root, renderPriorityLevel); - } catch (error) { - invariant(nextEffect !== null, 'Should be working on an effect.'); - captureCommitPhaseError(nextEffect, error); - nextEffect = nextEffect.nextEffect; - } - } - } while (nextEffect !== null); + commitMutationEffects(root, renderPriorityLevel, finishedWork); if (shouldFireAfterActiveInstanceBlur) { afterActiveInstanceBlur(); @@ -2304,117 +2267,6 @@ function commitBeforeMutationEffects() { } } -function commitMutationEffects( - root: FiberRoot, - renderPriorityLevel: ReactPriorityLevel, -) { - // TODO: Should probably move the bulk of this function to commitWork. - while (nextEffect !== null) { - setCurrentDebugFiberInDEV(nextEffect); - - const flags = nextEffect.flags; - - if (flags & ContentReset) { - commitResetTextContent(nextEffect); - } - - if (flags & Ref) { - const current = nextEffect.alternate; - if (current !== null) { - commitDetachRef(current); - } - if (enableScopeAPI) { - // TODO: This is a temporary solution that allowed us to transition away - // from React Flare on www. - if (nextEffect.tag === ScopeComponent) { - commitAttachRef(nextEffect); - } - } - } - - // The following switch statement is only concerned about placement, - // updates, and deletions. To avoid needing to add a case for every possible - // bitmap value, we remove the secondary effects from the effect tag and - // switch on that value. - const primaryFlags = flags & (Placement | Update | Deletion | Hydrating); - outer: switch (primaryFlags) { - case Placement: { - commitPlacement(nextEffect); - // Clear the "placement" from effect tag so that we know that this is - // inserted, before any life-cycles like componentDidMount gets called. - // TODO: findDOMNode doesn't rely on this any more but isMounted does - // and isMounted is deprecated anyway so we should be able to kill this. - nextEffect.flags &= ~Placement; - break; - } - case PlacementAndUpdate: { - // Placement - commitPlacement(nextEffect); - // Clear the "placement" from effect tag so that we know that this is - // inserted, before any life-cycles like componentDidMount gets called. - nextEffect.flags &= ~Placement; - - // Update - const current = nextEffect.alternate; - commitWork(current, nextEffect); - break; - } - case Hydrating: { - nextEffect.flags &= ~Hydrating; - break; - } - case HydratingAndUpdate: { - nextEffect.flags &= ~Hydrating; - - // Update - const current = nextEffect.alternate; - commitWork(current, nextEffect); - break; - } - case Update: { - const current = nextEffect.alternate; - commitWork(current, nextEffect); - break; - } - case Deletion: { - // Reached a deletion effect. Instead of commit this effect like we - // normally do, we're going to use the `deletions` array of the parent. - // However, because the effect list is sorted in depth-first order, we - // can't wait until we reach the parent node, because the child effects - // will have run in the meantime. - // - // So instead, we use a trick where the first time we hit a deletion - // effect, we commit all the deletion effects that belong to that parent. - // - // This is an incremental step away from using the effect list and - // toward a DFS + subtreeFlags traversal. - // - // A reference to the deletion array of the parent is also stored on - // each of the deletions. This is really weird. It would be better to - // follow the `.return` pointer, but unfortunately we can't assume that - // `.return` points to the correct fiber, even in the commit phase, - // because `findDOMNode` might mutate it. - const deletedChild = nextEffect; - const deletions = deletedChild.deletions; - if (deletions !== null) { - for (let i = 0; i < deletions.length; i++) { - const deletion = deletions[i]; - // Clear the deletion effect so that we don't delete this node more - // than once. - deletion.flags &= ~Deletion; - deletion.deletions = null; - commitDeletion(root, deletion, renderPriorityLevel); - } - } - break; - } - } - - resetCurrentDebugFiberInDEV(); - nextEffect = nextEffect.nextEffect; - } -} - export function flushPassiveEffects(): boolean { // Returns whether passive effects were flushed. if (pendingPassiveEffectsRenderPriority !== NoSchedulerPriority) { From 9a2150719b971adf078eca5cecbb18c146545cbd Mon Sep 17 00:00:00 2001 From: Sebastian Silbermann Date: Mon, 18 Jan 2021 20:56:17 +0100 Subject: [PATCH 15/16] Fix prod build in ci/codesandbox (#20606) --- .codesandbox/ci.json | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.codesandbox/ci.json b/.codesandbox/ci.json index 6b3a763cf0839..6f276d052e4a8 100644 --- a/.codesandbox/ci.json +++ b/.codesandbox/ci.json @@ -1,6 +1,6 @@ { "packages": ["packages/react", "packages/react-dom", "packages/scheduler"], - "buildCommand": "build --type=NODE react/index,react-dom/index,react-dom/server,react-dom/test-utils,scheduler/index,scheduler/tracing", + "buildCommand": "build --type=NODE react/index,react-dom/index,react-dom/server,react-dom/test-utils,scheduler/index,scheduler/unstable_no_dom,scheduler/tracing", "publishDirectory": { "react": "build/node_modules/react", "react-dom": "build/node_modules/react-dom", From af16f755dc7e0d6e2b4bf79b86c434f4ce0497fe Mon Sep 17 00:00:00 2001 From: Brian Vaughn Date: Tue, 19 Jan 2021 06:51:32 -0800 Subject: [PATCH 16/16] Update DevTools to use getCacheForType API (#20548) DevTools was built with a fork of an early idea for how Suspense cache might work. This idea is incompatible with newer APIs like `useTransition` which unfortunately prevented me from making certain UX improvements. This PR swaps out the primary usage of this cache (there are a few) in favor of the newer `unstable_getCacheForType` and `unstable_useCacheRefresh` APIs. We can go back and update the others in follow up PRs. ### Messaging changes I've refactored the way the frontend loads component props/state/etc to hopefully make it better match the Suspense+cache model. Doing this gave up some of the small optimizations I'd added but hopefully the actual performance impact of that is minor and the overall ergonomic improvements of working with the cache API make this worth it. The backend no longer remembers inspected paths. Instead, the frontend sends them every time and the backend sends a response with those paths. I've also added a new "force" parameter that the frontend can use to tell the backend to send a response even if the component hasn't rendered since the last time it asked. (This is used to get data for newly inspected paths.) _Initial inspection..._ ``` front | | back | -- "inspect" (id:1, paths:[], force:true) ---------> | | <------------------------ "inspected" (full-data) -- | ``` _1 second passes with no updates..._ ``` | -- "inspect" (id:1, paths:[], force:false) --------> | | <------------------------ "inspected" (no-change) -- | ``` _User clicks to expand a path, aka hydrate..._ ``` | -- "inspect" (id:1, paths:['foo'], force:true) ----> | | <------------------------ "inspected" (full-data) -- | ``` _1 second passes during which there is an update..._ ``` | -- "inspect" (id:1, paths:['foo'], force:false) ---> | | <----------------- "inspectedElement" (full-data) -- | ``` ### Clear errors/warnings transition Previously this meant there would be a delay after clicking the "clear" button. The UX after this change is much improved. ### Hydrating paths transition I also added a transition to hydration (expanding "dehyrated" paths). ### Better error boundaries I also added a lower-level error boundary in case the new suspense operation ever failed. It provides a better "retry" mechanism (select a new element) so DevTools doesn't become entirely useful. Here I'm intentionally causing an error every time I select an element. ### Improved snapshot tests I also migrated several of the existing snapshot tests to use inline snapshots and added a new serializer for dehydrated props. Inline snapshots are easier to verify and maintain and the new serializer means dehydrated props will be formatted in a way that makes sense rather than being empty (in external snapshots) or super verbose (default inline snapshot format). --- package.json | 2 +- .../inspectedElementContext-test.js.snap | 670 ------------- .../__tests__/dehydratedValueSerializer.js | 39 + ...ntext-test.js => inspectedElement-test.js} | 920 ++++++++++++------ .../__tests__/inspectedElementSerializer.js | 23 +- .../__snapshots__/inspectElement-test.js.snap | 303 ------ .../__tests__/legacy/inspectElement-test.js | 315 ++++-- .../src/__tests__/store-test.js | 26 +- .../src/__tests__/treeContext-test.js | 14 +- .../src/backend/agent.js | 17 +- .../src/backend/legacy/renderer.js | 33 +- .../src/backend/renderer.js | 148 +-- .../src/backend/types.js | 16 +- .../react-devtools-shared/src/backendAPI.js | 283 ++++++ packages/react-devtools-shared/src/bridge.js | 6 +- .../src/devtools/cache.js | 2 + .../src/devtools/store.js | 38 +- .../devtools/views/Components/Components.js | 53 +- .../views/Components/ExpandCollapseToggle.js | 3 + .../views/Components/InspectedElement.js | 16 +- .../Components/InspectedElementContext.js | 454 ++------- .../Components/InspectedElementContextTree.js | 10 +- .../InspectedElementErrorBoundary.css | 21 + .../InspectedElementErrorBoundary.js | 93 ++ .../InspectedElementErrorsAndWarningsTree.js | 80 +- .../Components/InspectedElementHooksTree.js | 34 +- .../Components/InspectedElementPropsTree.js | 8 +- .../Components/InspectedElementStateTree.js | 8 +- .../views/Components/InspectedElementView.js | 107 +- .../devtools/views/Components/KeyValue.css | 4 + .../src/devtools/views/Components/KeyValue.js | 64 +- .../views/Components/LoadingAnimation.css | 5 + .../views/Components/LoadingAnimation.js | 55 ++ .../src/devtools/views/Components/Tree.js | 8 +- .../src/devtools/views/ErrorBoundary.js | 19 +- .../src/inspectedElementCache.js | 220 +++++ .../forks/ReactFeatureFlags.test-renderer.js | 2 +- scripts/jest/config.build-devtools.js | 3 + scripts/jest/preprocessor.js | 4 + 39 files changed, 2054 insertions(+), 2072 deletions(-) delete mode 100644 packages/react-devtools-shared/src/__tests__/__snapshots__/inspectedElementContext-test.js.snap create mode 100644 packages/react-devtools-shared/src/__tests__/dehydratedValueSerializer.js rename packages/react-devtools-shared/src/__tests__/{inspectedElementContext-test.js => inspectedElement-test.js} (75%) delete mode 100644 packages/react-devtools-shared/src/__tests__/legacy/__snapshots__/inspectElement-test.js.snap create mode 100644 packages/react-devtools-shared/src/backendAPI.js create mode 100644 packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorBoundary.css create mode 100644 packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorBoundary.js create mode 100644 packages/react-devtools-shared/src/devtools/views/Components/LoadingAnimation.css create mode 100644 packages/react-devtools-shared/src/devtools/views/Components/LoadingAnimation.js create mode 100644 packages/react-devtools-shared/src/inspectedElementCache.js diff --git a/package.json b/package.json index deb7b24882da6..bca998508f1dc 100644 --- a/package.json +++ b/package.json @@ -107,7 +107,7 @@ "scripts": { "build": "node ./scripts/rollup/build.js", "build-combined": "node ./scripts/rollup/build-all-release-channels.js", - "build-for-devtools": "cross-env RELEASE_CHANNEL=experimental yarn build react/index,react-dom,react-is,react-debug-tools,scheduler,react-test-renderer,react-refresh", + "build-for-devtools": "cross-env RELEASE_CHANNEL=experimental yarn build-combined react/index,react-dom,react-is,react-debug-tools,scheduler,react-test-renderer,react-refresh", "build-for-devtools-dev": "yarn build-for-devtools --type=NODE_DEV", "build-for-devtools-prod": "yarn build-for-devtools --type=NODE_PROD", "linc": "node ./scripts/tasks/linc.js", diff --git a/packages/react-devtools-shared/src/__tests__/__snapshots__/inspectedElementContext-test.js.snap b/packages/react-devtools-shared/src/__tests__/__snapshots__/inspectedElementContext-test.js.snap deleted file mode 100644 index 8270d0c5b5f1d..0000000000000 --- a/packages/react-devtools-shared/src/__tests__/__snapshots__/inspectedElementContext-test.js.snap +++ /dev/null @@ -1,670 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`InspectedElementContext should dehydrate complex nested values when requested: 1: Initially inspect element 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": null, - "props": { - "set_of_sets": { - "0": {}, - "1": {} - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should dehydrate complex nested values when requested: 2: Inspect props.set_of_sets.0 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": null, - "props": { - "set_of_sets": { - "0": { - "0": 1, - "1": 2, - "2": 3 - }, - "1": {} - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should display complex values of useDebugValue: DisplayedComplexValue 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": [ - { - "id": null, - "isStateEditable": false, - "name": "DebuggableHook", - "value": { - "foo": 2 - }, - "subHooks": [ - { - "id": 0, - "isStateEditable": true, - "name": "State", - "value": 1, - "subHooks": [] - } - ] - } - ], - "props": {}, - "state": null -} -`; - -exports[`InspectedElementContext should include updates for nested values that were previously hydrated: 1: Initially inspect element 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": null, - "props": { - "nestedObject": { - "a": {}, - "c": {} - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should include updates for nested values that were previously hydrated: 2: Inspect props.nestedObject.a 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": null, - "props": { - "nestedObject": { - "a": { - "value": 1, - "b": { - "value": 1 - } - }, - "c": {} - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should include updates for nested values that were previously hydrated: 3: Inspect props.nestedObject.c 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": null, - "props": { - "nestedObject": { - "a": { - "value": 1, - "b": { - "value": 1 - } - }, - "c": { - "value": 1, - "d": { - "value": 1, - "e": {} - } - } - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should include updates for nested values that were previously hydrated: 4: update inspected element 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": null, - "props": { - "nestedObject": { - "a": { - "value": 2, - "b": { - "value": 2 - } - }, - "c": { - "value": 2, - "d": { - "value": 2, - "e": {} - } - } - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should inspect hooks for components that only use context: 1: Inspected element 2 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": [ - { - "id": null, - "isStateEditable": false, - "name": "Context", - "value": true, - "subHooks": [] - } - ], - "props": { - "a": 1, - "b": "abc" - }, - "state": null -} -`; - -exports[`InspectedElementContext should inspect the currently selected element: 1: Inspected element 2 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": [ - { - "id": 0, - "isStateEditable": true, - "name": "State", - "value": 1, - "subHooks": [] - } - ], - "props": { - "a": 1, - "b": "abc" - }, - "state": null -} -`; - -exports[`InspectedElementContext should not consume iterables while inspecting: 1: Inspected element 2 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": null, - "props": { - "prop": {} - }, - "state": null -} -`; - -exports[`InspectedElementContext should not dehydrate nested values until explicitly requested: 1: Initially inspect element 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": [ - { - "id": 0, - "isStateEditable": true, - "name": "State", - "value": { - "foo": {} - }, - "subHooks": [] - } - ], - "props": { - "nestedObject": { - "a": {} - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should not dehydrate nested values until explicitly requested: 2: Inspect props.nestedObject.a 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": [ - { - "id": 0, - "isStateEditable": true, - "name": "State", - "value": { - "foo": {} - }, - "subHooks": [] - } - ], - "props": { - "nestedObject": { - "a": { - "b": { - "c": {} - } - } - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should not dehydrate nested values until explicitly requested: 3: Inspect props.nestedObject.a.b.c 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": [ - { - "id": 0, - "isStateEditable": true, - "name": "State", - "value": { - "foo": {} - }, - "subHooks": [] - } - ], - "props": { - "nestedObject": { - "a": { - "b": { - "c": [ - { - "d": {} - } - ] - } - } - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should not dehydrate nested values until explicitly requested: 4: Inspect props.nestedObject.a.b.c.0.d 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": [ - { - "id": 0, - "isStateEditable": true, - "name": "State", - "value": { - "foo": {} - }, - "subHooks": [] - } - ], - "props": { - "nestedObject": { - "a": { - "b": { - "c": [ - { - "d": { - "e": {} - } - } - ] - } - } - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should not dehydrate nested values until explicitly requested: 5: Inspect hooks.0.value 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": [ - { - "id": 0, - "isStateEditable": true, - "name": "State", - "value": { - "foo": { - "bar": {} - } - }, - "subHooks": [] - } - ], - "props": { - "nestedObject": { - "a": { - "b": { - "c": [ - { - "d": { - "e": {} - } - } - ] - } - } - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should not dehydrate nested values until explicitly requested: 6: Inspect hooks.0.value.foo.bar 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": [ - { - "id": 0, - "isStateEditable": true, - "name": "State", - "value": { - "foo": { - "bar": { - "baz": "hi" - } - } - }, - "subHooks": [] - } - ], - "props": { - "nestedObject": { - "a": { - "b": { - "c": [ - { - "d": { - "e": {} - } - } - ] - } - } - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should not re-render a function with hooks if it did not update since it was last inspected: 1: initial render 1`] = ` -{ - "id": 3, - "owners": null, - "context": null, - "hooks": [ - { - "id": 0, - "isStateEditable": true, - "name": "State", - "value": 0, - "subHooks": [] - } - ], - "props": { - "a": 1, - "b": "abc" - }, - "state": null -} -`; - -exports[`InspectedElementContext should not re-render a function with hooks if it did not update since it was last inspected: 2: updated state 1`] = ` -{ - "id": 3, - "owners": null, - "context": null, - "hooks": [ - { - "id": 0, - "isStateEditable": true, - "name": "State", - "value": 0, - "subHooks": [] - } - ], - "props": { - "a": 2, - "b": "def" - }, - "state": null -} -`; - -exports[`InspectedElementContext should not tear if hydration is requested after an update: 1: Initially inspect element 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": null, - "props": { - "nestedObject": { - "value": 1, - "a": {} - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should not tear if hydration is requested after an update: 2: Inspect props.nestedObject.a 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": null, - "props": { - "nestedObject": { - "value": 2, - "a": { - "value": 2, - "b": { - "value": 2 - } - } - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should poll for updates for the currently selected element: 1: initial render 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": null, - "props": { - "a": 1, - "b": "abc" - }, - "state": null -} -`; - -exports[`InspectedElementContext should poll for updates for the currently selected element: 2: updated state 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": null, - "props": { - "a": 2, - "b": "def" - }, - "state": null -} -`; - -exports[`InspectedElementContext should support complex data types: 1: Inspected element 2 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": null, - "props": { - "anonymous_fn": {}, - "array_buffer": {}, - "array_of_arrays": [ - {} - ], - "big_int": {}, - "bound_fn": {}, - "data_view": {}, - "date": {}, - "fn": {}, - "html_element": {}, - "immutable": { - "0": {}, - "1": {}, - "2": {} - }, - "map": { - "0": {}, - "1": {} - }, - "map_of_maps": { - "0": {}, - "1": {} - }, - "object_of_objects": { - "inner": {} - }, - "object_with_symbol": { - "Symbol(name)": "hello" - }, - "proxy": {}, - "react_element": {}, - "regexp": {}, - "set": { - "0": "abc", - "1": 123 - }, - "set_of_sets": { - "0": {}, - "1": {} - }, - "symbol": {}, - "typed_array": { - "0": 100, - "1": -100, - "2": 0 - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should support custom objects with enumerable properties and getters: 1: Inspected element 2 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": null, - "props": { - "data": { - "_number": 42, - "number": 42 - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should support objects with no prototype: 1: Inspected element 2 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": null, - "props": { - "object": { - "string": "abc", - "number": 123, - "boolean": true - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should support objects with overridden hasOwnProperty: 1: Inspected element 2 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": null, - "props": { - "object": { - "name": "blah", - "hasOwnProperty": true - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should support objects with with inherited keys: 1: Inspected element 2 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": null, - "props": { - "object": { - "123": 3, - "enumerableString": 2, - "Symbol(enumerableSymbol)": 3, - "enumerableStringBase": 1, - "Symbol(enumerableSymbolBase)": 1 - } - }, - "state": null -} -`; - -exports[`InspectedElementContext should support simple data types: 1: Initial inspection 1`] = ` -{ - "id": 2, - "owners": null, - "context": null, - "hooks": null, - "props": { - "boolean_false": false, - "boolean_true": true, - "infinity": null, - "integer_zero": 0, - "integer_one": 1, - "float": 1.23, - "string": "abc", - "string_empty": "", - "nan": null, - "value_null": null - }, - "state": null -} -`; diff --git a/packages/react-devtools-shared/src/__tests__/dehydratedValueSerializer.js b/packages/react-devtools-shared/src/__tests__/dehydratedValueSerializer.js new file mode 100644 index 0000000000000..d8eac57df035c --- /dev/null +++ b/packages/react-devtools-shared/src/__tests__/dehydratedValueSerializer.js @@ -0,0 +1,39 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +// test() is part of Jest's serializer API +export function test(maybeDehydratedValue) { + const {meta} = require('react-devtools-shared/src/hydration'); + return ( + maybeDehydratedValue !== null && + typeof maybeDehydratedValue === 'object' && + maybeDehydratedValue.hasOwnProperty(meta.inspectable) && + maybeDehydratedValue[meta.inspected] !== true + ); +} + +// print() is part of Jest's serializer API +export function print(dehydratedValue, serialize, indent) { + const {meta} = require('react-devtools-shared/src/hydration'); + const indentation = Math.max(indent('.').indexOf('.') - 2, 0); + const paddingLeft = ' '.repeat(indentation); + return ( + 'Dehydrated {\n' + + paddingLeft + + ' "preview_short": ' + + dehydratedValue[meta.preview_short] + + ',\n' + + paddingLeft + + ' "preview_long": ' + + dehydratedValue[meta.preview_long] + + ',\n' + + paddingLeft + + '}' + ); +} diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js similarity index 75% rename from packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js rename to packages/react-devtools-shared/src/__tests__/inspectedElement-test.js index c60560302f55f..6a596a8b4a624 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElementContext-test.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElement-test.js @@ -8,16 +8,12 @@ */ import typeof ReactTestRenderer from 'react-test-renderer'; -import type { - CopyInspectedElementPath, - GetInspectedElementPath, - StoreAsGlobal, -} from 'react-devtools-shared/src/devtools/views/Components/InspectedElementContext'; +import {withErrorsOrWarningsIgnored} from 'react-devtools-shared/src/__tests__/utils'; + import type {FrontendBridge} from 'react-devtools-shared/src/bridge'; import type Store from 'react-devtools-shared/src/devtools/store'; -import {withErrorsOrWarningsIgnored} from 'react-devtools-shared/src/__tests__/utils'; -describe('InspectedElementContext', () => { +describe('InspectedElement', () => { let React; let ReactDOM; let PropTypes; @@ -81,14 +77,26 @@ describe('InspectedElementContext', () => { - - {children} - + + + {children} + + ); + function useInspectedElement(id: number) { + const {inspectedElement} = React.useContext(InspectedElementContext); + return inspectedElement; + } + + function useInspectElementPath(id: number) { + const {inspectPaths} = React.useContext(InspectedElementContext); + return inspectPaths; + } + it('should inspect the currently selected element', async done => { const Example = () => { const [count] = React.useState(1); @@ -105,9 +113,29 @@ describe('InspectedElementContext', () => { let didFinish = false; function Suspender({target}) { - const {getInspectedElement} = React.useContext(InspectedElementContext); - const inspectedElement = getInspectedElement(id); - expect(inspectedElement).toMatchSnapshot(`1: Inspected element ${id}`); + const inspectedElement = useInspectedElement(id); + expect(inspectedElement).toMatchInlineSnapshot(` + Object { + "context": null, + "events": undefined, + "hooks": Array [ + Object { + "id": 0, + "isStateEditable": true, + "name": "State", + "subHooks": Array [], + "value": 1, + }, + ], + "id": 2, + "owners": null, + "props": Object { + "a": 1, + "b": "abc", + }, + "state": null, + } + `); didFinish = true; return null; } @@ -211,8 +239,7 @@ describe('InspectedElementContext', () => { ]; function Suspender({target, shouldHaveLegacyContext}) { - const {getInspectedElement} = React.useContext(InspectedElementContext); - const inspectedElement = getInspectedElement(target); + const inspectedElement = useInspectedElement(target); expect(inspectedElement.context).not.toBe(null); expect(inspectedElement.hasLegacyContext).toBe(shouldHaveLegacyContext); @@ -257,8 +284,7 @@ describe('InspectedElementContext', () => { let inspectedElement = null; function Suspender({target}) { - const {getInspectedElement} = React.useContext(InspectedElementContext); - inspectedElement = getInspectedElement(id); + inspectedElement = useInspectedElement(id); return null; } @@ -273,14 +299,28 @@ describe('InspectedElementContext', () => { , ); }, false); - expect(inspectedElement).toMatchSnapshot('1: initial render'); + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "a": 1, + "b": "abc", + } + `); await utils.actAsync( () => ReactDOM.render(, container), false, ); - inspectedElement = null; + // TODO (cache) + // This test only passes if both the check-for-updates poll AND the test renderer.update() call are included below. + // It seems like either one of the two should be sufficient but: + // 1. Running only check-for-updates schedules a transition that React never renders. + // 2. Running only renderer.update() loads stale data (first props) + + // Wait for our check-for-updates poll to get the new data. + jest.runOnlyPendingTimers(); + await Promise.resolve(); + await utils.actAsync( () => renderer.update( @@ -294,7 +334,12 @@ describe('InspectedElementContext', () => { ), false, ); - expect(inspectedElement).toMatchSnapshot('2: updated state'); + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "a": 2, + "b": "def", + } + `); done(); }); @@ -305,6 +350,7 @@ describe('InspectedElementContext', () => { const Wrapper = ({children}) => children; const Target = React.memo(props => { targetRenderCount++; + // Even though his hook isn't referenced, it's used to observe backend rendering. React.useState(0); return null; }); @@ -324,8 +370,7 @@ describe('InspectedElementContext', () => { let inspectedElement = null; function Suspender({target}) { - const {getInspectedElement} = React.useContext(InspectedElementContext); - inspectedElement = getInspectedElement(target); + inspectedElement = useInspectedElement(target); return null; } @@ -346,7 +391,12 @@ describe('InspectedElementContext', () => { false, ); expect(targetRenderCount).toBe(1); - expect(inspectedElement).toMatchSnapshot('1: initial render'); + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "a": 1, + "b": "abc", + } + `); const initialInspectedElement = inspectedElement; @@ -369,6 +419,7 @@ describe('InspectedElementContext', () => { expect(inspectedElement).toEqual(initialInspectedElement); targetRenderCount = 0; + inspectedElement = null; await utils.actAsync( () => @@ -382,8 +433,26 @@ describe('InspectedElementContext', () => { ); // Target should have been rendered once (by ReactDOM) and once by DevTools for inspection. + await utils.actAsync( + () => + renderer.update( + + + + + , + ), + false, + ); expect(targetRenderCount).toBe(2); - expect(inspectedElement).toMatchSnapshot('2: updated state'); + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "a": 2, + "b": "def", + } + `); done(); }); @@ -426,8 +495,7 @@ describe('InspectedElementContext', () => { let inspectedElement = null; function Suspender({target}) { - const {getInspectedElement} = React.useContext(InspectedElementContext); - inspectedElement = getInspectedElement(target); + inspectedElement = useInspectedElement(target); return null; } @@ -482,8 +550,7 @@ describe('InspectedElementContext', () => { let inspectedElement = null; function Suspender({target}) { - const {getInspectedElement} = React.useContext(InspectedElementContext); - inspectedElement = getInspectedElement(id); + inspectedElement = useInspectedElement(id); return null; } @@ -501,9 +568,6 @@ describe('InspectedElementContext', () => { false, ); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot('1: Initial inspection'); - const {props} = (inspectedElement: any); expect(props.boolean_false).toBe(false); expect(props.boolean_true).toBe(true); @@ -606,8 +670,7 @@ describe('InspectedElementContext', () => { let inspectedElement = null; function Suspender({target}) { - const {getInspectedElement} = React.useContext(InspectedElementContext); - inspectedElement = getInspectedElement(id); + inspectedElement = useInspectedElement(id); return null; } @@ -625,9 +688,6 @@ describe('InspectedElementContext', () => { false, ); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot(`1: Inspected element ${id}`); - const { anonymous_fn, array_buffer, @@ -820,8 +880,7 @@ describe('InspectedElementContext', () => { let inspectedElement = null; function Suspender({target}) { - const {getInspectedElement} = React.useContext(InspectedElementContext); - inspectedElement = getInspectedElement(id); + inspectedElement = useInspectedElement(id); return null; } @@ -839,9 +898,6 @@ describe('InspectedElementContext', () => { false, ); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot(`1: Inspected element ${id}`); - const {prop} = (inspectedElement: any).props; expect(prop[meta.inspectable]).toBe(false); expect(prop[meta.name]).toBe('Generator'); @@ -870,8 +926,7 @@ describe('InspectedElementContext', () => { let inspectedElement = null; function Suspender({target}) { - const {getInspectedElement} = React.useContext(InspectedElementContext); - inspectedElement = getInspectedElement(id); + inspectedElement = useInspectedElement(id); return null; } @@ -889,13 +944,15 @@ describe('InspectedElementContext', () => { false, ); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot(`1: Inspected element ${id}`); - expect(inspectedElement.props.object).toEqual({ - boolean: true, - number: 123, - string: 'abc', - }); + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "object": Object { + "boolean": true, + "number": 123, + "string": "abc", + }, + } + `); done(); }); @@ -918,8 +975,7 @@ describe('InspectedElementContext', () => { let inspectedElement = null; function Suspender({target}) { - const {getInspectedElement} = React.useContext(InspectedElementContext); - inspectedElement = getInspectedElement(id); + inspectedElement = useInspectedElement(id); return null; } @@ -937,12 +993,10 @@ describe('InspectedElementContext', () => { false, ); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot(`1: Inspected element ${id}`); - expect(inspectedElement.props.object).toEqual({ - name: 'blah', - hasOwnProperty: true, - }); + // TRICKY: Don't use toMatchInlineSnapshot() for this test! + // Our snapshot serializer relies on hasOwnProperty() for feature detection. + expect(inspectedElement.props.object.name).toBe('blah'); + expect(inspectedElement.props.object.hasOwnProperty).toBe(true); done(); }); @@ -977,9 +1031,15 @@ describe('InspectedElementContext', () => { let didFinish = false; function Suspender({target}) { - const {getInspectedElement} = React.useContext(InspectedElementContext); - const inspectedElement = getInspectedElement(id); - expect(inspectedElement).toMatchSnapshot(`1: Inspected element ${id}`); + const inspectedElement = useInspectedElement(id); + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "data": Object { + "_number": 42, + "number": 42, + }, + } + `); didFinish = true; return null; } @@ -1075,8 +1135,7 @@ describe('InspectedElementContext', () => { let inspectedElement = null; function Suspender({target}) { - const {getInspectedElement} = React.useContext(InspectedElementContext); - inspectedElement = getInspectedElement(id); + inspectedElement = useInspectedElement(id); return null; } @@ -1094,15 +1153,17 @@ describe('InspectedElementContext', () => { false, ); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot(`1: Inspected element ${id}`); - expect(inspectedElement.props.object).toEqual({ - 123: 3, - 'Symbol(enumerableSymbol)': 3, - 'Symbol(enumerableSymbolBase)': 1, - enumerableString: 2, - enumerableStringBase: 1, - }); + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "object": Object { + "123": 3, + "Symbol(enumerableSymbol)": 3, + "Symbol(enumerableSymbolBase)": 1, + "enumerableString": 2, + "enumerableStringBase": 1, + }, + } + `); done(); }); @@ -1144,96 +1205,155 @@ describe('InspectedElementContext', () => { const id = ((store.getElementIDAtIndex(0): any): number); - let getInspectedElementPath: GetInspectedElementPath = ((null: any): GetInspectedElementPath); let inspectedElement = null; + let inspectElementPath = null; - function Suspender({target}) { - const context = React.useContext(InspectedElementContext); - getInspectedElementPath = context.getInspectedElementPath; - inspectedElement = context.getInspectedElement(target); + function Suspender({path, target}) { + inspectedElement = useInspectedElement(id); + inspectElementPath = useInspectElementPath(id); return null; } - await utils.actAsync( - () => - TestRenderer.create( - - - - - , - ), - false, - ); - expect(getInspectedElementPath).not.toBeNull(); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot('1: Initially inspect element'); + const renderer = TestRenderer.create(null); - inspectedElement = null; - TestUtilsAct(() => { - TestRendererAct(() => { - getInspectedElementPath(id, ['props', 'nestedObject', 'a']); - jest.runOnlyPendingTimers(); - }); - }); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot('2: Inspect props.nestedObject.a'); + async function getInspectedElement() { + await utils.actAsync( + () => + renderer.update( + + + + + , + ), + false, + ); + } - inspectedElement = null; - TestUtilsAct(() => { - TestRendererAct(() => { - getInspectedElementPath(id, ['props', 'nestedObject', 'a', 'b', 'c']); - jest.runOnlyPendingTimers(); - }); - }); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot( - '3: Inspect props.nestedObject.a.b.c', - ); + // Render once to get a handle on inspectElementPath() + await getInspectedElement(); - inspectedElement = null; - TestUtilsAct(() => { - TestRendererAct(() => { - getInspectedElementPath(id, [ - 'props', - 'nestedObject', - 'a', - 'b', - 'c', - 0, - 'd', - ]); - jest.runOnlyPendingTimers(); + async function loadPath(path) { + TestUtilsAct(() => { + TestRendererAct(() => { + inspectElementPath(path); + jest.runOnlyPendingTimers(); + }); }); - }); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot( - '4: Inspect props.nestedObject.a.b.c.0.d', - ); + await getInspectedElement(); + } - inspectedElement = null; - TestUtilsAct(() => { - TestRendererAct(() => { - getInspectedElementPath(id, ['hooks', 0, 'value']); - jest.runOnlyPendingTimers(); - }); - }); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot('5: Inspect hooks.0.value'); + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "nestedObject": Object { + "a": Dehydrated { + "preview_short": {…}, + "preview_long": {b: {…}}, + }, + }, + } + `); + + await loadPath(['props', 'nestedObject', 'a']); + + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "nestedObject": Object { + "a": Object { + "b": Object { + "c": Dehydrated { + "preview_short": Array(1), + "preview_long": [{…}], + }, + }, + }, + }, + } + `); + + await loadPath(['props', 'nestedObject', 'a', 'b', 'c']); + + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "nestedObject": Object { + "a": Object { + "b": Object { + "c": Array [ + Object { + "d": Dehydrated { + "preview_short": {…}, + "preview_long": {e: {…}}, + }, + }, + ], + }, + }, + }, + } + `); + + await loadPath(['props', 'nestedObject', 'a', 'b', 'c', 0, 'd']); + + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "nestedObject": Object { + "a": Object { + "b": Object { + "c": Array [ + Object { + "d": Object { + "e": Object {}, + }, + }, + ], + }, + }, + }, + } + `); - inspectedElement = null; - TestUtilsAct(() => { - TestRendererAct(() => { - getInspectedElementPath(id, ['hooks', 0, 'value', 'foo', 'bar']); - jest.runOnlyPendingTimers(); - }); - }); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot( - '6: Inspect hooks.0.value.foo.bar', - ); + await loadPath(['hooks', 0, 'value']); + + expect(inspectedElement.hooks).toMatchInlineSnapshot(` + Array [ + Object { + "id": 0, + "isStateEditable": true, + "name": "State", + "subHooks": Array [], + "value": Object { + "foo": Object { + "bar": Dehydrated { + "preview_short": {…}, + "preview_long": {baz: "hi"}, + }, + }, + }, + }, + ] + `); + + await loadPath(['hooks', 0, 'value', 'foo', 'bar']); + + expect(inspectedElement.hooks).toMatchInlineSnapshot(` + Array [ + Object { + "id": 0, + "isStateEditable": true, + "name": "State", + "subHooks": Array [], + "value": Object { + "foo": Object { + "bar": Object { + "baz": "hi", + }, + }, + }, + }, + ] + `); done(); }); @@ -1253,42 +1373,79 @@ describe('InspectedElementContext', () => { const id = ((store.getElementIDAtIndex(0): any): number); - let getInspectedElementPath: GetInspectedElementPath = ((null: any): GetInspectedElementPath); let inspectedElement = null; + let inspectElementPath = null; - function Suspender({target}) { - const context = React.useContext(InspectedElementContext); - getInspectedElementPath = context.getInspectedElementPath; - inspectedElement = context.getInspectedElement(target); + function Suspender({path, target}) { + inspectedElement = useInspectedElement(id); + inspectElementPath = useInspectElementPath(id); return null; } - await utils.actAsync( - () => - TestRenderer.create( - - - - - , - ), - false, - ); - expect(getInspectedElementPath).not.toBeNull(); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot('1: Initially inspect element'); + const renderer = TestRenderer.create(null); - inspectedElement = null; - TestUtilsAct(() => { - TestRendererAct(() => { - getInspectedElementPath(id, ['props', 'set_of_sets', 0]); - jest.runOnlyPendingTimers(); + async function getInspectedElement() { + await utils.actAsync( + () => + renderer.update( + + + + + , + ), + false, + ); + } + + // Render once to get a handle on inspectElementPath() + await getInspectedElement(); + + async function loadPath(path) { + TestUtilsAct(() => { + TestRendererAct(() => { + inspectElementPath(path); + jest.runOnlyPendingTimers(); + }); }); - }); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot('2: Inspect props.set_of_sets.0'); + await getInspectedElement(); + } + + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "set_of_sets": Object { + "0": Dehydrated { + "preview_short": Set(3), + "preview_long": Set(3) {1, 2, 3}, + }, + "1": Dehydrated { + "preview_short": Set(3), + "preview_long": Set(3) {"a", "b", "c"}, + }, + }, + } + `); + + await loadPath(['props', 'set_of_sets', 0]); + + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "set_of_sets": Object { + "0": Object { + "0": 1, + "1": 2, + "2": 3, + }, + "1": Object { + "0": "a", + "1": "b", + "2": "c", + }, + }, + } + `); done(); }); @@ -1324,48 +1481,107 @@ describe('InspectedElementContext', () => { const id = ((store.getElementIDAtIndex(0): any): number); - let getInspectedElementPath: GetInspectedElementPath = ((null: any): GetInspectedElementPath); let inspectedElement = null; + let inspectElementPath = null; - function Suspender({target}) { - const context = React.useContext(InspectedElementContext); - getInspectedElementPath = context.getInspectedElementPath; - inspectedElement = context.getInspectedElement(id); + function Suspender({path, target}) { + inspectedElement = useInspectedElement(id); + inspectElementPath = useInspectElementPath(id); return null; } - await utils.actAsync( - () => - TestRenderer.create( - - - - - , - ), - false, - ); - expect(getInspectedElementPath).not.toBeNull(); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot('1: Initially inspect element'); + const renderer = TestRenderer.create(null); - inspectedElement = null; - TestRendererAct(() => { - getInspectedElementPath(id, ['props', 'nestedObject', 'a']); - jest.runOnlyPendingTimers(); - }); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot('2: Inspect props.nestedObject.a'); + async function getInspectedElement() { + await utils.actAsync( + () => + renderer.update( + + + + + , + ), + false, + ); + } - inspectedElement = null; - TestRendererAct(() => { - getInspectedElementPath(id, ['props', 'nestedObject', 'c']); - jest.runOnlyPendingTimers(); - }); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot('3: Inspect props.nestedObject.c'); + // Render once to get a handle on inspectElementPath() + await getInspectedElement(); + + async function loadPath(path) { + TestUtilsAct(() => { + TestRendererAct(() => { + inspectElementPath(path); + jest.runOnlyPendingTimers(); + }); + }); + await getInspectedElement(); + } + + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "nestedObject": Object { + "a": Dehydrated { + "preview_short": {…}, + "preview_long": {b: {…}, value: 1}, + }, + "c": Dehydrated { + "preview_short": {…}, + "preview_long": {d: {…}, value: 1}, + }, + }, + } + `); + + await loadPath(['props', 'nestedObject', 'a']); + + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "nestedObject": Object { + "a": Object { + "b": Object { + "value": 1, + }, + "value": 1, + }, + "c": Object { + "d": Dehydrated { + "preview_short": {…}, + "preview_long": {e: {…}, value: 1}, + }, + "value": 1, + }, + }, + } + `); + + await loadPath(['props', 'nestedObject', 'c']); + + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "nestedObject": Object { + "a": Object { + "b": Object { + "value": 1, + }, + "value": 1, + }, + "c": Object { + "d": Object { + "e": Dehydrated { + "preview_short": {…}, + "preview_long": {value: 1}, + }, + "value": 1, + }, + "value": 1, + }, + }, + } + `); TestRendererAct(() => { TestUtilsAct(() => { @@ -1394,12 +1610,33 @@ describe('InspectedElementContext', () => { }); }); - TestRendererAct(() => { - inspectedElement = null; - jest.advanceTimersByTime(1000); - }); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot('4: update inspected element'); + // Wait for pending poll-for-update and then update inspected element data. + jest.runOnlyPendingTimers(); + await Promise.resolve(); + await getInspectedElement(); + + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "nestedObject": Object { + "a": Object { + "b": Object { + "value": 2, + }, + "value": 2, + }, + "c": Object { + "d": Object { + "e": Dehydrated { + "preview_short": {…}, + "preview_long": {value: 2}, + }, + "value": 2, + }, + "value": 2, + }, + }, + } + `); done(); }); @@ -1427,32 +1664,57 @@ describe('InspectedElementContext', () => { const id = ((store.getElementIDAtIndex(0): any): number); - let getInspectedElementPath: GetInspectedElementPath = ((null: any): GetInspectedElementPath); let inspectedElement = null; + let inspectElementPath = null; - function Suspender({target}) { - const context = React.useContext(InspectedElementContext); - getInspectedElementPath = context.getInspectedElementPath; - inspectedElement = context.getInspectedElement(id); + function Suspender({path, target}) { + inspectedElement = useInspectedElement(id); + inspectElementPath = useInspectElementPath(id); return null; } - await utils.actAsync( - () => - TestRenderer.create( - - - - - , - ), - false, - ); - expect(getInspectedElementPath).not.toBeNull(); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot('1: Initially inspect element'); + const renderer = TestRenderer.create(null); + + async function getInspectedElement() { + await utils.actAsync( + () => + renderer.update( + + + + + , + ), + false, + ); + } + + // Render once to get a handle on inspectElementPath() + await getInspectedElement(); + + async function loadPath(path) { + TestUtilsAct(() => { + TestRendererAct(() => { + inspectElementPath(path); + jest.runOnlyPendingTimers(); + }); + }); + await getInspectedElement(); + } + + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "nestedObject": Object { + "a": Dehydrated { + "preview_short": {…}, + "preview_long": {b: {…}, value: 1}, + }, + "value": 1, + }, + } + `); TestUtilsAct(() => { ReactDOM.render( @@ -1471,16 +1733,21 @@ describe('InspectedElementContext', () => { ); }); - inspectedElement = null; + await loadPath(['props', 'nestedObject', 'a']); - TestRendererAct(() => { - TestUtilsAct(() => { - getInspectedElementPath(id, ['props', 'nestedObject', 'a']); - jest.runOnlyPendingTimers(); - }); - }); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot('2: Inspect props.nestedObject.a'); + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "nestedObject": Object { + "a": Object { + "b": Object { + "value": 2, + }, + "value": 2, + }, + "value": 2, + }, + } + `); done(); }); @@ -1502,9 +1769,29 @@ describe('InspectedElementContext', () => { let didFinish = false; function Suspender({target}) { - const {getInspectedElement} = React.useContext(InspectedElementContext); - const inspectedElement = getInspectedElement(id); - expect(inspectedElement).toMatchSnapshot(`1: Inspected element ${id}`); + const inspectedElement = useInspectedElement(id); + expect(inspectedElement).toMatchInlineSnapshot(` + Object { + "context": null, + "events": undefined, + "hooks": Array [ + Object { + "id": null, + "isStateEditable": false, + "name": "Context", + "subHooks": Array [], + "value": true, + }, + ], + "id": 2, + "owners": null, + "props": Object { + "a": 1, + "b": "abc", + }, + "state": null, + } + `); didFinish = true; return null; } @@ -1554,8 +1841,20 @@ describe('InspectedElementContext', () => { let storeAsGlobal: StoreAsGlobal = ((null: any): StoreAsGlobal); function Suspender({target}) { - const context = React.useContext(InspectedElementContext); - storeAsGlobal = context.storeAsGlobal; + storeAsGlobal = (elementID: number, path: Array) => { + const rendererID = store.getRendererIDForElement(elementID); + if (rendererID !== null) { + const { + storeAsGlobal: storeAsGlobalAPI, + } = require('react-devtools-shared/src/backendAPI'); + storeAsGlobalAPI({ + bridge, + id: elementID, + path, + rendererID, + }); + } + }; return null; } @@ -1572,23 +1871,22 @@ describe('InspectedElementContext', () => { ), false, ); - expect(storeAsGlobal).not.toBeNull(); jest.spyOn(console, 'log').mockImplementation(() => {}); // Should store the whole value (not just the hydrated parts) storeAsGlobal(id, ['props', 'nestedObject']); jest.runOnlyPendingTimers(); - expect(console.log).toHaveBeenCalledWith('$reactTemp1'); - expect(global.$reactTemp1).toBe(nestedObject); + expect(console.log).toHaveBeenCalledWith('$reactTemp0'); + expect(global.$reactTemp0).toBe(nestedObject); console.log.mockReset(); // Should store the nested property specified (not just the outer value) storeAsGlobal(id, ['props', 'nestedObject', 'a', 'b']); jest.runOnlyPendingTimers(); - expect(console.log).toHaveBeenCalledWith('$reactTemp2'); - expect(global.$reactTemp2).toBe(nestedObject.a.b); + expect(console.log).toHaveBeenCalledWith('$reactTemp1'); + expect(global.$reactTemp1).toBe(nestedObject.a.b); done(); }); @@ -1620,8 +1918,20 @@ describe('InspectedElementContext', () => { let copyPath: CopyInspectedElementPath = ((null: any): CopyInspectedElementPath); function Suspender({target}) { - const context = React.useContext(InspectedElementContext); - copyPath = context.copyInspectedElementPath; + copyPath = (elementID: number, path: Array) => { + const rendererID = store.getRendererIDForElement(elementID); + if (rendererID !== null) { + const { + copyInspectedElementPath, + } = require('react-devtools-shared/src/backendAPI'); + copyInspectedElementPath({ + bridge, + id: elementID, + path, + rendererID, + }); + } + }; return null; } @@ -1712,8 +2022,20 @@ describe('InspectedElementContext', () => { let copyPath: CopyInspectedElementPath = ((null: any): CopyInspectedElementPath); function Suspender({target}) { - const context = React.useContext(InspectedElementContext); - copyPath = context.copyInspectedElementPath; + copyPath = (elementID: number, path: Array) => { + const rendererID = store.getRendererIDForElement(elementID); + if (rendererID !== null) { + const { + copyInspectedElementPath, + } = require('react-devtools-shared/src/backendAPI'); + copyInspectedElementPath({ + bridge, + id: elementID, + path, + rendererID, + }); + } + }; return null; } @@ -1761,12 +2083,9 @@ describe('InspectedElementContext', () => { }); it('should display complex values of useDebugValue', async done => { - let getInspectedElementPath: GetInspectedElementPath = ((null: any): GetInspectedElementPath); let inspectedElement = null; function Suspender({target}) { - const context = React.useContext(InspectedElementContext); - getInspectedElementPath = context.getInspectedElementPath; - inspectedElement = context.getInspectedElement(target); + inspectedElement = useInspectedElement(target); return null; } @@ -1803,9 +2122,27 @@ describe('InspectedElementContext', () => { ), false, ); - expect(getInspectedElementPath).not.toBeNull(); - expect(inspectedElement).not.toBeNull(); - expect(inspectedElement).toMatchSnapshot('DisplayedComplexValue'); + expect(inspectedElement.hooks).toMatchInlineSnapshot(` + Array [ + Object { + "id": null, + "isStateEditable": false, + "name": "DebuggableHook", + "subHooks": Array [ + Object { + "id": 0, + "isStateEditable": true, + "name": "State", + "subHooks": Array [], + "value": 1, + }, + ], + "value": Object { + "foo": 2, + }, + }, + ] + `); done(); }); @@ -1825,8 +2162,7 @@ describe('InspectedElementContext', () => { let warnings = null; function Suspender({target}) { - const {getInspectedElement} = React.useContext(InspectedElementContext); - const inspectedElement = getInspectedElement(id); + const inspectedElement = useInspectedElement(id); errors = inspectedElement.errors; warnings = inspectedElement.warnings; return null; @@ -2038,7 +2374,11 @@ describe('InspectedElementContext', () => { ); }); - store.clearErrorsAndWarnings(); + const { + clearErrorsAndWarnings, + } = require('react-devtools-shared/src/backendAPI'); + clearErrorsAndWarnings({bridge, store}); + // Flush events to the renderer. jest.runOnlyPendingTimers(); @@ -2051,7 +2391,7 @@ describe('InspectedElementContext', () => { `); }); - it('can be cleared for a particular Fiber (only errors)', async () => { + it('can be cleared for a particular Fiber (only warnings)', async () => { const Example = ({id}) => { console.error(`test-only: render error #${id}`); console.warn(`test-only: render warning #${id}`); @@ -2071,7 +2411,14 @@ describe('InspectedElementContext', () => { ); }); - store.clearWarningsForElement(2); + let id = ((store.getElementIDAtIndex(1): any): number); + const rendererID = store.getRendererIDForElement(id); + + const { + clearWarningsForElement, + } = require('react-devtools-shared/src/backendAPI'); + clearWarningsForElement({bridge, id, rendererID}); + // Flush events to the renderer. jest.runOnlyPendingTimers(); @@ -2107,7 +2454,9 @@ describe('InspectedElementContext', () => { ] `); - store.clearWarningsForElement(1); + id = ((store.getElementIDAtIndex(0): any): number); + clearWarningsForElement({bridge, id, rendererID}); + // Flush events to the renderer. jest.runOnlyPendingTimers(); @@ -2139,7 +2488,7 @@ describe('InspectedElementContext', () => { `); }); - it('can be cleared for a particular Fiber (only warnings)', async () => { + it('can be cleared for a particular Fiber (only errors)', async () => { const Example = ({id}) => { console.error(`test-only: render error #${id}`); console.warn(`test-only: render warning #${id}`); @@ -2159,7 +2508,14 @@ describe('InspectedElementContext', () => { ); }); - store.clearErrorsForElement(2); + let id = ((store.getElementIDAtIndex(1): any): number); + const rendererID = store.getRendererIDForElement(id); + + const { + clearErrorsForElement, + } = require('react-devtools-shared/src/backendAPI'); + clearErrorsForElement({bridge, id, rendererID}); + // Flush events to the renderer. jest.runOnlyPendingTimers(); @@ -2195,7 +2551,9 @@ describe('InspectedElementContext', () => { ] `); - store.clearErrorsForElement(1); + id = ((store.getElementIDAtIndex(0): any): number); + clearErrorsForElement({bridge, id, rendererID}); + // Flush events to the renderer. jest.runOnlyPendingTimers(); diff --git a/packages/react-devtools-shared/src/__tests__/inspectedElementSerializer.js b/packages/react-devtools-shared/src/__tests__/inspectedElementSerializer.js index b6290daf61a95..a615777421fe3 100644 --- a/packages/react-devtools-shared/src/__tests__/inspectedElementSerializer.js +++ b/packages/react-devtools-shared/src/__tests__/inspectedElementSerializer.js @@ -12,17 +12,14 @@ export function test(maybeInspectedElement) { // print() is part of Jest's serializer API export function print(inspectedElement, serialize, indent) { - return JSON.stringify( - { - id: inspectedElement.id, - owners: inspectedElement.owners, - context: inspectedElement.context, - events: inspectedElement.events, - hooks: inspectedElement.hooks, - props: inspectedElement.props, - state: inspectedElement.state, - }, - null, - 2, - ); + // Don't stringify this object; that would break nested serializers. + return serialize({ + context: inspectedElement.context, + events: inspectedElement.events, + hooks: inspectedElement.hooks, + id: inspectedElement.id, + owners: inspectedElement.owners, + props: inspectedElement.props, + state: inspectedElement.state, + }); } diff --git a/packages/react-devtools-shared/src/__tests__/legacy/__snapshots__/inspectElement-test.js.snap b/packages/react-devtools-shared/src/__tests__/legacy/__snapshots__/inspectElement-test.js.snap deleted file mode 100644 index 1ed17c109bb12..0000000000000 --- a/packages/react-devtools-shared/src/__tests__/legacy/__snapshots__/inspectElement-test.js.snap +++ /dev/null @@ -1,303 +0,0 @@ -// Jest Snapshot v1, https://goo.gl/fbAQLP - -exports[`InspectedElementContext should inspect the currently selected element: 1: Initial inspection 1`] = ` -Object { - "id": 2, - "type": "full-data", - "value": { - "id": 2, - "owners": null, - "context": {}, - "hooks": null, - "props": { - "a": 1, - "b": "abc" - }, - "state": null -}, -} -`; - -exports[`InspectedElementContext should not consume iterables while inspecting: 1: Initial inspection 1`] = ` -Object { - "id": 2, - "type": "full-data", - "value": { - "id": 2, - "owners": null, - "context": {}, - "hooks": null, - "props": { - "iteratable": {} - }, - "state": null -}, -} -`; - -exports[`InspectedElementContext should not dehydrate nested values until explicitly requested: 1: Initially inspect element 1`] = ` -Object { - "id": 2, - "type": "full-data", - "value": { - "id": 2, - "owners": null, - "context": {}, - "hooks": null, - "props": { - "nestedObject": { - "a": {} - } - }, - "state": null -}, -} -`; - -exports[`InspectedElementContext should not dehydrate nested values until explicitly requested: 2: Inspect props.nestedObject.a 1`] = ` -Object { - "id": 2, - "type": "full-data", - "value": { - "id": 2, - "owners": null, - "context": {}, - "hooks": null, - "props": { - "nestedObject": { - "a": { - "b": { - "c": {} - } - } - } - }, - "state": null -}, -} -`; - -exports[`InspectedElementContext should not dehydrate nested values until explicitly requested: 3: Inspect props.nestedObject.a.b.c 1`] = ` -Object { - "id": 2, - "type": "full-data", - "value": { - "id": 2, - "owners": null, - "context": {}, - "hooks": null, - "props": { - "nestedObject": { - "a": { - "b": { - "c": [ - { - "d": {} - } - ] - } - } - } - }, - "state": null -}, -} -`; - -exports[`InspectedElementContext should not dehydrate nested values until explicitly requested: 4: Inspect props.nestedObject.a.b.c.0.d 1`] = ` -Object { - "id": 2, - "type": "full-data", - "value": { - "id": 2, - "owners": null, - "context": {}, - "hooks": null, - "props": { - "nestedObject": { - "a": { - "b": { - "c": [ - { - "d": { - "e": {} - } - } - ] - } - } - } - }, - "state": null -}, -} -`; - -exports[`InspectedElementContext should support complex data types: 1: Initial inspection 1`] = ` -Object { - "id": 2, - "type": "full-data", - "value": { - "id": 2, - "owners": null, - "context": {}, - "hooks": null, - "props": { - "anonymous_fn": {}, - "array_buffer": {}, - "array_of_arrays": [ - {} - ], - "big_int": {}, - "bound_fn": {}, - "data_view": {}, - "date": {}, - "fn": {}, - "html_element": {}, - "immutable": { - "0": {}, - "1": {}, - "2": {} - }, - "map": { - "0": {}, - "1": {} - }, - "map_of_maps": { - "0": {}, - "1": {} - }, - "object_of_objects": { - "inner": {} - }, - "react_element": {}, - "regexp": {}, - "set": { - "0": "abc", - "1": 123 - }, - "set_of_sets": { - "0": {}, - "1": {} - }, - "symbol": {}, - "typed_array": { - "0": 100, - "1": -100, - "2": 0 - } - }, - "state": null -}, -} -`; - -exports[`InspectedElementContext should support custom objects with enumerable properties and getters: 1: Initial inspection 1`] = ` -Object { - "id": 2, - "type": "full-data", - "value": { - "id": 2, - "owners": null, - "context": {}, - "hooks": null, - "props": { - "data": { - "_number": 42, - "number": 42 - } - }, - "state": null -}, -} -`; - -exports[`InspectedElementContext should support objects with no prototype: 1: Initial inspection 1`] = ` -Object { - "id": 2, - "type": "full-data", - "value": { - "id": 2, - "owners": null, - "context": {}, - "hooks": null, - "props": { - "object": { - "string": "abc", - "number": 123, - "boolean": true - } - }, - "state": null -}, -} -`; - -exports[`InspectedElementContext should support objects with overridden hasOwnProperty: 1: Initial inspection 1`] = ` -Object { - "id": 2, - "type": "full-data", - "value": { - "id": 2, - "owners": null, - "context": {}, - "hooks": null, - "props": { - "object": { - "name": "blah", - "hasOwnProperty": true - } - }, - "state": null -}, -} -`; - -exports[`InspectedElementContext should support objects with with inherited keys: 1: Initial inspection 1`] = ` -Object { - "id": 2, - "type": "full-data", - "value": { - "id": 2, - "owners": null, - "context": {}, - "hooks": null, - "props": { - "data": { - "123": 3, - "enumerableString": 2, - "Symbol(enumerableSymbol)": 3, - "enumerableStringBase": 1, - "Symbol(enumerableSymbolBase)": 1 - } - }, - "state": null -}, -} -`; - -exports[`InspectedElementContext should support simple data types: 1: Initial inspection 1`] = ` -Object { - "id": 2, - "type": "full-data", - "value": { - "id": 2, - "owners": null, - "context": {}, - "hooks": null, - "props": { - "boolean_false": false, - "boolean_true": true, - "infinity": null, - "integer_zero": 0, - "integer_one": 1, - "float": 1.23, - "string": "abc", - "string_empty": "", - "nan": null, - "value_null": null - }, - "state": null -}, -} -`; diff --git a/packages/react-devtools-shared/src/__tests__/legacy/inspectElement-test.js b/packages/react-devtools-shared/src/__tests__/legacy/inspectElement-test.js index 6ccff70c77772..05d2bdcbeb6eb 100644 --- a/packages/react-devtools-shared/src/__tests__/legacy/inspectElement-test.js +++ b/packages/react-devtools-shared/src/__tests__/legacy/inspectElement-test.js @@ -7,71 +7,50 @@ * @flow */ -import type {InspectedElementPayload} from 'react-devtools-shared/src/backend/types'; -import type {DehydratedData} from 'react-devtools-shared/src/devtools/views/Components/types'; import type {FrontendBridge} from 'react-devtools-shared/src/bridge'; import type Store from 'react-devtools-shared/src/devtools/store'; describe('InspectedElementContext', () => { let React; let ReactDOM; - let hydrate; - let meta; let bridge: FrontendBridge; let store: Store; + let backendAPI; + const act = (callback: Function) => { callback(); jest.runAllTimers(); // Flush Bridge operations }; - function dehydrateHelper( - dehydratedData: DehydratedData | null, - ): Object | null { - if (dehydratedData !== null) { - return hydrate( - dehydratedData.data, - dehydratedData.cleaned, - dehydratedData.unserializable, - ); - } else { - return null; - } - } - async function read( id: number, - path?: Array, + inspectedPaths?: Object = {}, ): Promise { - return new Promise((resolve, reject) => { - const rendererID = ((store.getRendererIDForElement(id): any): number); - - const onInspectedElement = (payload: InspectedElementPayload) => { - bridge.removeListener('inspectedElement', onInspectedElement); - - if (payload.type === 'full-data' && payload.value !== null) { - payload.value.context = dehydrateHelper(payload.value.context); - payload.value.props = dehydrateHelper(payload.value.props); - payload.value.state = dehydrateHelper(payload.value.state); - } - - resolve(payload); - }; + const rendererID = ((store.getRendererIDForElement(id): any): number); + const promise = backendAPI + .inspectElement({ + bridge, + forceUpdate: true, + id, + inspectedPaths, + rendererID, + }) + .then(data => + backendAPI.convertInspectedElementBackendToFrontend(data.value), + ); - bridge.addListener('inspectedElement', onInspectedElement); - bridge.send('inspectElement', {id, path, rendererID}); + jest.runOnlyPendingTimers(); - jest.runOnlyPendingTimers(); - }); + return promise; } beforeEach(() => { bridge = global.bridge; store = global.store; - hydrate = require('react-devtools-shared/src/hydration').hydrate; - meta = require('react-devtools-shared/src/hydration').meta; + backendAPI = require('react-devtools-shared/src/backendAPI'); // Redirect all React/ReactDOM requires to the v15 UMD. // We use the UMD because Jest doesn't enable us to mock deep imports (e.g. "react/lib/Something"). @@ -94,7 +73,20 @@ describe('InspectedElementContext', () => { const id = ((store.getElementIDAtIndex(0): any): number); const inspectedElement = await read(id); - expect(inspectedElement).toMatchSnapshot('1: Initial inspection'); + expect(inspectedElement).toMatchInlineSnapshot(` + Object { + "context": Object {}, + "events": undefined, + "hooks": null, + "id": 2, + "owners": null, + "props": Object { + "a": 1, + "b": "abc", + }, + "state": null, + } + `); done(); }); @@ -124,20 +116,29 @@ describe('InspectedElementContext', () => { const id = ((store.getElementIDAtIndex(0): any): number); const inspectedElement = await read(id); - expect(inspectedElement).toMatchSnapshot('1: Initial inspection'); - - const {props} = inspectedElement.value; - expect(props.boolean_false).toBe(false); - expect(props.boolean_true).toBe(true); - expect(Number.isFinite(props.infinity)).toBe(false); - expect(props.integer_zero).toEqual(0); - expect(props.integer_one).toEqual(1); - expect(props.float).toEqual(1.23); - expect(props.string).toEqual('abc'); - expect(props.string_empty).toEqual(''); - expect(props.nan).toBeNaN(); - expect(props.value_null).toBeNull(); - expect(props.value_undefined).toBeUndefined(); + expect(inspectedElement).toMatchInlineSnapshot(` + Object { + "context": Object {}, + "events": undefined, + "hooks": null, + "id": 2, + "owners": null, + "props": Object { + "boolean_false": false, + "boolean_true": true, + "float": 1.23, + "infinity": Infinity, + "integer_one": 1, + "integer_zero": 0, + "nan": NaN, + "string": "abc", + "string_empty": "", + "value_null": null, + "value_undefined": undefined, + }, + "state": null, + } + `); done(); }); @@ -211,8 +212,6 @@ describe('InspectedElementContext', () => { const id = ((store.getElementIDAtIndex(0): any): number); const inspectedElement = await read(id); - expect(inspectedElement).toMatchSnapshot('1: Initial inspection'); - const { anonymous_fn, array_buffer, @@ -233,7 +232,9 @@ describe('InspectedElementContext', () => { set_of_sets, symbol, typed_array, - } = inspectedElement.value.props; + } = inspectedElement.props; + + const {meta} = require('react-devtools-shared/src/hydration'); expect(anonymous_fn[meta.inspectable]).toBe(false); expect(anonymous_fn[meta.name]).toBe('function'); @@ -360,12 +361,15 @@ describe('InspectedElementContext', () => { const id = ((store.getElementIDAtIndex(0): any): number); const inspectedElement = await read(id); - expect(inspectedElement).toMatchSnapshot('1: Initial inspection'); - expect(inspectedElement.value.props.object).toEqual({ - boolean: true, - number: 123, - string: 'abc', - }); + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "object": Object { + "boolean": true, + "number": 123, + "string": "abc", + }, + } + `); done(); }); @@ -388,11 +392,10 @@ describe('InspectedElementContext', () => { const id = ((store.getElementIDAtIndex(0): any): number); const inspectedElement = await read(id); - expect(inspectedElement).toMatchSnapshot('1: Initial inspection'); - expect(inspectedElement.value.props.object).toEqual({ - name: 'blah', - hasOwnProperty: true, - }); + // TRICKY: Don't use toMatchInlineSnapshot() for this test! + // Our snapshot serializer relies on hasOwnProperty() for feature detection. + expect(inspectedElement.props.object.name).toBe('blah'); + expect(inspectedElement.props.object.hasOwnProperty).toBe(true); done(); }); @@ -417,7 +420,22 @@ describe('InspectedElementContext', () => { const id = ((store.getElementIDAtIndex(0): any): number); const inspectedElement = await read(id); - expect(inspectedElement).toMatchSnapshot('1: Initial inspection'); + expect(inspectedElement).toMatchInlineSnapshot(` + Object { + "context": Object {}, + "events": undefined, + "hooks": null, + "id": 2, + "owners": null, + "props": Object { + "iteratable": Dehydrated { + "preview_short": Generator, + "preview_long": Generator, + }, + }, + "state": null, + } + `); // Inspecting should not consume the iterable. expect(iteratable.next().value).toEqual(1); @@ -457,7 +475,22 @@ describe('InspectedElementContext', () => { const id = ((store.getElementIDAtIndex(0): any): number); const inspectedElement = await read(id); - expect(inspectedElement).toMatchSnapshot('1: Initial inspection'); + expect(inspectedElement).toMatchInlineSnapshot(` + Object { + "context": Object {}, + "events": undefined, + "hooks": null, + "id": 2, + "owners": null, + "props": Object { + "data": Object { + "_number": 42, + "number": 42, + }, + }, + "state": null, + } + `); done(); }); @@ -532,7 +565,25 @@ describe('InspectedElementContext', () => { const id = ((store.getElementIDAtIndex(0): any): number); const inspectedElement = await read(id); - expect(inspectedElement).toMatchSnapshot('1: Initial inspection'); + expect(inspectedElement).toMatchInlineSnapshot(` + Object { + "context": Object {}, + "events": undefined, + "hooks": null, + "id": 2, + "owners": null, + "props": Object { + "data": Object { + "123": 3, + "Symbol(enumerableSymbol)": 3, + "Symbol(enumerableSymbolBase)": 1, + "enumerableString": 2, + "enumerableStringBase": 1, + }, + }, + "state": null, + } + `); done(); }); @@ -564,28 +615,75 @@ describe('InspectedElementContext', () => { const id = ((store.getElementIDAtIndex(0): any): number); let inspectedElement = await read(id); - expect(inspectedElement).toMatchSnapshot('1: Initially inspect element'); - - inspectedElement = await read(id, ['props', 'nestedObject', 'a']); - expect(inspectedElement).toMatchSnapshot('2: Inspect props.nestedObject.a'); + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "nestedObject": Object { + "a": Dehydrated { + "preview_short": {…}, + "preview_long": {b: {…}}, + }, + }, + } + `); + + inspectedElement = await read(id, {props: {nestedObject: {a: {}}}}); + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "nestedObject": Object { + "a": Object { + "b": Object { + "c": Dehydrated { + "preview_short": Array(1), + "preview_long": [{…}], + }, + }, + }, + }, + } + `); - inspectedElement = await read(id, ['props', 'nestedObject', 'a', 'b', 'c']); - expect(inspectedElement).toMatchSnapshot( - '3: Inspect props.nestedObject.a.b.c', - ); + inspectedElement = await read(id, { + props: {nestedObject: {a: {b: {c: {}}}}}, + }); + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "nestedObject": Object { + "a": Object { + "b": Object { + "c": Array [ + Object { + "d": Dehydrated { + "preview_short": {…}, + "preview_long": {e: {…}}, + }, + }, + ], + }, + }, + }, + } + `); - inspectedElement = await read(id, [ - 'props', - 'nestedObject', - 'a', - 'b', - 'c', - 0, - 'd', - ]); - expect(inspectedElement).toMatchSnapshot( - '4: Inspect props.nestedObject.a.b.c.0.d', - ); + inspectedElement = await read(id, { + props: {nestedObject: {a: {b: {c: {0: {d: {}}}}}}}, + }); + expect(inspectedElement.props).toMatchInlineSnapshot(` + Object { + "nestedObject": Object { + "a": Object { + "b": Object { + "c": Array [ + Object { + "d": Object { + "e": Object {}, + }, + }, + ], + }, + }, + }, + } + `); done(); }); @@ -619,28 +717,30 @@ describe('InspectedElementContext', () => { spyOn(console, 'log').and.callFake(logSpy); // Should store the whole value (not just the hydrated parts) - bridge.send('storeAsGlobal', { - count: 1, + backendAPI.storeAsGlobal({ + bridge, id, path: ['props', 'nestedObject'], rendererID, }); + jest.runOnlyPendingTimers(); - expect(logSpy).toHaveBeenCalledWith('$reactTemp1'); - expect(global.$reactTemp1).toBe(nestedObject); + expect(logSpy).toHaveBeenCalledWith('$reactTemp0'); + expect(global.$reactTemp0).toBe(nestedObject); logSpy.mockReset(); // Should store the nested property specified (not just the outer value) - bridge.send('storeAsGlobal', { - count: 2, + backendAPI.storeAsGlobal({ + bridge, id, path: ['props', 'nestedObject', 'a', 'b'], rendererID, }); + jest.runOnlyPendingTimers(); - expect(logSpy).toHaveBeenCalledWith('$reactTemp2'); - expect(global.$reactTemp2).toBe(nestedObject.a.b); + expect(logSpy).toHaveBeenCalledWith('$reactTemp1'); + expect(global.$reactTemp1).toBe(nestedObject.a.b); }); it('should enable inspected values to be copied to the clipboard', () => { @@ -669,11 +769,13 @@ describe('InspectedElementContext', () => { const rendererID = ((store.getRendererIDForElement(id): any): number); // Should copy the whole value (not just the hydrated parts) - bridge.send('copyElementPath', { + backendAPI.copyInspectedElementPath({ + bridge, id, path: ['props', 'nestedObject'], rendererID, }); + jest.runOnlyPendingTimers(); expect(global.mockClipboardCopy).toHaveBeenCalledTimes(1); expect(global.mockClipboardCopy).toHaveBeenCalledWith( @@ -683,11 +785,13 @@ describe('InspectedElementContext', () => { global.mockClipboardCopy.mockReset(); // Should copy the nested property specified (not just the outer value) - bridge.send('copyElementPath', { + backendAPI.copyInspectedElementPath({ + bridge, id, path: ['props', 'nestedObject', 'a', 'b'], rendererID, }); + jest.runOnlyPendingTimers(); expect(global.mockClipboardCopy).toHaveBeenCalledTimes(1); expect(global.mockClipboardCopy).toHaveBeenCalledWith( @@ -745,7 +849,8 @@ describe('InspectedElementContext', () => { const rendererID = ((store.getRendererIDForElement(id): any): number); // Should copy the whole value (not just the hydrated parts) - bridge.send('copyElementPath', { + backendAPI.copyInspectedElementPath({ + bridge, id, path: ['props'], rendererID, @@ -756,7 +861,8 @@ describe('InspectedElementContext', () => { global.mockClipboardCopy.mockReset(); // Should copy the nested property specified (not just the outer value) - bridge.send('copyElementPath', { + backendAPI.copyInspectedElementPath({ + bridge, id, path: ['props', 'bigInt'], rendererID, @@ -770,7 +876,8 @@ describe('InspectedElementContext', () => { global.mockClipboardCopy.mockReset(); // Should copy the nested property specified (not just the outer value) - bridge.send('copyElementPath', { + backendAPI.copyInspectedElementPath({ + bridge, id, path: ['props', 'typedArray'], rendererID, diff --git a/packages/react-devtools-shared/src/__tests__/store-test.js b/packages/react-devtools-shared/src/__tests__/store-test.js index d219f3cba7def..d5b9c3cbc9213 100644 --- a/packages/react-devtools-shared/src/__tests__/store-test.js +++ b/packages/react-devtools-shared/src/__tests__/store-test.js @@ -12,12 +12,14 @@ describe('Store', () => { let ReactDOM; let agent; let act; + let bridge; let getRendererID; let store; let withErrorsOrWarningsIgnored; beforeEach(() => { agent = global.agent; + bridge = global.bridge; store = global.store; React = require('react'); @@ -1159,7 +1161,11 @@ describe('Store', () => { ✕⚠ `); - store.clearErrorsAndWarnings(); + const { + clearErrorsAndWarnings, + } = require('react-devtools-shared/src/backendAPI'); + clearErrorsAndWarnings({bridge, store}); + // flush events to the renderer jest.runAllTimers(); @@ -1196,7 +1202,14 @@ describe('Store', () => { ✕⚠ `); - store.clearWarningsForElement(2); + const id = ((store.getElementIDAtIndex(1): any): number); + const rendererID = store.getRendererIDForElement(id); + + const { + clearWarningsForElement, + } = require('react-devtools-shared/src/backendAPI'); + clearWarningsForElement({bridge, id, rendererID}); + // Flush events to the renderer. jest.runAllTimers(); @@ -1234,7 +1247,14 @@ describe('Store', () => { ✕⚠ `); - store.clearErrorsForElement(2); + const id = ((store.getElementIDAtIndex(1): any): number); + const rendererID = store.getRendererIDForElement(id); + + const { + clearErrorsForElement, + } = require('react-devtools-shared/src/backendAPI'); + clearErrorsForElement({bridge, id, rendererID}); + // Flush events to the renderer. jest.runAllTimers(); diff --git a/packages/react-devtools-shared/src/__tests__/treeContext-test.js b/packages/react-devtools-shared/src/__tests__/treeContext-test.js index 8c057fb720807..36f615c1155b6 100644 --- a/packages/react-devtools-shared/src/__tests__/treeContext-test.js +++ b/packages/react-devtools-shared/src/__tests__/treeContext-test.js @@ -1441,20 +1441,28 @@ describe('TreeListContext', () => { }); describe('inline errors/warnings state', () => { + const { + clearErrorsAndWarnings: clearErrorsAndWarningsAPI, + clearErrorsForElement: clearErrorsForElementAPI, + clearWarningsForElement: clearWarningsForElementAPI, + } = require('react-devtools-shared/src/backendAPI'); + function clearAllErrors() { - utils.act(() => store.clearErrorsAndWarnings()); + utils.act(() => clearErrorsAndWarningsAPI({bridge, store})); // flush events to the renderer jest.runAllTimers(); } function clearErrorsForElement(id) { - utils.act(() => store.clearErrorsForElement(id)); + const rendererID = store.getRendererIDForElement(id); + utils.act(() => clearErrorsForElementAPI({bridge, id, rendererID})); // flush events to the renderer jest.runAllTimers(); } function clearWarningsForElement(id) { - utils.act(() => store.clearWarningsForElement(id)); + const rendererID = store.getRendererIDForElement(id); + utils.act(() => clearWarningsForElementAPI({bridge, id, rendererID})); // flush events to the renderer jest.runAllTimers(); } diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js index 326b0fb2e340f..e570febf5d67e 100644 --- a/packages/react-devtools-shared/src/backend/agent.js +++ b/packages/react-devtools-shared/src/backend/agent.js @@ -70,8 +70,10 @@ type CopyElementParams = {| type InspectElementParams = {| id: number, - path?: Array, + inspectedPaths: Object, + forceUpdate: boolean, rendererID: number, + requestID: number, |}; type OverrideHookParams = {| @@ -328,12 +330,21 @@ export default class Agent extends EventEmitter<{| } }; - inspectElement = ({id, path, rendererID}: InspectElementParams) => { + inspectElement = ({ + id, + inspectedPaths, + forceUpdate, + rendererID, + requestID, + }: InspectElementParams) => { const renderer = this._rendererInterfaces[rendererID]; if (renderer == null) { console.warn(`Invalid renderer id "${rendererID}" for element "${id}"`); } else { - this._bridge.send('inspectedElement', renderer.inspectElement(id, path)); + this._bridge.send( + 'inspectedElement', + renderer.inspectElement(requestID, id, inspectedPaths, forceUpdate), + ); // When user selects an element, stop trying to restore the selection, // and instead remember the current selection for the next reload. diff --git a/packages/react-devtools-shared/src/backend/legacy/renderer.js b/packages/react-devtools-shared/src/backend/legacy/renderer.js index 90f0f3ad12b46..63c9b1660e75e 100644 --- a/packages/react-devtools-shared/src/backend/legacy/renderer.js +++ b/packages/react-devtools-shared/src/backend/legacy/renderer.js @@ -584,25 +584,12 @@ export function attach( } let currentlyInspectedElementID: number | null = null; - let currentlyInspectedPaths: Object = {}; - - // Track the intersection of currently inspected paths, - // so that we can send their data along if the element is re-rendered. - function mergeInspectedPaths(path: Array) { - let current = currentlyInspectedPaths; - path.forEach(key => { - if (!current[key]) { - current[key] = {}; - } - current = current[key]; - }); - } - function createIsPathAllowed(key: string) { + function createIsPathAllowed(key: string, inspectedPaths: Object) { // This function helps prevent previously-inspected paths from being dehydrated in updates. // This is important to avoid a bad user experience where expanded toggles collapse on update. return function isPathAllowed(path: Array): boolean { - let current = currentlyInspectedPaths[key]; + let current = inspectedPaths[key]; if (!current) { return false; } @@ -691,26 +678,23 @@ export function attach( } function inspectElement( + requestID: number, id: number, - path?: Array, + inspectedPaths: Object, ): InspectedElementPayload { if (currentlyInspectedElementID !== id) { currentlyInspectedElementID = id; - currentlyInspectedPaths = {}; } const inspectedElement = inspectElementRaw(id); if (inspectedElement === null) { return { id, + responseID: requestID, type: 'not-found', }; } - if (path != null) { - mergeInspectedPaths(path); - } - // Any time an inspected element has an update, // we should update the selected $r value as wel. // Do this before dehyration (cleanForBridge). @@ -718,19 +702,20 @@ export function attach( inspectedElement.context = cleanForBridge( inspectedElement.context, - createIsPathAllowed('context'), + createIsPathAllowed('context', inspectedPaths), ); inspectedElement.props = cleanForBridge( inspectedElement.props, - createIsPathAllowed('props'), + createIsPathAllowed('props', inspectedPaths), ); inspectedElement.state = cleanForBridge( inspectedElement.state, - createIsPathAllowed('state'), + createIsPathAllowed('state', inspectedPaths), ); return { id, + responseID: requestID, type: 'full-data', value: inspectedElement, }; diff --git a/packages/react-devtools-shared/src/backend/renderer.js b/packages/react-devtools-shared/src/backend/renderer.js index b942b7561b7b8..ee90dfee688ac 100644 --- a/packages/react-devtools-shared/src/backend/renderer.js +++ b/packages/react-devtools-shared/src/backend/renderer.js @@ -2718,7 +2718,6 @@ export function attach( let mostRecentlyInspectedElement: InspectedElement | null = null; let hasElementUpdatedSinceLastInspected: boolean = false; - let currentlyInspectedPaths: Object = {}; function isMostRecentlyInspectedElementCurrent(id: number): boolean { return ( @@ -2728,21 +2727,10 @@ export function attach( ); } - // Track the intersection of currently inspected paths, - // so that we can send their data along if the element is re-rendered. - function mergeInspectedPaths(path: Array) { - let current = currentlyInspectedPaths; - path.forEach(key => { - if (!current[key]) { - current[key] = {}; - } - current = current[key]; - }); - } - function createIsPathAllowed( key: string | null, secondaryCategory: 'hooks' | null, + inspectedPaths: Object, ) { // This function helps prevent previously-inspected paths from being dehydrated in updates. // This is important to avoid a bad user experience where expanded toggles collapse on update. @@ -2767,8 +2755,7 @@ export function attach( break; } - let current = - key === null ? currentlyInspectedPaths : currentlyInspectedPaths[key]; + let current = key === null ? inspectedPaths : inspectedPaths[key]; if (!current) { return false; } @@ -2863,97 +2850,66 @@ export function attach( } function inspectElement( + requestID: number, id: number, - path?: Array, + inspectedPaths: Object, + forceUpdate: boolean, ): InspectedElementPayload { - const isCurrent = isMostRecentlyInspectedElementCurrent(id); + const isCurrent = !forceUpdate && isMostRecentlyInspectedElementCurrent(id); if (isCurrent) { - if (path != null) { - mergeInspectedPaths(path); - - let secondaryCategory = null; - if (path[0] === 'hooks') { - secondaryCategory = 'hooks'; - } - - // If this element has not been updated since it was last inspected, - // we can just return the subset of data in the newly-inspected path. - return { - id, - type: 'hydrated-path', - path, - value: cleanForBridge( - getInObject( - ((mostRecentlyInspectedElement: any): InspectedElement), - path, - ), - createIsPathAllowed(null, secondaryCategory), - path, - ), - }; - } else { - // If this element has not been updated since it was last inspected, we don't need to re-run it. - // Instead we can just return the ID to indicate that it has not changed. - return { - id, - type: 'no-change', - }; - } - } else { - hasElementUpdatedSinceLastInspected = false; - - if ( - mostRecentlyInspectedElement === null || - mostRecentlyInspectedElement.id !== id - ) { - currentlyInspectedPaths = {}; - } - - mostRecentlyInspectedElement = inspectElementRaw(id); - if (mostRecentlyInspectedElement === null) { - return { - id, - type: 'not-found', - }; - } - - if (path != null) { - mergeInspectedPaths(path); - } - - // Any time an inspected element has an update, - // we should update the selected $r value as wel. - // Do this before dehydration (cleanForBridge). - updateSelectedElement(mostRecentlyInspectedElement); + // If this element has not been updated since it was last inspected, we don't need to return it. + // Instead we can just return the ID to indicate that it has not changed. + return { + id, + responseID: requestID, + type: 'no-change', + }; + } - // Clone before cleaning so that we preserve the full data. - // This will enable us to send patches without re-inspecting if hydrated paths are requested. - // (Reducing how often we shallow-render is a better DX for function components that use hooks.) - const cleanedInspectedElement = {...mostRecentlyInspectedElement}; - cleanedInspectedElement.context = cleanForBridge( - cleanedInspectedElement.context, - createIsPathAllowed('context', null), - ); - cleanedInspectedElement.hooks = cleanForBridge( - cleanedInspectedElement.hooks, - createIsPathAllowed('hooks', 'hooks'), - ); - cleanedInspectedElement.props = cleanForBridge( - cleanedInspectedElement.props, - createIsPathAllowed('props', null), - ); - cleanedInspectedElement.state = cleanForBridge( - cleanedInspectedElement.state, - createIsPathAllowed('state', null), - ); + hasElementUpdatedSinceLastInspected = false; + mostRecentlyInspectedElement = inspectElementRaw(id); + if (mostRecentlyInspectedElement === null) { return { id, - type: 'full-data', - value: cleanedInspectedElement, + responseID: requestID, + type: 'not-found', }; } + + // Any time an inspected element has an update, + // we should update the selected $r value as wel. + // Do this before dehydration (cleanForBridge). + updateSelectedElement(mostRecentlyInspectedElement); + + // Clone before cleaning so that we preserve the full data. + // This will enable us to send patches without re-inspecting if hydrated paths are requested. + // (Reducing how often we shallow-render is a better DX for function components that use hooks.) + const cleanedInspectedElement = {...mostRecentlyInspectedElement}; + cleanedInspectedElement.context = cleanForBridge( + cleanedInspectedElement.context, + createIsPathAllowed('context', null, inspectedPaths), + ); + cleanedInspectedElement.hooks = cleanForBridge( + cleanedInspectedElement.hooks, + createIsPathAllowed('hooks', 'hooks', inspectedPaths), + ); + cleanedInspectedElement.props = cleanForBridge( + cleanedInspectedElement.props, + createIsPathAllowed('props', null, inspectedPaths), + ); + cleanedInspectedElement.state = cleanForBridge( + cleanedInspectedElement.state, + createIsPathAllowed('state', null, inspectedPaths), + ); + + return { + id, + responseID: requestID, + type: 'full-data', + value: cleanedInspectedElement, + }; } function logElementToConsole(id) { diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index 4cecf59df2983..09b32aa1ceef3 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -257,34 +257,28 @@ export type InspectedElement = {| export const InspectElementFullDataType = 'full-data'; export const InspectElementNoChangeType = 'no-change'; export const InspectElementNotFoundType = 'not-found'; -export const InspectElementHydratedPathType = 'hydrated-path'; type InspectElementFullData = {| id: number, + responseID: number, type: 'full-data', value: InspectedElement, |}; -type InspectElementHydratedPath = {| - id: number, - type: 'hydrated-path', - path: Array, - value: any, -|}; - type InspectElementNoChange = {| id: number, + responseID: number, type: 'no-change', |}; type InspectElementNotFound = {| id: number, + responseID: number, type: 'not-found', |}; export type InspectedElementPayload = | InspectElementFullData - | InspectElementHydratedPath | InspectElementNoChange | InspectElementNotFound; @@ -319,8 +313,10 @@ export type RendererInterface = { handleCommitFiberRoot: (fiber: Object, commitPriority?: number) => void, handleCommitFiberUnmount: (fiber: Object) => void, inspectElement: ( + requestID: number, id: number, - path?: Array, + inspectedPaths: Object, + forceUpdate: boolean, ) => InspectedElementPayload, logElementToConsole: (id: number) => void, overrideSuspense: (id: number, forceFallback: boolean) => void, diff --git a/packages/react-devtools-shared/src/backendAPI.js b/packages/react-devtools-shared/src/backendAPI.js new file mode 100644 index 0000000000000..372fd247d3b5a --- /dev/null +++ b/packages/react-devtools-shared/src/backendAPI.js @@ -0,0 +1,283 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +import {hydrate, fillInPath} from 'react-devtools-shared/src/hydration'; +import {separateDisplayNameAndHOCs} from 'react-devtools-shared/src/utils'; +import Store from 'react-devtools-shared/src/devtools/store'; + +import type { + InspectedElement as InspectedElementBackend, + InspectedElementPayload, +} from 'react-devtools-shared/src/backend/types'; +import type { + BackendEvents, + FrontendBridge, +} from 'react-devtools-shared/src/bridge'; +import type { + DehydratedData, + InspectedElement as InspectedElementFrontend, +} from 'react-devtools-shared/src/devtools/views/Components/types'; + +export function clearErrorsAndWarnings({ + bridge, + store, +}: {| + bridge: FrontendBridge, + store: Store, +|}): void { + store.rootIDToRendererID.forEach(rendererID => { + bridge.send('clearErrorsAndWarnings', {rendererID}); + }); +} + +export function clearErrorsForElement({ + bridge, + id, + rendererID, +}: {| + bridge: FrontendBridge, + id: number, + rendererID: number, +|}): void { + bridge.send('clearErrorsForFiberID', { + rendererID, + id, + }); +} + +export function clearWarningsForElement({ + bridge, + id, + rendererID, +}: {| + bridge: FrontendBridge, + id: number, + rendererID: number, +|}): void { + bridge.send('clearWarningsForFiberID', { + rendererID, + id, + }); +} + +export function copyInspectedElementPath({ + bridge, + id, + path, + rendererID, +}: {| + bridge: FrontendBridge, + id: number, + path: Array, + rendererID: number, +|}): void { + bridge.send('copyElementPath', { + id, + path, + rendererID, + }); +} + +export function inspectElement({ + bridge, + forceUpdate, + id, + inspectedPaths, + rendererID, +}: {| + bridge: FrontendBridge, + forceUpdate: boolean, + id: number, + inspectedPaths: Object, + rendererID: number, +|}): Promise { + const requestID = requestCounter++; + const promise = getPromiseForRequestID( + requestID, + 'inspectedElement', + bridge, + ); + + bridge.send('inspectElement', { + forceUpdate, + id, + inspectedPaths, + rendererID, + requestID, + }); + + return promise; +} + +let storeAsGlobalCount = 0; + +export function storeAsGlobal({ + bridge, + id, + path, + rendererID, +}: {| + bridge: FrontendBridge, + id: number, + path: Array, + rendererID: number, +|}): void { + bridge.send('storeAsGlobal', { + count: storeAsGlobalCount++, + id, + path, + rendererID, + }); +} + +const TIMEOUT_DELAY = 5000; + +let requestCounter = 0; + +function getPromiseForRequestID( + requestID: number, + eventType: $Keys, + bridge: FrontendBridge, +): Promise { + return new Promise((resolve, reject) => { + const cleanup = () => { + bridge.removeListener(eventType, onInspectedElement); + + clearTimeout(timeoutID); + }; + + const onInspectedElement = (data: any) => { + if (data.responseID === requestID) { + cleanup(); + resolve((data: T)); + } + }; + + const onTimeout = () => { + cleanup(); + reject(); + }; + + bridge.addListener(eventType, onInspectedElement); + + const timeoutID = setTimeout(onTimeout, TIMEOUT_DELAY); + }); +} + +export function cloneInspectedElementWithPath( + inspectedElement: InspectedElementFrontend, + path: Array, + value: Object, +): InspectedElementFrontend { + const hydratedValue = hydrateHelper(value, path); + const clonedInspectedElement = {...inspectedElement}; + + fillInPath(clonedInspectedElement, value, path, hydratedValue); + + return clonedInspectedElement; +} + +export function convertInspectedElementBackendToFrontend( + inspectedElementBackend: InspectedElementBackend, +): InspectedElementFrontend { + const { + canEditFunctionProps, + canEditFunctionPropsDeletePaths, + canEditFunctionPropsRenamePaths, + canEditHooks, + canEditHooksAndDeletePaths, + canEditHooksAndRenamePaths, + canToggleSuspense, + canViewSource, + hasLegacyContext, + id, + source, + type, + owners, + context, + hooks, + props, + rendererPackageName, + rendererVersion, + rootType, + state, + key, + errors, + warnings, + } = inspectedElementBackend; + + const inspectedElement: InspectedElementFrontend = { + canEditFunctionProps, + canEditFunctionPropsDeletePaths, + canEditFunctionPropsRenamePaths, + canEditHooks, + canEditHooksAndDeletePaths, + canEditHooksAndRenamePaths, + canToggleSuspense, + canViewSource, + hasLegacyContext, + id, + key, + rendererPackageName, + rendererVersion, + rootType, + source, + type, + owners: + owners === null + ? null + : owners.map(owner => { + const [displayName, hocDisplayNames] = separateDisplayNameAndHOCs( + owner.displayName, + owner.type, + ); + return { + ...owner, + displayName, + hocDisplayNames, + }; + }), + context: hydrateHelper(context), + hooks: hydrateHelper(hooks), + props: hydrateHelper(props), + state: hydrateHelper(state), + errors, + warnings, + }; + + return inspectedElement; +} + +function hydrateHelper( + dehydratedData: DehydratedData | null, + path?: Array, +): Object | null { + if (dehydratedData !== null) { + const {cleaned, data, unserializable} = dehydratedData; + + if (path) { + const {length} = path; + if (length > 0) { + // Hydration helper requires full paths, but inspection dehydrates with relative paths. + // In that event it's important that we adjust the "cleaned" paths to match. + return hydrate( + data, + cleaned.map(cleanedPath => cleanedPath.slice(length)), + unserializable.map(unserializablePath => + unserializablePath.slice(length), + ), + ); + } + } + + return hydrate(data, cleaned, unserializable); + } else { + return null; + } +} diff --git a/packages/react-devtools-shared/src/bridge.js b/packages/react-devtools-shared/src/bridge.js index 1da4d8eb3e549..dad25e93258c1 100644 --- a/packages/react-devtools-shared/src/bridge.js +++ b/packages/react-devtools-shared/src/bridge.js @@ -89,7 +89,9 @@ type ViewAttributeSourceParams = {| type InspectElementParams = {| ...ElementAndRendererID, - path?: Array, + forceUpdate: boolean, + inspectedPaths: Object, + requestID: number, |}; type StoreAsGlobalParams = {| @@ -117,7 +119,7 @@ type UpdateConsolePatchSettingsParams = {| showInlineWarningsAndErrors: boolean, |}; -type BackendEvents = {| +export type BackendEvents = {| extensionBackendInitialized: [], inspectedElement: [InspectedElementPayload], isBackendStorageAPISupported: [boolean], diff --git a/packages/react-devtools-shared/src/devtools/cache.js b/packages/react-devtools-shared/src/devtools/cache.js index af6a02faa0cf2..573f666402717 100644 --- a/packages/react-devtools-shared/src/devtools/cache.js +++ b/packages/react-devtools-shared/src/devtools/cache.js @@ -12,6 +12,8 @@ import type {Thenable} from 'shared/ReactTypes'; import * as React from 'react'; import {createContext} from 'react'; +// TODO (cache) Remove this cache; it is outdated and will not work with newer APIs like startTransition. + // Cache implementation was forked from the React repo: // https://github.com/facebook/react/blob/master/packages/react-cache/src/ReactCache.js // diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index 6b1b83e524838..e5a6ac15fbde1 100644 --- a/packages/react-devtools-shared/src/devtools/store.js +++ b/packages/react-devtools-shared/src/devtools/store.js @@ -101,7 +101,7 @@ export default class Store extends EventEmitter<{| // Map of ID to (mutable) Element. // Elements are mutated to avoid excessive cloning during tree updates. - // The InspectedElementContext also relies on this mutability for its WeakMap usage. + // The InspectedElement Suspense cache also relies on this mutability for its WeakMap usage. _idToElement: Map = new Map(); // Should the React Native style editor panel be shown? @@ -378,42 +378,6 @@ export default class Store extends EventEmitter<{| return this._cachedWarningCount; } - clearErrorsAndWarnings(): void { - this._rootIDToRendererID.forEach(rendererID => { - this._bridge.send('clearErrorsAndWarnings', { - rendererID, - }); - }); - } - - clearErrorsForElement(id: number): void { - const rendererID = this.getRendererIDForElement(id); - if (rendererID === null) { - console.warn( - `Unable to find rendererID for element ${id} when clearing errors.`, - ); - } else { - this._bridge.send('clearErrorsForFiberID', { - rendererID, - id, - }); - } - } - - clearWarningsForElement(id: number): void { - const rendererID = this.getRendererIDForElement(id); - if (rendererID === null) { - console.warn( - `Unable to find rendererID for element ${id} when clearing warnings.`, - ); - } else { - this._bridge.send('clearWarningsForFiberID', { - rendererID, - id, - }); - } - } - containsElement(id: number): boolean { return this._idToElement.get(id) != null; } diff --git a/packages/react-devtools-shared/src/devtools/views/Components/Components.js b/packages/react-devtools-shared/src/devtools/views/Components/Components.js index 91e31d4bf0294..712afdd987287 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/Components.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/Components.js @@ -17,7 +17,6 @@ import { useRef, } from 'react'; import Tree from './Tree'; -import {InspectedElementContextController} from './InspectedElementContext'; import {OwnersListContextController} from './OwnersListContext'; import portaledContent from '../portaledContent'; import {SettingsModalContextController} from 'react-devtools-shared/src/devtools/views/Settings/SettingsModalContext'; @@ -25,7 +24,9 @@ import { localStorageGetItem, localStorageSetItem, } from 'react-devtools-shared/src/storage'; +import InspectedElementErrorBoundary from './InspectedElementErrorBoundary'; import InspectedElement from './InspectedElement'; +import {InspectedElementContextController} from './InspectedElementContext'; import {ModalDialog} from '../ModalDialog'; import SettingsModal from 'react-devtools-shared/src/devtools/views/Settings/SettingsModal'; import {NativeStyleContextController} from './NativeStyleEditor/context'; @@ -151,32 +152,34 @@ function Components(_: {||}) { return ( - -
- -
- -
-
-
-
-
- +
+ +
+ +
+
+
+
+
+ + }> - + + + - -
- - - -
- + + +
+ + + +
); diff --git a/packages/react-devtools-shared/src/devtools/views/Components/ExpandCollapseToggle.js b/packages/react-devtools-shared/src/devtools/views/Components/ExpandCollapseToggle.js index 8950696b7e4fc..1851c75e0391e 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/ExpandCollapseToggle.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/ExpandCollapseToggle.js @@ -14,17 +14,20 @@ import ButtonIcon from '../ButtonIcon'; import styles from './ExpandCollapseToggle.css'; type ExpandCollapseToggleProps = {| + disabled: boolean, isOpen: boolean, setIsOpen: Function, |}; export default function ExpandCollapseToggle({ + disabled, isOpen, setIsOpen, }: ExpandCollapseToggleProps) { return (
diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js index 01b05a9fa33a1..48768b57aaac1 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContext.js @@ -10,6 +10,8 @@ import * as React from 'react'; import { createContext, + unstable_startTransition as startTransition, + unstable_useCacheRefresh as useCacheRefresh, useCallback, useContext, useEffect, @@ -17,360 +19,134 @@ import { useRef, useState, } from 'react'; -import {unstable_batchedUpdates as batchedUpdates} from 'react-dom'; -import {createResource} from '../../cache'; -import {BridgeContext, StoreContext} from '../context'; -import {hydrate, fillInPath} from 'react-devtools-shared/src/hydration'; import {TreeStateContext} from './TreeContext'; -import {separateDisplayNameAndHOCs} from 'react-devtools-shared/src/utils'; +import {BridgeContext, StoreContext} from '../context'; +import { + checkForUpdate, + inspectElement, +} from 'react-devtools-shared/src/inspectedElementCache'; +import type {ReactNodeList} from 'shared/ReactTypes'; import type { - InspectedElement as InspectedElementBackend, - InspectedElementPayload, -} from 'react-devtools-shared/src/backend/types'; -import type { - DehydratedData, Element, - InspectedElement as InspectedElementFrontend, + InspectedElement, } from 'react-devtools-shared/src/devtools/views/Components/types'; -import type {Resource, Thenable} from '../../cache'; - -export type StoreAsGlobal = (id: number, path: Array) => void; -export type CopyInspectedElementPath = ( - id: number, - path: Array, -) => void; +type Path = Array; +type InspectPathFunction = (path: Path) => void; -export type GetInspectedElementPath = ( - id: number, - path: Array, -) => void; - -export type GetInspectedElement = ( - id: number, -) => InspectedElementFrontend | null; - -type RefreshInspectedElement = () => void; - -export type InspectedElementContextType = {| - copyInspectedElementPath: CopyInspectedElementPath, - getInspectedElementPath: GetInspectedElementPath, - getInspectedElement: GetInspectedElement, - refreshInspectedElement: RefreshInspectedElement, - storeAsGlobal: StoreAsGlobal, +type Context = {| + inspectedElement: InspectedElement | null, + inspectPaths: InspectPathFunction, |}; -const InspectedElementContext = createContext( - ((null: any): InspectedElementContextType), +export const InspectedElementContext = createContext( + ((null: any): Context), ); -InspectedElementContext.displayName = 'InspectedElementContext'; - -type ResolveFn = (inspectedElement: InspectedElementFrontend) => void; -type InProgressRequest = {| - promise: Thenable, - resolveFn: ResolveFn, -|}; - -const inProgressRequests: WeakMap = new WeakMap(); -const resource: Resource< - Element, - Element, - InspectedElementFrontend, -> = createResource( - (element: Element) => { - const request = inProgressRequests.get(element); - if (request != null) { - return request.promise; - } - - let resolveFn = ((null: any): ResolveFn); - const promise = new Promise(resolve => { - resolveFn = resolve; - }); - - inProgressRequests.set(element, {promise, resolveFn}); - return promise; - }, - (element: Element) => element, - {useWeakMap: true}, -); +const POLL_INTERVAL = 1000; -type Props = {| - children: React$Node, +export type Props = {| + children: ReactNodeList, |}; -function InspectedElementContextController({children}: Props) { +export function InspectedElementContextController({children}: Props) { + const {selectedElementID} = useContext(TreeStateContext); const bridge = useContext(BridgeContext); const store = useContext(StoreContext); - const storeAsGlobalCount = useRef(1); - - // Ask the backend to store the value at the specified path as a global variable. - const storeAsGlobal = useCallback( - (id: number, path: Array) => { - const rendererID = store.getRendererIDForElement(id); - if (rendererID !== null) { - bridge.send('storeAsGlobal', { - count: storeAsGlobalCount.current++, - id, - path, - rendererID, - }); - } - }, - [bridge, store], - ); - - // Ask the backend to copy the specified path to the clipboard. - const copyInspectedElementPath = useCallback( - (id: number, path: Array) => { - const rendererID = store.getRendererIDForElement(id); - if (rendererID !== null) { - bridge.send('copyElementPath', {id, path, rendererID}); - } - }, - [bridge, store], - ); - - // Ask the backend to fill in a "dehydrated" path; this will result in a "inspectedElement". - const getInspectedElementPath = useCallback( - (id: number, path: Array) => { - const rendererID = store.getRendererIDForElement(id); - if (rendererID !== null) { - bridge.send('inspectElement', {id, path, rendererID}); - } - }, - [bridge, store], - ); - - const getInspectedElement = useCallback( - (id: number) => { - const element = store.getElementByID(id); - if (element !== null) { - return resource.read(element); - } else { - return null; - } - }, - [store], - ); - - // It's very important that this context consumes selectedElementID and not inspectedElementID. - // Otherwise the effect that sends the "inspect" message across the bridge- - // would itself be blocked by the same render that suspends (waiting for the data). - const {selectedElementID} = useContext(TreeStateContext); - - const refreshInspectedElement = useCallback(() => { - if (selectedElementID !== null) { - const rendererID = store.getRendererIDForElement(selectedElementID); - if (rendererID !== null) { - bridge.send('inspectElement', {id: selectedElementID, rendererID}); - } - } - }, [bridge, selectedElementID]); - - const [ - currentlyInspectedElement, - setCurrentlyInspectedElement, - ] = useState(null); - - // This effect handler invalidates the suspense cache and schedules rendering updates with React. - useEffect(() => { - const onInspectedElement = (data: InspectedElementPayload) => { - const {id} = data; + const refresh = useCacheRefresh(); - let element; + // Track when insepected paths have changed; we need to force the backend to send an udpate then. + const forceUpdateRef = useRef(true); - switch (data.type) { - case 'no-change': - case 'not-found': - // No-op - break; - case 'hydrated-path': - // Merge new data into previous object and invalidate cache - element = store.getElementByID(id); - if (element !== null) { - if (currentlyInspectedElement != null) { - const value = hydrateHelper(data.value, data.path); - const inspectedElement = {...currentlyInspectedElement}; + // Track the paths insepected for the currently selected element. + const [state, setState] = useState<{| + element: Element | null, + inspectedPaths: Object, + |}>({ + element: null, + inspectedPaths: {}, + }); - fillInPath(inspectedElement, data.value, data.path, value); + const element = + selectedElementID !== null ? store.getElementByID(selectedElementID) : null; - resource.write(element, inspectedElement); + const elementHasChanged = element !== null && element !== state.element; - // Schedule update with React if the currently-selected element has been invalidated. - if (id === selectedElementID) { - setCurrentlyInspectedElement(inspectedElement); - } - } - } - break; - case 'full-data': - const { - canEditFunctionProps, - canEditFunctionPropsDeletePaths, - canEditFunctionPropsRenamePaths, - canEditHooks, - canEditHooksAndDeletePaths, - canEditHooksAndRenamePaths, - canToggleSuspense, - canViewSource, - hasLegacyContext, - source, - type, - owners, - context, - hooks, - props, - rendererPackageName, - rendererVersion, - rootType, - state, - key, - errors, - warnings, - } = ((data.value: any): InspectedElementBackend); - - const inspectedElement: InspectedElementFrontend = { - canEditFunctionProps, - canEditFunctionPropsDeletePaths, - canEditFunctionPropsRenamePaths, - canEditHooks, - canEditHooksAndDeletePaths, - canEditHooksAndRenamePaths, - canToggleSuspense, - canViewSource, - hasLegacyContext, - id, - key, - rendererPackageName, - rendererVersion, - rootType, - source, - type, - owners: - owners === null - ? null - : owners.map(owner => { - const [ - displayName, - hocDisplayNames, - ] = separateDisplayNameAndHOCs( - owner.displayName, - owner.type, - ); - return { - ...owner, - displayName, - hocDisplayNames, - }; - }), - context: hydrateHelper(context), - hooks: hydrateHelper(hooks), - props: hydrateHelper(props), - state: hydrateHelper(state), - errors, - warnings, - }; - - element = store.getElementByID(id); - if (element !== null) { - const request = inProgressRequests.get(element); - if (request != null) { - inProgressRequests.delete(element); - batchedUpdates(() => { - request.resolveFn(inspectedElement); - setCurrentlyInspectedElement(inspectedElement); - }); - } else { - resource.write(element, inspectedElement); + // Reset the cached inspected paths when a new element is selected. + if (elementHasChanged) { + setState({ + element, + inspectedPaths: {}, + }); + } - // Schedule update with React if the currently-selected element has been invalidated. - if (id === selectedElementID) { - setCurrentlyInspectedElement(inspectedElement); - } + // Don't load a stale element from the backend; it wastes bridge bandwidth. + const inspectedElement = + !elementHasChanged && element !== null + ? inspectElement( + element, + state.inspectedPaths, + forceUpdateRef.current, + store, + bridge, + ) + : null; + + const inspectPaths: InspectPathFunction = useCallback( + (path: Path) => { + startTransition(() => { + forceUpdateRef.current = true; + setState(prevState => { + const cloned = {...prevState}; + let current = cloned.inspectedPaths; + path.forEach(key => { + if (!current[key]) { + current[key] = {}; } - } - break; - default: - break; - } - }; - - bridge.addListener('inspectedElement', onInspectedElement); - return () => bridge.removeListener('inspectedElement', onInspectedElement); - }, [bridge, currentlyInspectedElement, selectedElementID, store]); + current = current[key]; + }); + return cloned; + }); + refresh(); + }); + }, + [setState], + ); - // This effect handler polls for updates on the currently selected element. + // Force backend update when inspected paths change. useEffect(() => { - if (selectedElementID === null) { - return () => {}; - } - - const rendererID = store.getRendererIDForElement(selectedElementID); - - let timeoutID: TimeoutID | null = null; + forceUpdateRef.current = false; + }, [element, state]); - const sendRequest = () => { - timeoutID = null; - - if (rendererID !== null) { - bridge.send('inspectElement', {id: selectedElementID, rendererID}); - } - }; - - // Send the initial inspection request. - // We'll poll for an update in the response handler below. - sendRequest(); - - const onInspectedElement = (data: InspectedElementPayload) => { - // If this is the element we requested, wait a little bit and then ask for another update. - if (data.id === selectedElementID) { - switch (data.type) { - case 'no-change': - case 'full-data': - case 'hydrated-path': - if (timeoutID !== null) { - clearTimeout(timeoutID); - } - timeoutID = setTimeout(sendRequest, 1000); - break; - default: - break; - } - } - }; - - bridge.addListener('inspectedElement', onInspectedElement); - - return () => { - bridge.removeListener('inspectedElement', onInspectedElement); - - if (timeoutID !== null) { + // Periodically poll the selected element for updates. + useEffect(() => { + if (element !== null) { + const inspectedPaths = state.inspectedPaths; + const checkForUpdateWrapper = () => { + checkForUpdate({bridge, element, inspectedPaths, refresh, store}); + timeoutID = setTimeout(checkForUpdateWrapper, POLL_INTERVAL); + }; + let timeoutID = setTimeout(checkForUpdateWrapper, POLL_INTERVAL); + return () => { clearTimeout(timeoutID); - } - }; - }, [bridge, selectedElementID, store]); - - const value = useMemo( + }; + } + }, [ + element, + // Reset this timer any time the element we're inspecting gets a new response. + // No sense to ping right away after e.g. inspecting/hydrating a path. + inspectedElement, + state, + ]); + + const value = useMemo( () => ({ - copyInspectedElementPath, - getInspectedElement, - getInspectedElementPath, - refreshInspectedElement, - storeAsGlobal, + inspectedElement, + inspectPaths, }), - // InspectedElement is used to invalidate the cache and schedule an update with React. - [ - copyInspectedElementPath, - currentlyInspectedElement, - getInspectedElement, - getInspectedElementPath, - refreshInspectedElement, - storeAsGlobal, - ], + [inspectedElement, inspectPaths], ); return ( @@ -379,33 +155,3 @@ function InspectedElementContextController({children}: Props) { ); } - -function hydrateHelper( - dehydratedData: DehydratedData | null, - path?: Array, -): Object | null { - if (dehydratedData !== null) { - const {cleaned, data, unserializable} = dehydratedData; - - if (path) { - const {length} = path; - if (length > 0) { - // Hydration helper requires full paths, but inspection dehydrates with relative paths. - // In that event it's important that we adjust the "cleaned" paths to match. - return hydrate( - data, - cleaned.map(cleanedPath => cleanedPath.slice(length)), - unserializable.map(unserializablePath => - unserializablePath.slice(length), - ), - ); - } - } - - return hydrate(data, cleaned, unserializable); - } else { - return null; - } -} - -export {InspectedElementContext, InspectedElementContextController}; diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContextTree.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContextTree.js index 22347cef07f63..8dfe6310a3609 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContextTree.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementContextTree.js @@ -20,20 +20,20 @@ import { ElementTypeFunction, } from 'react-devtools-shared/src/types'; -import type {GetInspectedElementPath} from './InspectedElementContext'; import type {InspectedElement} from './types'; import type {FrontendBridge} from 'react-devtools-shared/src/bridge'; +import type {Element} from 'react-devtools-shared/src/devtools/views/Components/types'; type Props = {| bridge: FrontendBridge, - getInspectedElementPath: GetInspectedElementPath, + element: Element, inspectedElement: InspectedElement, store: Store, |}; export default function InspectedElementContextTree({ bridge, - getInspectedElementPath, + element, inspectedElement, store, }: Props) { @@ -81,15 +81,15 @@ export default function InspectedElementContextTree({ canEditValues={!isReadOnly} canRenamePaths={!isReadOnly} canRenamePathsAtDepth={canRenamePathsAtDepth} - type="context" depth={1} - getInspectedElementPath={getInspectedElementPath} + element={element} hidden={false} inspectedElement={inspectedElement} name={name} path={[name]} pathRoot="context" store={store} + type="context" value={value} /> ))} diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorBoundary.css b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorBoundary.css new file mode 100644 index 0000000000000..399f83cec45b0 --- /dev/null +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorBoundary.css @@ -0,0 +1,21 @@ +.Error { + justify-content: center; + align-items: center; + display: flex; + flex-direction: column; + height: 100%; + font-size: var(--font-size-sans-large); + font-weight: bold; + text-align: center; + background-color: var(--color-error-background); + color: var(--color-error-text); + border: 1px solid var(--color-error-border); + padding: 1rem; +} + +.Message { + margin-bottom: 1rem; +} + +.RetryButton { +} \ No newline at end of file diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorBoundary.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorBoundary.js new file mode 100644 index 0000000000000..09c11664dcf77 --- /dev/null +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorBoundary.js @@ -0,0 +1,93 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +import * as React from 'react'; +import {Component, useContext} from 'react'; +import {TreeDispatcherContext} from './TreeContext'; +import Button from 'react-devtools-shared/src/devtools/views/Button'; +import styles from './InspectedElementErrorBoundary.css'; + +import type {DispatcherContext} from './InspectedElementErrorBoundary.css'; + +type WrapperProps = {| + children: React$Node, +|}; + +export default function InspectedElementErrorBoundaryWrapper({ + children, +}: WrapperProps) { + const dispatch = useContext(TreeDispatcherContext); + + return ( + + ); +} + +type Props = {| + children: React$Node, + dispatch: DispatcherContext, +|}; + +type State = {| + errorMessage: string | null, + hasError: boolean, +|}; + +const InitialState: State = { + errorMessage: null, + hasError: false, +}; + +class InspectedElementErrorBoundary extends Component { + state: State = InitialState; + + static getDerivedStateFromError(error: any) { + const errorMessage = + typeof error === 'object' && + error !== null && + error.hasOwnProperty('message') + ? error.message + : error; + + return { + errorMessage, + hasError: true, + }; + } + + render() { + const {children} = this.props; + const {errorMessage, hasError} = this.state; + + if (hasError) { + return ( +
+
{errorMessage || 'Error'}
+ +
+ ); + } + + return children; + } + + _retry = () => { + const {dispatch} = this.props; + dispatch({ + type: 'SELECT_ELEMENT_BY_ID', + payload: null, + }); + this.setState({ + errorMessage: null, + hasError: false, + }); + }; +} diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorsAndWarningsTree.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorsAndWarningsTree.js index 1f161c98836b6..3b7aaca3216d5 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorsAndWarningsTree.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementErrorsAndWarningsTree.js @@ -8,14 +8,21 @@ */ import * as React from 'react'; -import {useContext} from 'react'; +import { + useContext, + unstable_useCacheRefresh as useCacheRefresh, + unstable_useTransition as useTransition, +} from 'react'; import Button from '../Button'; import ButtonIcon from '../ButtonIcon'; import Store from '../../store'; import sharedStyles from './InspectedElementSharedStyles.css'; import styles from './InspectedElementErrorsAndWarningsTree.css'; import {SettingsContext} from '../Settings/SettingsContext'; -import {InspectedElementContext} from './InspectedElementContext'; +import { + clearErrorsForElement as clearErrorsForElementAPI, + clearWarningsForElement as clearWarningsForElementAPI, +} from 'react-devtools-shared/src/backendAPI'; import type {InspectedElement} from './types'; import type {FrontendBridge} from 'react-devtools-shared/src/bridge'; @@ -31,7 +38,45 @@ export default function InspectedElementErrorsAndWarningsTree({ inspectedElement, store, }: Props) { - const {refreshInspectedElement} = useContext(InspectedElementContext); + const refresh = useCacheRefresh(); + + const [ + startClearErrorsTransition, + isErrorsTransitionPending, + ] = useTransition(); + const clearErrorsForInspectedElement = () => { + const {id} = inspectedElement; + const rendererID = store.getRendererIDForElement(id); + if (rendererID !== null) { + startClearErrorsTransition(() => { + clearErrorsForElementAPI({ + bridge, + id, + rendererID, + }); + refresh(); + }); + } + }; + + const [ + startClearWarningsTransition, + isWarningsTransitionPending, + ] = useTransition(); + const clearWarningsForInspectedElement = () => { + const {id} = inspectedElement; + const rendererID = store.getRendererIDForElement(id); + if (rendererID !== null) { + startClearWarningsTransition(() => { + clearWarningsForElementAPI({ + bridge, + id, + rendererID, + }); + refresh(); + }); + } + }; const {showInlineWarningsAndErrors} = useContext(SettingsContext); if (!showInlineWarningsAndErrors) { @@ -40,26 +85,6 @@ export default function InspectedElementErrorsAndWarningsTree({ const {errors, warnings} = inspectedElement; - const clearErrors = () => { - const {id} = inspectedElement; - store.clearErrorsForElement(id); - - // Immediately poll for updated data. - // This avoids a delay between clicking the clear button and refreshing errors. - // Ideally this would be done with useTranstion but that requires updating to a newer Cache strategy. - refreshInspectedElement(); - }; - - const clearWarnings = () => { - const {id} = inspectedElement; - store.clearWarningsForElement(id); - - // Immediately poll for updated data. - // This avoids a delay between clicking the clear button and refreshing warnings. - // Ideally this would be done with useTranstion but that requires updating to a newer Cache strategy. - refreshInspectedElement(); - }; - return ( {errors.length > 0 && ( @@ -67,8 +92,9 @@ export default function InspectedElementErrorsAndWarningsTree({ badgeClassName={styles.ErrorBadge} bridge={bridge} className={styles.ErrorTree} - clearMessages={clearErrors} + clearMessages={clearErrorsForInspectedElement} entries={errors} + isTransitionPending={isErrorsTransitionPending} label="errors" messageClassName={styles.Error} /> @@ -78,8 +104,9 @@ export default function InspectedElementErrorsAndWarningsTree({ badgeClassName={styles.WarningBadge} bridge={bridge} className={styles.WarningTree} - clearMessages={clearWarnings} + clearMessages={clearWarningsForInspectedElement} entries={warnings} + isTransitionPending={isWarningsTransitionPending} label="warnings" messageClassName={styles.Warning} /> @@ -94,6 +121,7 @@ type TreeProps = {| className: string, clearMessages: () => {}, entries: Array<[string, number]>, + isTransitionPending: boolean, label: string, messageClassName: string, |}; @@ -104,6 +132,7 @@ function Tree({ className, clearMessages, entries, + isTransitionPending, label, messageClassName, }: TreeProps) { @@ -115,6 +144,7 @@ function Tree({
{label}
diff --git a/packages/react-devtools-shared/src/devtools/views/ErrorBoundary.js b/packages/react-devtools-shared/src/devtools/views/ErrorBoundary.js index bd932cff395f9..7b777115d3b5e 100644 --- a/packages/react-devtools-shared/src/devtools/views/ErrorBoundary.js +++ b/packages/react-devtools-shared/src/devtools/views/ErrorBoundary.js @@ -38,14 +38,25 @@ const InitialState: State = { export default class ErrorBoundary extends Component { state: State = InitialState; - componentDidCatch(error: any, {componentStack}: any) { + static getDerivedStateFromError(error: any) { const errorMessage = - typeof error === 'object' && error.hasOwnProperty('message') + typeof error === 'object' && + error !== null && + error.hasOwnProperty('message') ? error.message : error; + return { + errorMessage, + hasError: true, + }; + } + + componentDidCatch(error: any, {componentStack}: any) { const callStack = - typeof error === 'object' && error.hasOwnProperty('stack') + typeof error === 'object' && + error !== null && + error.hasOwnProperty('stack') ? error.stack .split('\n') .slice(1) @@ -55,8 +66,6 @@ export default class ErrorBoundary extends Component { this.setState({ callStack, componentStack, - errorMessage, - hasError: true, }); } diff --git a/packages/react-devtools-shared/src/inspectedElementCache.js b/packages/react-devtools-shared/src/inspectedElementCache.js new file mode 100644 index 0000000000000..6169207df0dbe --- /dev/null +++ b/packages/react-devtools-shared/src/inspectedElementCache.js @@ -0,0 +1,220 @@ +/** + * Copyright (c) Facebook, Inc. and its affiliates. + * + * This source code is licensed under the MIT license found in the + * LICENSE file in the root directory of this source tree. + * + * @flow + */ + +import { + unstable_getCacheForType, + unstable_startTransition as startTransition, +} from 'react'; +import Store from './devtools/store'; +import { + convertInspectedElementBackendToFrontend, + inspectElement as inspectElementAPI, +} from './backendAPI'; + +import type {FrontendBridge} from 'react-devtools-shared/src/bridge'; +import type {Wakeable} from 'shared/ReactTypes'; +import type { + InspectedElement as InspectedElementBackend, + InspectedElementPayload, +} from 'react-devtools-shared/src/backend/types'; +import type { + Element, + InspectedElement as InspectedElementFrontend, +} from 'react-devtools-shared/src/devtools/views/Components/types'; + +const Pending = 0; +const Resolved = 1; +const Rejected = 2; + +type PendingRecord = {| + status: 0, + value: Wakeable, +|}; + +type ResolvedRecord = {| + status: 1, + value: T, +|}; + +type RejectedRecord = {| + status: 2, + value: string, +|}; + +type Record = PendingRecord | ResolvedRecord | RejectedRecord; + +function readRecord(record: Record): ResolvedRecord { + if (record.status === Resolved) { + // This is just a type refinement. + return record; + } else { + throw record.value; + } +} + +type InspectedElementMap = WeakMap>; +type CacheSeedKey = () => InspectedElementMap; + +function createMap(): InspectedElementMap { + return new WeakMap(); +} + +function getRecordMap(): WeakMap> { + return unstable_getCacheForType(createMap); +} + +function createCacheSeed( + element: Element, + inspectedElement: InspectedElementFrontend, +): [CacheSeedKey, InspectedElementMap] { + const newRecord: Record = { + status: Resolved, + value: inspectedElement, + }; + const map = createMap(); + map.set(element, newRecord); + return [createMap, map]; +} + +/** + * Fetches element props and state from the backend for inspection. + * This method should be called during render; it will suspend if data has not yet been fetched. + */ +export function inspectElement( + element: Element, + inspectedPaths: Object, + forceUpdate: boolean, + store: Store, + bridge: FrontendBridge, +): InspectedElementFrontend | null { + const map = getRecordMap(); + let record = map.get(element); + if (!record) { + const callbacks = new Set(); + const wakeable: Wakeable = { + then(callback) { + callbacks.add(callback); + }, + }; + const wake = () => { + // This assumes they won't throw. + callbacks.forEach(callback => callback()); + callbacks.clear(); + }; + const newRecord: Record = (record = { + status: Pending, + value: wakeable, + }); + + const rendererID = store.getRendererIDForElement(element.id); + if (rendererID == null) { + const rejectedRecord = ((newRecord: any): RejectedRecord); + rejectedRecord.status = Rejected; + rejectedRecord.value = 'Inspected element not found.'; + return null; + } + + inspectElementAPI({ + bridge, + forceUpdate: true, + id: element.id, + inspectedPaths, + rendererID: ((rendererID: any): number), + }).then( + (data: InspectedElementPayload) => { + if (newRecord.status === Pending) { + switch (data.type) { + case 'no-change': + // This response type should never be received. + // We always send forceUpdate:true when we have a cache miss. + break; + + case 'not-found': + const notFoundRecord = ((newRecord: any): RejectedRecord); + notFoundRecord.status = Rejected; + notFoundRecord.value = 'Inspected element not found.'; + wake(); + break; + + case 'full-data': + const resolvedRecord = ((newRecord: any): ResolvedRecord); + resolvedRecord.status = Resolved; + resolvedRecord.value = convertInspectedElementBackendToFrontend( + ((data.value: any): InspectedElementBackend), + ); + wake(); + break; + } + } + }, + + () => { + // Timed out without receiving a response. + if (newRecord.status === Pending) { + const timedOutRecord = ((newRecord: any): RejectedRecord); + timedOutRecord.status = Rejected; + timedOutRecord.value = 'Inspected element timed out.'; + wake(); + } + }, + ); + map.set(element, record); + } + + const response = readRecord(record).value; + return response; +} + +type RefreshFunction = ( + seedKey: CacheSeedKey, + cacheMap: InspectedElementMap, +) => void; + +/** + * Asks the backend for updated props and state from an expected element. + * This method should never be called during render; call it from an effect or event handler. + * This method will schedule an update if updated information is returned. + */ +export function checkForUpdate({ + bridge, + element, + inspectedPaths, + refresh, + store, +}: { + bridge: FrontendBridge, + element: Element, + inspectedPaths: Object, + refresh: RefreshFunction, + store: Store, +}): void { + const {id} = element; + const rendererID = store.getRendererIDForElement(id); + if (rendererID != null) { + inspectElementAPI({ + bridge, + forceUpdate: false, + id, + inspectedPaths, + rendererID, + }).then((data: InspectedElementPayload) => { + switch (data.type) { + case 'full-data': + const inspectedElement = convertInspectedElementBackendToFrontend( + ((data.value: any): InspectedElementBackend), + ); + startTransition(() => { + const [key, value] = createCacheSeed(element, inspectedElement); + refresh(key, value); + }); + break; + } + }); + } +} diff --git a/packages/shared/forks/ReactFeatureFlags.test-renderer.js b/packages/shared/forks/ReactFeatureFlags.test-renderer.js index 16e488409e0c4..c6ffacb27bab2 100644 --- a/packages/shared/forks/ReactFeatureFlags.test-renderer.js +++ b/packages/shared/forks/ReactFeatureFlags.test-renderer.js @@ -23,7 +23,7 @@ export const enableSchedulerTracing = __PROFILE__; export const enableSuspenseServerRenderer = false; export const enableSelectiveHydration = false; export const enableLazyElements = false; -export const enableCache = false; +export const enableCache = __EXPERIMENTAL__; export const disableJavaScriptURLs = false; export const disableInputAttributeSyncing = false; export const enableSchedulerDebugging = false; diff --git a/scripts/jest/config.build-devtools.js b/scripts/jest/config.build-devtools.js index 0d5a6039e414d..761479ce45c50 100644 --- a/scripts/jest/config.build-devtools.js +++ b/scripts/jest/config.build-devtools.js @@ -58,6 +58,9 @@ module.exports = Object.assign({}, baseConfig, { transformIgnorePatterns: ['/node_modules/', '/build2/'], testRegex: 'packages/react-devtools-shared/src/__tests__/[^]+.test.js$', snapshotSerializers: [ + require.resolve( + '../../packages/react-devtools-shared/src/__tests__/dehydratedValueSerializer.js' + ), require.resolve( '../../packages/react-devtools-shared/src/__tests__/inspectedElementSerializer.js' ), diff --git a/scripts/jest/preprocessor.js b/scripts/jest/preprocessor.js index f57005c9407fa..072071be07fbd 100644 --- a/scripts/jest/preprocessor.js +++ b/scripts/jest/preprocessor.js @@ -59,6 +59,10 @@ const babelOptions = { module.exports = { process: function(src, filePath) { + if (filePath.match(/\.css$/)) { + // Don't try to parse CSS modules; they aren't needed for tests anyway. + return ''; + } if (filePath.match(/\.coffee$/)) { return coffee.compile(src, {bare: true}); }