diff --git a/.github/workflows/runtime_commit_artifacts.yml b/.github/workflows/runtime_commit_artifacts.yml index 073c289843..f47eb3ff36 100644 --- a/.github/workflows/runtime_commit_artifacts.yml +++ b/.github/workflows/runtime_commit_artifacts.yml @@ -87,7 +87,7 @@ jobs: build/oss-experimental/react-refresh/cjs/react-refresh-babel.development.js - name: Insert @headers into eslint plugin and react-refresh run: | - sed -i -e 's/ LICENSE file in the root directory of this source tree./ LICENSE file in the root directory of this source tree.\n * \n * @noformat\n * @nolint\n * @lightSyntaxTransform\n * @preventMunge\n * @oncall react_core/' \ + sed -i -e 's/ LICENSE file in the root directory of this source tree./ LICENSE file in the root directory of this source tree.\n *\n * @noformat\n * @nolint\n * @lightSyntaxTransform\n * @preventMunge\n * @oncall react_core/' \ build/oss-experimental/eslint-plugin-react-hooks/cjs/eslint-plugin-react-hooks.development.js \ build/oss-experimental/react-refresh/cjs/react-refresh-babel.development.js - name: Move relevant files for React in www into compiled diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts index df6241a73f..8a65b4709c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoRefAccesInRender.ts @@ -11,12 +11,12 @@ import { IdentifierId, Place, SourceLocation, - isRefOrRefValue, isRefValueType, isUseRefType, } from '../HIR'; import { eachInstructionValueOperand, + eachPatternOperand, eachTerminalOperand, } from '../HIR/visitors'; import {Err, Ok, Result} from '../Utils/Result'; @@ -42,58 +42,165 @@ import {isEffectHook} from './ValidateMemoizedEffectDependencies'; * In the future we may reject more cases, based on either object names (`fooRef.current` is likely a ref) * or based on property name alone (`foo.current` might be a ref). */ +type State = { + refs: Set; + refValues: Map; + refAccessingFunctions: Set; +}; + export function validateNoRefAccessInRender(fn: HIRFunction): void { - const refAccessingFunctions: Set = new Set(); - validateNoRefAccessInRenderImpl(fn, refAccessingFunctions).unwrap(); + const state = { + refs: new Set(), + refValues: new Map(), + refAccessingFunctions: new Set(), + }; + validateNoRefAccessInRenderImpl(fn, state).unwrap(); } function validateNoRefAccessInRenderImpl( fn: HIRFunction, - refAccessingFunctions: Set, + state: State, ): Result { + let place; + for (const param of fn.params) { + if (param.kind === 'Identifier') { + place = param; + } else { + place = param.place; + } + + if (isRefValueType(place.identifier)) { + state.refValues.set(place.identifier.id, null); + } + if (isUseRefType(place.identifier)) { + state.refs.add(place.identifier.id); + } + } const errors = new CompilerError(); - const lookupLocations: Map = new Map(); for (const [, block] of fn.body.blocks) { + for (const phi of block.phis) { + phi.operands.forEach(operand => { + if (state.refs.has(operand.id) || isUseRefType(phi.id)) { + state.refs.add(phi.id.id); + } + const refValue = state.refValues.get(operand.id); + if (refValue !== undefined || isRefValueType(operand)) { + state.refValues.set( + phi.id.id, + refValue ?? state.refValues.get(phi.id.id) ?? null, + ); + } + if (state.refAccessingFunctions.has(operand.id)) { + state.refAccessingFunctions.add(phi.id.id); + } + }); + } + for (const instr of block.instructions) { + for (const operand of eachInstructionValueOperand(instr.value)) { + if (isRefValueType(operand.identifier)) { + CompilerError.invariant(state.refValues.has(operand.identifier.id), { + reason: 'Expected ref value to be in state', + loc: operand.loc, + }); + } + if (isUseRefType(operand.identifier)) { + CompilerError.invariant(state.refs.has(operand.identifier.id), { + reason: 'Expected ref to be in state', + loc: operand.loc, + }); + } + } + switch (instr.value.kind) { case 'JsxExpression': case 'JsxFragment': { for (const operand of eachInstructionValueOperand(instr.value)) { - validateNoDirectRefValueAccess(errors, operand, lookupLocations); + validateNoDirectRefValueAccess(errors, operand, state); } break; } + case 'ComputedLoad': case 'PropertyLoad': { + if (typeof instr.value.property !== 'string') { + validateNoRefValueAccess(errors, state, instr.value.property); + } if ( - isRefValueType(instr.lvalue.identifier) && - instr.value.property === 'current' + state.refAccessingFunctions.has(instr.value.object.identifier.id) ) { - lookupLocations.set(instr.lvalue.identifier.id, instr.loc); + state.refAccessingFunctions.add(instr.lvalue.identifier.id); + } + if (state.refs.has(instr.value.object.identifier.id)) { + /* + * Once an object contains a ref at any level, we treat it as a ref. + * If we look something up from it, that value may either be a ref + * or the ref value (or neither), so we conservatively assume it's both. + */ + state.refs.add(instr.lvalue.identifier.id); + state.refValues.set(instr.lvalue.identifier.id, instr.loc); } break; } + case 'LoadContext': case 'LoadLocal': { - if (refAccessingFunctions.has(instr.value.place.identifier.id)) { - refAccessingFunctions.add(instr.lvalue.identifier.id); + if ( + state.refAccessingFunctions.has(instr.value.place.identifier.id) + ) { + state.refAccessingFunctions.add(instr.lvalue.identifier.id); } - if (isRefValueType(instr.lvalue.identifier)) { - const loc = lookupLocations.get(instr.value.place.identifier.id); - if (loc !== undefined) { - lookupLocations.set(instr.lvalue.identifier.id, loc); - } + const refValue = state.refValues.get(instr.value.place.identifier.id); + if (refValue !== undefined) { + state.refValues.set(instr.lvalue.identifier.id, refValue); + } + if (state.refs.has(instr.value.place.identifier.id)) { + state.refs.add(instr.lvalue.identifier.id); } break; } + case 'StoreContext': case 'StoreLocal': { - if (refAccessingFunctions.has(instr.value.value.identifier.id)) { - refAccessingFunctions.add(instr.value.lvalue.place.identifier.id); - refAccessingFunctions.add(instr.lvalue.identifier.id); + if ( + state.refAccessingFunctions.has(instr.value.value.identifier.id) + ) { + state.refAccessingFunctions.add( + instr.value.lvalue.place.identifier.id, + ); + state.refAccessingFunctions.add(instr.lvalue.identifier.id); } - if (isRefValueType(instr.value.lvalue.place.identifier)) { - const loc = lookupLocations.get(instr.value.value.identifier.id); - if (loc !== undefined) { - lookupLocations.set(instr.value.lvalue.place.identifier.id, loc); - lookupLocations.set(instr.lvalue.identifier.id, loc); + const refValue = state.refValues.get(instr.value.value.identifier.id); + if ( + refValue !== undefined || + isRefValueType(instr.value.lvalue.place.identifier) + ) { + state.refValues.set( + instr.value.lvalue.place.identifier.id, + refValue ?? null, + ); + state.refValues.set(instr.lvalue.identifier.id, refValue ?? null); + } + if (state.refs.has(instr.value.value.identifier.id)) { + state.refs.add(instr.value.lvalue.place.identifier.id); + state.refs.add(instr.lvalue.identifier.id); + } + break; + } + case 'Destructure': { + const destructuredFunction = state.refAccessingFunctions.has( + instr.value.value.identifier.id, + ); + const destructuredRef = state.refs.has( + instr.value.value.identifier.id, + ); + for (const lval of eachPatternOperand(instr.value.lvalue.pattern)) { + if (isUseRefType(lval.identifier)) { + state.refs.add(lval.identifier.id); + } + if (destructuredRef || isRefValueType(lval.identifier)) { + state.refs.add(lval.identifier.id); + state.refValues.set(lval.identifier.id, null); + } + if (destructuredFunction) { + state.refAccessingFunctions.add(lval.identifier.id); } } break; @@ -107,32 +214,27 @@ function validateNoRefAccessInRenderImpl( */ [...eachInstructionValueOperand(instr.value)].some( operand => - isRefValueType(operand.identifier) || - refAccessingFunctions.has(operand.identifier.id), + state.refValues.has(operand.identifier.id) || + state.refAccessingFunctions.has(operand.identifier.id), ) || // check for cases where .current is accessed through an aliased ref ([...eachInstructionValueOperand(instr.value)].some(operand => - isUseRefType(operand.identifier), + state.refs.has(operand.identifier.id), ) && validateNoRefAccessInRenderImpl( instr.value.loweredFunc.func, - refAccessingFunctions, + state, ).isErr()) ) { // This function expression unconditionally accesses a ref - refAccessingFunctions.add(instr.lvalue.identifier.id); + state.refAccessingFunctions.add(instr.lvalue.identifier.id); } break; } case 'MethodCall': { if (!isEffectHook(instr.value.property.identifier)) { for (const operand of eachInstructionValueOperand(instr.value)) { - validateNoRefAccess( - errors, - refAccessingFunctions, - operand, - operand.loc, - ); + validateNoRefAccess(errors, state, operand, operand.loc); } } break; @@ -142,7 +244,7 @@ function validateNoRefAccessInRenderImpl( const isUseEffect = isEffectHook(callee.identifier); if (!isUseEffect) { // Report a more precise error when calling a local function that accesses a ref - if (refAccessingFunctions.has(callee.identifier.id)) { + if (state.refAccessingFunctions.has(callee.identifier.id)) { errors.push({ severity: ErrorSeverity.InvalidReact, reason: @@ -159,9 +261,9 @@ function validateNoRefAccessInRenderImpl( for (const operand of eachInstructionValueOperand(instr.value)) { validateNoRefAccess( errors, - refAccessingFunctions, + state, operand, - lookupLocations.get(operand.identifier.id) ?? operand.loc, + state.refValues.get(operand.identifier.id) ?? operand.loc, ); } } @@ -170,12 +272,17 @@ function validateNoRefAccessInRenderImpl( case 'ObjectExpression': case 'ArrayExpression': { for (const operand of eachInstructionValueOperand(instr.value)) { - validateNoRefAccess( - errors, - refAccessingFunctions, - operand, - lookupLocations.get(operand.identifier.id) ?? operand.loc, - ); + validateNoDirectRefValueAccess(errors, operand, state); + if (state.refAccessingFunctions.has(operand.identifier.id)) { + state.refAccessingFunctions.add(instr.lvalue.identifier.id); + } + if (state.refs.has(operand.identifier.id)) { + state.refs.add(instr.lvalue.identifier.id); + } + const refValue = state.refValues.get(operand.identifier.id); + if (refValue !== undefined) { + state.refValues.set(instr.lvalue.identifier.id, refValue); + } } break; } @@ -185,20 +292,15 @@ function validateNoRefAccessInRenderImpl( case 'ComputedStore': { validateNoRefAccess( errors, - refAccessingFunctions, + state, instr.value.object, - lookupLocations.get(instr.value.object.identifier.id) ?? instr.loc, + state.refValues.get(instr.value.object.identifier.id) ?? instr.loc, ); for (const operand of eachInstructionValueOperand(instr.value)) { if (operand === instr.value.object) { continue; } - validateNoRefValueAccess( - errors, - refAccessingFunctions, - lookupLocations, - operand, - ); + validateNoRefValueAccess(errors, state, operand); } break; } @@ -207,28 +309,27 @@ function validateNoRefAccessInRenderImpl( break; default: { for (const operand of eachInstructionValueOperand(instr.value)) { - validateNoRefValueAccess( - errors, - refAccessingFunctions, - lookupLocations, - operand, - ); + validateNoRefValueAccess(errors, state, operand); } break; } } + if (isUseRefType(instr.lvalue.identifier)) { + state.refs.add(instr.lvalue.identifier.id); + } + if ( + isRefValueType(instr.lvalue.identifier) && + !state.refValues.has(instr.lvalue.identifier.id) + ) { + state.refValues.set(instr.lvalue.identifier.id, instr.loc); + } } for (const operand of eachTerminalOperand(block.terminal)) { if (block.terminal.kind !== 'return') { - validateNoRefValueAccess( - errors, - refAccessingFunctions, - lookupLocations, - operand, - ); + validateNoRefValueAccess(errors, state, operand); } else { // Allow functions containing refs to be returned, but not direct ref values - validateNoDirectRefValueAccess(errors, operand, lookupLocations); + validateNoDirectRefValueAccess(errors, operand, state); } } } @@ -242,19 +343,18 @@ function validateNoRefAccessInRenderImpl( function validateNoRefValueAccess( errors: CompilerError, - refAccessingFunctions: Set, - lookupLocations: Map, + state: State, operand: Place, ): void { if ( - isRefValueType(operand.identifier) || - refAccessingFunctions.has(operand.identifier.id) + state.refValues.has(operand.identifier.id) || + state.refAccessingFunctions.has(operand.identifier.id) ) { errors.push({ severity: ErrorSeverity.InvalidReact, reason: 'Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)', - loc: lookupLocations.get(operand.identifier.id) ?? operand.loc, + loc: state.refValues.get(operand.identifier.id) ?? operand.loc, description: operand.identifier.name !== null && operand.identifier.name.kind === 'named' @@ -267,13 +367,14 @@ function validateNoRefValueAccess( function validateNoRefAccess( errors: CompilerError, - refAccessingFunctions: Set, + state: State, operand: Place, loc: SourceLocation, ): void { if ( - isRefOrRefValue(operand.identifier) || - refAccessingFunctions.has(operand.identifier.id) + state.refs.has(operand.identifier.id) || + state.refValues.has(operand.identifier.id) || + state.refAccessingFunctions.has(operand.identifier.id) ) { errors.push({ severity: ErrorSeverity.InvalidReact, @@ -293,14 +394,14 @@ function validateNoRefAccess( function validateNoDirectRefValueAccess( errors: CompilerError, operand: Place, - lookupLocations: Map, + state: State, ): void { - if (isRefValueType(operand.identifier)) { + if (state.refValues.has(operand.identifier.id)) { errors.push({ severity: ErrorSeverity.InvalidReact, reason: 'Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef)', - loc: lookupLocations.get(operand.identifier.id) ?? operand.loc, + loc: state.refValues.get(operand.identifier.id) ?? operand.loc, description: operand.identifier.name !== null && operand.identifier.name.kind === 'named' diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md index a28a74730b..f576bac764 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-use-ref-added-to-dep-without-type-info.expect.md @@ -22,13 +22,15 @@ function Foo({a}) { ## Error ``` - 3 | const ref = useRef(); - 4 | // type information is lost here as we don't track types of fields -> 5 | const val = {ref}; - | ^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (5:5) - 6 | // without type info, we don't know that val.ref.current is a ref value so we - 7 | // *would* end up depending on val.ref.current - 8 | // however, this is an instance of accessing a ref during render and is disallowed + 8 | // however, this is an instance of accessing a ref during render and is disallowed + 9 | // under React's rules, so we reject this input +> 10 | const x = {a, val: val.ref.current}; + | ^^^^^^^^^^^^^^^ InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (10:10) + +InvalidReact: Ref values (the `current` property) may not be accessed during render. (https://react.dev/reference/react/useRef) (10:10) + 11 | + 12 | return ; + 13 | } ``` \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/return-ref-callback-structure.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/return-ref-callback-structure.expect.md new file mode 100644 index 0000000000..95976383cb --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/return-ref-callback-structure.expect.md @@ -0,0 +1,87 @@ + +## Input + +```javascript +// @flow @validateRefAccessDuringRender @validatePreserveExistingMemoizationGuarantees + +import {useRef} from 'react'; + +component Foo(cond: boolean, cond2: boolean) { + const ref = useRef(); + + const s = () => { + return ref.current; + }; + + if (cond) return [s]; + else if (cond2) return {s}; + else return {s: [s]}; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{cond: false, cond2: false}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; + +import { useRef } from "react"; + +function Foo(t0) { + const $ = _c(4); + const { cond, cond2 } = t0; + const ref = useRef(); + let t1; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t1 = () => ref.current; + $[0] = t1; + } else { + t1 = $[0]; + } + const s = t1; + if (cond) { + let t2; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + t2 = [s]; + $[1] = t2; + } else { + t2 = $[1]; + } + return t2; + } else { + if (cond2) { + let t2; + if ($[2] === Symbol.for("react.memo_cache_sentinel")) { + t2 = { s }; + $[2] = t2; + } else { + t2 = $[2]; + } + return t2; + } else { + let t2; + if ($[3] === Symbol.for("react.memo_cache_sentinel")) { + t2 = { s: [s] }; + $[3] = t2; + } else { + t2 = $[3]; + } + return t2; + } + } +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{ cond: false, cond2: false }], +}; + +``` + +### Eval output +(kind: ok) {"s":["[[ function params=0 ]]"]} \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/return-ref-callback-structure.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/return-ref-callback-structure.js new file mode 100644 index 0000000000..e37acbde34 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/return-ref-callback-structure.js @@ -0,0 +1,20 @@ +// @flow @validateRefAccessDuringRender @validatePreserveExistingMemoizationGuarantees + +import {useRef} from 'react'; + +component Foo(cond: boolean, cond2: boolean) { + const ref = useRef(); + + const s = () => { + return ref.current; + }; + + if (cond) return [s]; + else if (cond2) return {s}; + else return {s: [s]}; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Foo, + params: [{cond: false, cond2: false}], +}; diff --git a/packages/react-devtools-extensions/src/main/index.js b/packages/react-devtools-extensions/src/main/index.js index d0bc285b11..3a51b996e2 100644 --- a/packages/react-devtools-extensions/src/main/index.js +++ b/packages/react-devtools-extensions/src/main/index.js @@ -21,6 +21,8 @@ import { setBrowserSelectionFromReact, setReactSelectionFromBrowser, } from './elementSelection'; +import {viewAttributeSource} from './sourceSelection'; + import {startReactPolling} from './reactPolling'; import cloneStyleTags from './cloneStyleTags'; import fetchFileWithCaching from './fetchFileWithCaching'; @@ -113,19 +115,7 @@ function createBridgeAndStore() { const viewAttributeSourceFunction = (id, path) => { const rendererID = store.getRendererIDForElement(id); if (rendererID != null) { - // Ask the renderer interface to find the specified attribute, - // and store it as a global variable on the window. - bridge.send('viewAttributeSource', {id, path, rendererID}); - - setTimeout(() => { - // Ask Chrome to display the location of the attribute, - // assuming the renderer found a match. - chrome.devtools.inspectedWindow.eval(` - if (window.$attribute != null) { - inspect(window.$attribute); - } - `); - }, 100); + viewAttributeSource(rendererID, id, path); } }; diff --git a/packages/react-devtools-extensions/src/main/sourceSelection.js b/packages/react-devtools-extensions/src/main/sourceSelection.js new file mode 100644 index 0000000000..0534a921af --- /dev/null +++ b/packages/react-devtools-extensions/src/main/sourceSelection.js @@ -0,0 +1,59 @@ +/* global chrome */ + +export function viewAttributeSource(rendererID, elementID, path) { + chrome.devtools.inspectedWindow.eval( + '{' + // The outer block is important because it means we can declare local variables. + 'const renderer = window.__REACT_DEVTOOLS_GLOBAL_HOOK__.rendererInterfaces.get(' + + JSON.stringify(rendererID) + + ');' + + 'if (renderer) {' + + ' const value = renderer.getElementAttributeByPath(' + + JSON.stringify(elementID) + + ',' + + JSON.stringify(path) + + ');' + + ' if (value) {' + + ' inspect(value);' + + ' true;' + + ' } else {' + + ' false;' + + ' }' + + '} else {' + + ' false;' + + '}' + + '}', + (didInspect, evalError) => { + if (evalError) { + console.error(evalError); + } + }, + ); +} + +export function viewElementSource(rendererID, elementID) { + chrome.devtools.inspectedWindow.eval( + '{' + // The outer block is important because it means we can declare local variables. + 'const renderer = window.__REACT_DEVTOOLS_GLOBAL_HOOK__.rendererInterfaces.get(' + + JSON.stringify(rendererID) + + ');' + + 'if (renderer) {' + + ' const value = renderer.getElementSourceFunctionById(' + + JSON.stringify(elementID) + + ');' + + ' if (value) {' + + ' inspect(value);' + + ' true;' + + ' } else {' + + ' false;' + + ' }' + + '} else {' + + ' false;' + + '}' + + '}', + (didInspect, evalError) => { + if (evalError) { + console.error(evalError); + } + }, + ); +} diff --git a/packages/react-devtools-shared/src/backend/agent.js b/packages/react-devtools-shared/src/backend/agent.js index e81af56ebd..a71b259441 100644 --- a/packages/react-devtools-shared/src/backend/agent.js +++ b/packages/react-devtools-shared/src/backend/agent.js @@ -220,8 +220,6 @@ export default class Agent extends EventEmitter<{ this.updateConsolePatchSettings, ); bridge.addListener('updateComponentFilters', this.updateComponentFilters); - bridge.addListener('viewAttributeSource', this.viewAttributeSource); - bridge.addListener('viewElementSource', this.viewElementSource); // Temporarily support older standalone front-ends sending commands to newer embedded backends. // We do this because React Native embeds the React DevTools backend, @@ -816,24 +814,6 @@ export default class Agent extends EventEmitter<{ } }; - viewAttributeSource: CopyElementParams => void = ({id, path, rendererID}) => { - const renderer = this._rendererInterfaces[rendererID]; - if (renderer == null) { - console.warn(`Invalid renderer id "${rendererID}" for element "${id}"`); - } else { - renderer.prepareViewAttributeSource(id, path); - } - }; - - viewElementSource: ElementAndRendererID => void = ({id, rendererID}) => { - const renderer = this._rendererInterfaces[rendererID]; - if (renderer == null) { - console.warn(`Invalid renderer id "${rendererID}" for element "${id}"`); - } else { - renderer.prepareViewElementSource(id); - } - }; - onTraceUpdates: (nodes: Set) => void = nodes => { this.emit('traceUpdates', nodes); }; diff --git a/packages/react-devtools-shared/src/backend/fiber/DevToolsFiberComponentStack.js b/packages/react-devtools-shared/src/backend/fiber/DevToolsFiberComponentStack.js index 5a1fc4ee53..17475cab28 100644 --- a/packages/react-devtools-shared/src/backend/fiber/DevToolsFiberComponentStack.js +++ b/packages/react-devtools-shared/src/backend/fiber/DevToolsFiberComponentStack.js @@ -108,6 +108,23 @@ export function getStackByFiberInDevAndProd( } } +export function getSourceLocationByFiber( + workTagMap: WorkTagMap, + fiber: Fiber, + currentDispatcherRef: CurrentDispatcherRef, +): null | string { + // This is like getStackByFiberInDevAndProd but just the first stack frame. + try { + const info = describeFiber(workTagMap, fiber, currentDispatcherRef); + if (info !== '') { + return info.slice(1); // skip the leading newline + } + } catch (x) { + console.error(x); + } + return null; +} + export function supportsConsoleTasks(fiber: Fiber): boolean { // If this Fiber supports native console.createTask then we are already running // inside a native async stack trace if it's active - meaning the DevTools is open. diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 62eacdf5db..729c8fce5a 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -101,6 +101,14 @@ import { import {enableStyleXFeatures} from 'react-devtools-feature-flags'; import is from 'shared/objectIs'; import hasOwnProperty from 'shared/hasOwnProperty'; + +// $FlowFixMe[method-unbinding] +const toString = Object.prototype.toString; + +function isError(object: mixed) { + return toString.call(object) === '[object Error]'; +} + import {getStyleXData} from '../StyleX/utils'; import {createProfilingHooks} from '../profilingHooks'; @@ -131,7 +139,8 @@ import type { Plugins, } from 'react-devtools-shared/src/frontend/types'; import type {Source} from 'react-devtools-shared/src/shared/types'; -import {getStackByFiberInDevAndProd} from './DevToolsFiberComponentStack'; +import {getSourceLocationByFiber} from './DevToolsFiberComponentStack'; +import {formatOwnerStack} from '../shared/DevToolsOwnerStack'; // Kinds const FIBER_INSTANCE = 0; @@ -149,12 +158,12 @@ type FiberInstance = { id: number, parent: null | DevToolsInstance, // filtered parent, including virtual firstChild: null | DevToolsInstance, // filtered first child, including virtual - previousSibling: null | DevToolsInstance, // filtered next sibling, including virtual nextSibling: null | DevToolsInstance, // filtered next sibling, including virtual flags: number, // Force Error/Suspense - componentStack: null | string, + source: null | string | Error | Source, // source location of this component function, or owned child stack errors: null | Map, // error messages and count warnings: null | Map, // warning messages and count + treeBaseDuration: number, // the profiled time of the last render of this subtree data: Fiber, // one of a Fiber pair }; @@ -164,12 +173,12 @@ function createFiberInstance(fiber: Fiber): FiberInstance { id: getUID(), parent: null, firstChild: null, - previousSibling: null, nextSibling: null, flags: 0, - componentStack: null, + source: null, errors: null, warnings: null, + treeBaseDuration: 0, data: fiber, }; } @@ -184,15 +193,15 @@ type VirtualInstance = { id: number, parent: null | DevToolsInstance, // filtered parent, including virtual firstChild: null | DevToolsInstance, // filtered first child, including virtual - previousSibling: null | DevToolsInstance, // filtered next sibling, including virtual nextSibling: null | DevToolsInstance, // filtered next sibling, including virtual flags: number, - componentStack: null | string, + source: null | string | Error | Source, // source location of this server component, or owned child stack // Errors and Warnings happen per ReactComponentInfo which can appear in // multiple places but we track them per stateful VirtualInstance so // that old errors/warnings don't disappear when the instance is refreshed. errors: null | Map, // error messages and count warnings: null | Map, // warning messages and count + treeBaseDuration: number, // the profiled time of the last render of this subtree // The latest info for this instance. This can be updated over time and the // same info can appear in more than once ServerComponentInstance. data: ReactComponentInfo, @@ -206,12 +215,12 @@ function createVirtualInstance( id: getUID(), parent: null, firstChild: null, - previousSibling: null, nextSibling: null, flags: 0, - componentStack: null, + source: null, errors: null, warnings: null, + treeBaseDuration: 0, data: debugEntry, }; } @@ -1075,8 +1084,6 @@ export function attach( ' '.repeat(indent) + '- ' + instance.id + ' (' + name + ')', 'parent', instance.parent === null ? ' ' : instance.parent.id, - 'prev', - instance.previousSibling === null ? ' ' : instance.previousSibling.id, 'next', instance.nextSibling === null ? ' ' : instance.nextSibling.id, ); @@ -1346,16 +1353,6 @@ export function attach( } } - // When profiling is supported, we store the latest tree base durations for each Fiber. - // This is so that we can quickly capture a snapshot of those values if profiling starts. - // If we didn't store these values, we'd have to crawl the tree when profiling started, - // and use a slow path to find each of the current Fibers. - const idToTreeBaseDurationMap: Map = new Map(); - - // When profiling is supported, we store the latest tree base durations for each Fiber. - // This map enables us to filter these times by root when sending them to the frontend. - const idToRootMap: Map = new Map(); - // When a mount or update is in progress, this value tracks the root that is being operated on. let currentRootID: number = -1; @@ -2154,6 +2151,16 @@ export function attach( parentInstance, debugOwner, ); + if ( + ownerInstance !== null && + debugOwner === fiber._debugOwner && + fiber._debugStack != null && + ownerInstance.source === null + ) { + // The new Fiber is directly owned by the ownerInstance. Therefore somewhere on + // the debugStack will be a stack frame inside the ownerInstance's source. + ownerInstance.source = fiber._debugStack; + } const ownerID = ownerInstance === null ? 0 : ownerInstance.id; const parentID = parentInstance ? parentInstance.id : 0; @@ -2192,9 +2199,7 @@ export function attach( } if (isProfilingSupported) { - idToRootMap.set(id, currentRootID); - - recordProfilingDurations(fiber); + recordProfilingDurations(fiberInstance); } return fiberInstance; } @@ -2207,8 +2212,6 @@ export function attach( idToDevToolsInstanceMap.set(id, instance); - const isProfilingSupported = false; // TODO: Support Tree Base Duration Based on Children. - const componentInfo = instance.data; const key = @@ -2228,6 +2231,16 @@ export function attach( // away so maybe it's not so bad. const debugOwner = getUnfilteredOwner(componentInfo); const ownerInstance = findNearestOwnerInstance(parentInstance, debugOwner); + if ( + ownerInstance !== null && + debugOwner === componentInfo.owner && + componentInfo.debugStack != null && + ownerInstance.source === null + ) { + // The new Fiber is directly owned by the ownerInstance. Therefore somewhere on + // the debugStack will be a stack frame inside the ownerInstance's source. + ownerInstance.source = componentInfo.debugStack; + } const ownerID = ownerInstance === null ? 0 : ownerInstance.id; const parentID = parentInstance ? parentInstance.id : 0; @@ -2245,12 +2258,6 @@ export function attach( pushOperation(ownerID); pushOperation(displayNameStringID); pushOperation(keyStringID); - - if (isProfilingSupported) { - idToRootMap.set(id, currentRootID); - // TODO: Include tree base duration of children somehow. - // recordProfilingDurations(...); - } } function recordUnmount(fiberInstance: FiberInstance): void { @@ -2285,12 +2292,6 @@ export function attach( } untrackFiber(fiberInstance); - - const isProfilingSupported = fiber.hasOwnProperty('treeBaseDuration'); - if (isProfilingSupported) { - idToRootMap.delete(id); - idToTreeBaseDurationMap.delete(id); - } } // Running state of the remaining children from the previous version of this parent that @@ -2314,21 +2315,25 @@ export function attach( if (previouslyReconciledSibling === null) { previouslyReconciledSibling = instance; parentInstance.firstChild = instance; - instance.previousSibling = null; } else { previouslyReconciledSibling.nextSibling = instance; - instance.previousSibling = previouslyReconciledSibling; previouslyReconciledSibling = instance; } instance.nextSibling = null; } - function moveChild(instance: DevToolsInstance): void { - removeChild(instance); + function moveChild( + instance: DevToolsInstance, + previousSibling: null | DevToolsInstance, + ): void { + removeChild(instance, previousSibling); insertChild(instance); } - function removeChild(instance: DevToolsInstance): void { + function removeChild( + instance: DevToolsInstance, + previousSibling: null | DevToolsInstance, + ): void { if (instance.parent === null) { if (remainingReconcilingChildren === instance) { throw new Error( @@ -2336,8 +2341,6 @@ export function attach( ); } else if (instance.nextSibling !== null) { throw new Error('A deleted instance should not have next siblings'); - } else if (instance.previousSibling !== null) { - throw new Error('A deleted instance should not have previous siblings'); } // Already deleted. return; @@ -2353,7 +2356,7 @@ export function attach( } // Remove an existing child from its current position, which we assume is in the // remainingReconcilingChildren set. - if (instance.previousSibling === null) { + if (previousSibling === null) { // We're first in the remaining set. Remove us. if (remainingReconcilingChildren !== instance) { throw new Error( @@ -2362,13 +2365,9 @@ export function attach( } remainingReconcilingChildren = instance.nextSibling; } else { - instance.previousSibling.nextSibling = instance.nextSibling; - } - if (instance.nextSibling !== null) { - instance.nextSibling.previousSibling = instance.previousSibling; + previousSibling.nextSibling = instance.nextSibling; } instance.nextSibling = null; - instance.previousSibling = null; instance.parent = null; } @@ -2401,6 +2400,8 @@ export function attach( traceNearestHostComponentUpdate, virtualLevel + 1, ); + // Must be called after all children have been appended. + recordVirtualProfilingDurations(virtualInstance); } finally { reconcilingParent = stashedParent; previouslyReconciledSibling = stashedPrevious; @@ -2416,12 +2417,6 @@ export function attach( const id = instance.id; pendingRealUnmountedIDs.push(id); - - const isProfilingSupported = false; // TODO: Profiling support. - if (isProfilingSupported) { - idToRootMap.delete(id); - idToTreeBaseDurationMap.delete(id); - } } function mountVirtualChildrenRecursively( @@ -2652,14 +2647,15 @@ export function attach( } else { recordVirtualUnmount(instance); } - removeChild(instance); + removeChild(instance, null); } - function recordProfilingDurations(fiber: Fiber) { - const id = getFiberIDThrows(fiber); + function recordProfilingDurations(fiberInstance: FiberInstance) { + const id = fiberInstance.id; + const fiber = fiberInstance.data; const {actualDuration, treeBaseDuration} = fiber; - idToTreeBaseDurationMap.set(id, treeBaseDuration || 0); + fiberInstance.treeBaseDuration = treeBaseDuration || 0; if (isProfiling) { const {alternate} = fiber; @@ -2722,6 +2718,38 @@ export function attach( } } + function recordVirtualProfilingDurations(virtualInstance: VirtualInstance) { + const id = virtualInstance.id; + + let treeBaseDuration = 0; + // Add up the base duration of the child instances. The virtual base duration + // will be the same as children's duration since we don't take up any render + // time in the virtual instance. + for ( + let child = virtualInstance.firstChild; + child !== null; + child = child.nextSibling + ) { + treeBaseDuration += child.treeBaseDuration; + } + + if (isProfiling) { + const previousTreeBaseDuration = virtualInstance.treeBaseDuration; + if (treeBaseDuration !== previousTreeBaseDuration) { + // Tree base duration updates are included in the operations typed array. + // So we have to convert them from milliseconds to microseconds so we can send them as ints. + const convertedTreeBaseDuration = Math.floor( + (treeBaseDuration || 0) * 1000, + ); + pushOperation(TREE_OPERATION_UPDATE_TREE_BASE_DURATION); + pushOperation(id); + pushOperation(convertedTreeBaseDuration); + } + } + + virtualInstance.treeBaseDuration = treeBaseDuration; + } + function recordResetChildren(parentInstance: DevToolsInstance) { if (__DEBUG__) { if ( @@ -2789,6 +2817,8 @@ export function attach( ) { recordResetChildren(virtualInstance); } + // Must be called after all children have been appended. + recordVirtualProfilingDurations(virtualInstance); } finally { unmountRemainingChildren(); reconcilingParent = stashedParent; @@ -2851,8 +2881,7 @@ export function attach( ); } } - // TODO: Find the best matching existing child based on the key if defined. - + let previousSiblingOfBestMatch = null; let bestMatch = remainingReconcilingChildren; if (componentInfo.key != null) { // If there is a key try to find a matching key in the set. @@ -2864,6 +2893,7 @@ export function attach( ) { break; } + previousSiblingOfBestMatch = bestMatch; bestMatch = bestMatch.nextSibling; } } @@ -2878,7 +2908,7 @@ export function attach( // with the same name, then we claim it and reuse it for this update. // Update it with the latest entry. bestMatch.data = componentInfo; - moveChild(bestMatch); + moveChild(bestMatch, previousSiblingOfBestMatch); previousVirtualInstance = bestMatch; previousVirtualInstanceWasMount = false; } else { @@ -2927,26 +2957,59 @@ export function attach( } previousVirtualInstance = null; } + // We've reached the end of the virtual levels, but not beyond, // and now continue with the regular fiber. + + // Do a fast pass over the remaining children to find the previous instance. + // TODO: This doesn't have the best O(n) for a large set of children that are + // reordered. Consider using a temporary map if it's not the very next one. + let prevChild; if (prevChildAtSameIndex === nextChild) { // This set is unchanged. We're just going through it to place all the // children again. - if ( - updateFiberRecursively( - nextChild, - nextChild, - traceNearestHostComponentUpdate, - ) - ) { - throw new Error('Updating the same fiber should not cause reorder'); + prevChild = nextChild; + } else { + // We don't actually need to rely on the alternate here. We could also + // reconcile against stateNode, key or whatever. Doesn't have to be same + // Fiber pair. + prevChild = nextChild.alternate; + } + let previousSiblingOfExistingInstance = null; + let existingInstance = null; + if (prevChild !== null) { + existingInstance = remainingReconcilingChildren; + while (existingInstance !== null) { + if (existingInstance.data === prevChild) { + break; + } + previousSiblingOfExistingInstance = existingInstance; + existingInstance = existingInstance.nextSibling; } - } else if (nextChild.alternate) { - const prevChild = nextChild.alternate; + } + if (existingInstance !== null) { + // Common case. Match in the same parent. + const fiberInstance: FiberInstance = (existingInstance: any); // Only matches if it's a Fiber. + + // We keep track if the order of the children matches the previous order. + // They are always different referentially, but if the instances line up + // conceptually we'll want to know that. + if (prevChild !== prevChildAtSameIndex) { + shouldResetChildren = true; + } + + // Register the new alternate in case it's not already in. + fiberToFiberInstanceMap.set(nextChild, fiberInstance); + + // Update the Fiber so we that we always keep the current Fiber on the data. + fiberInstance.data = nextChild; + moveChild(fiberInstance, previousSiblingOfExistingInstance); + if ( updateFiberRecursively( + fiberInstance, nextChild, - prevChild, + (prevChild: any), traceNearestHostComponentUpdate, ) ) { @@ -2955,14 +3018,32 @@ export function attach( // propagate the need to reset child order upwards to this Fiber. shouldResetChildren = true; } - // However we also keep track if the order of the children matches - // the previous order. They are always different referentially, but - // if the instances line up conceptually we'll want to know that. - if (prevChild !== prevChildAtSameIndex) { + } else if (prevChild !== null && shouldFilterFiber(nextChild)) { + // If this Fiber should be filtered, we need to still update its children. + // This relies on an alternate since we don't have an Instance with the previous + // child on it. Ideally, the reconciliation wouldn't need previous Fibers that + // are filtered from the tree. + if ( + updateFiberRecursively( + null, + nextChild, + prevChild, + traceNearestHostComponentUpdate, + ) + ) { shouldResetChildren = true; } } else { + // It's possible for a FiberInstance to be reparented when virtual parents + // get their sequence split or change structure with the same render result. + // In this case we unmount the and remount the FiberInstances. + // This might cause us to lose the selection but it's an edge case. + + // We let the previous instance remain in the "remaining queue" it is + // in to be deleted at the end since it'll have no match. + mountFiberRecursively(nextChild, traceNearestHostComponentUpdate); + // Need to mark the parent set to remount the new instance. shouldResetChildren = true; } } @@ -3021,6 +3102,7 @@ export function attach( // Returns whether closest unfiltered fiber parent needs to reset its child list. function updateFiberRecursively( + fiberInstance: null | FiberInstance, // null if this should be filtered nextFiber: Fiber, prevFiber: Fiber, traceNearestHostComponentUpdate: boolean, @@ -3054,34 +3136,10 @@ export function attach( } } - let fiberInstance: null | FiberInstance = null; - const shouldIncludeInTree = !shouldFilterFiber(nextFiber); - if (shouldIncludeInTree) { - const entry = fiberToFiberInstanceMap.get(prevFiber); - if (entry !== undefined && entry.parent === reconcilingParent) { - // Common case. Match in the same parent. - fiberInstance = entry; - // Register the new alternate in case it's not already in. - fiberToFiberInstanceMap.set(nextFiber, fiberInstance); - - // Update the Fiber so we that we always keep the current Fiber on the data. - fiberInstance.data = nextFiber; - moveChild(fiberInstance); - } else { - // It's possible for a FiberInstance to be reparented when virtual parents - // get their sequence split or change structure with the same render result. - // In this case we unmount the and remount the FiberInstances. - // This might cause us to lose the selection but it's an edge case. - - // We let the previous instance remain in the "remaining queue" it is - // in to be deleted at the end since it'll have no match. - - mountFiberRecursively(nextFiber, traceNearestHostComponentUpdate); - - // Need to mark the parent set to remount the new instance. - return true; - } - + const stashedParent = reconcilingParent; + const stashedPrevious = previouslyReconciledSibling; + const stashedRemaining = remainingReconcilingChildren; + if (fiberInstance !== null) { if ( mostRecentlyInspectedElement !== null && mostRecentlyInspectedElement.id === fiberInstance.id && @@ -3091,12 +3149,6 @@ export function attach( // If it is inspected again, it may need to be re-run to obtain updated hooks values. hasElementUpdatedSinceLastInspected = true; } - } - - const stashedParent = reconcilingParent; - const stashedPrevious = previouslyReconciledSibling; - const stashedRemaining = remainingReconcilingChildren; - if (fiberInstance !== null) { // Push a new DevTools instance parent while reconciling this subtree. reconcilingParent = fiberInstance; previouslyReconciledSibling = null; @@ -3151,7 +3203,7 @@ export function attach( if ( nextFallbackChildSet != null && prevFallbackChildSet != null && - updateFiberRecursively( + updateChildrenRecursively( nextFallbackChildSet, prevFallbackChildSet, traceNearestHostComponentUpdate, @@ -3236,20 +3288,18 @@ export function attach( } } - if (shouldIncludeInTree) { + if (fiberInstance !== null) { const isProfilingSupported = nextFiber.hasOwnProperty('treeBaseDuration'); if (isProfilingSupported) { - recordProfilingDurations(nextFiber); + recordProfilingDurations(fiberInstance); } } if (shouldResetChildren) { // We need to crawl the subtree for closest non-filtered Fibers // so that we can display them in a flat children set. - if (shouldIncludeInTree) { - if (reconcilingParent !== null) { - recordResetChildren(reconcilingParent); - } + if (fiberInstance !== null) { + recordResetChildren(fiberInstance); // We've handled the child order change for this Fiber. // Since it's included, there's no need to invalidate parent child order. return false; @@ -3261,7 +3311,7 @@ export function attach( return false; } } finally { - if (shouldIncludeInTree) { + if (fiberInstance !== null) { unmountRemainingChildren(); reconcilingParent = stashedParent; previouslyReconciledSibling = stashedPrevious; @@ -3451,7 +3501,7 @@ export function attach( mountFiberRecursively(current, false); } else if (wasMounted && isMounted) { // Update an existing root. - updateFiberRecursively(current, alternate, false); + updateFiberRecursively(rootInstance, current, alternate, false); } else if (wasMounted && !isMounted) { // Unmount an existing root. removeRootPseudoKey(currentRootID); @@ -3512,7 +3562,7 @@ export function attach( fiberInstance: FiberInstance, ): $ReadOnlyArray { const hostInstances = []; - const fiber = findCurrentFiberUsingSlowPathByFiberInstance(fiberInstance); + const fiber = fiberInstance.data; if (!fiber) { return hostInstances; } @@ -3563,8 +3613,7 @@ export function attach( // TODO: Handle VirtualInstance. return null; } - const fiber = - findCurrentFiberUsingSlowPathByFiberInstance(devtoolsInstance); + const fiber = devtoolsInstance.data; if (fiber === null) { return null; } @@ -3672,229 +3721,28 @@ export function attach( return null; } - // This function is copied from React and should be kept in sync: - // https://github.com/facebook/react/blob/main/packages/react-reconciler/src/ReactFiberTreeReflection.js - function assertIsMounted(fiber: Fiber) { - if (getNearestMountedFiber(fiber) !== fiber) { - throw new Error('Unable to find node on an unmounted component.'); - } - } - - // This function is copied from React and should be kept in sync: - // https://github.com/facebook/react/blob/main/packages/react-reconciler/src/ReactFiberTreeReflection.js - function getNearestMountedFiber(fiber: Fiber): null | Fiber { - let node = fiber; - let nearestMounted: null | Fiber = fiber; - if (!fiber.alternate) { - // If there is no alternate, this might be a new tree that isn't inserted - // yet. If it is, then it will have a pending insertion effect on it. - let nextNode: Fiber = node; - do { - node = nextNode; - // TODO: This function, and these flags, are a leaked implementation - // detail. Once we start releasing DevTools in lockstep with React, we - // should import a function from the reconciler instead. - const Placement = 0b000000000000000000000000010; - const Hydrating = 0b000000000000001000000000000; - if ((node.flags & (Placement | Hydrating)) !== 0) { - // This is an insertion or in-progress hydration. The nearest possible - // mounted fiber is the parent but we need to continue to figure out - // if that one is still mounted. - nearestMounted = node.return; - } - // $FlowFixMe[incompatible-type] we bail out when we get a null - nextNode = node.return; - } while (nextNode); - } else { - while (node.return) { - node = node.return; - } - } - if (node.tag === HostRoot) { - // TODO: Check if this was a nested HostRoot when used with - // renderContainerIntoSubtree. - return nearestMounted; - } - // If we didn't hit the root, that means that we're in an disconnected tree - // that has been unmounted. - return null; - } - - // This function is copied from React and should be kept in sync: - // https://github.com/facebook/react/blob/main/packages/react-reconciler/src/ReactFiberTreeReflection.js - // It would be nice if we updated React to inject this function directly (vs just indirectly via findDOMNode). - // BEGIN copied code - function findCurrentFiberUsingSlowPathByFiberInstance( - fiberInstance: FiberInstance, - ): Fiber | null { - const fiber = fiberInstance.data; - const alternate = fiber.alternate; - if (!alternate) { - // If there is no alternate, then we only need to check if it is mounted. - const nearestMounted = getNearestMountedFiber(fiber); - - if (nearestMounted === null) { - throw new Error('Unable to find node on an unmounted component.'); - } - - if (nearestMounted !== fiber) { - return null; - } - return fiber; - } - // If we have two possible branches, we'll walk backwards up to the root - // to see what path the root points to. On the way we may hit one of the - // special cases and we'll deal with them. - let a: Fiber = fiber; - let b: Fiber = alternate; - while (true) { - const parentA = a.return; - if (parentA === null) { - // We're at the root. - break; - } - const parentB = parentA.alternate; - if (parentB === null) { - // There is no alternate. This is an unusual case. Currently, it only - // happens when a Suspense component is hidden. An extra fragment fiber - // is inserted in between the Suspense fiber and its children. Skip - // over this extra fragment fiber and proceed to the next parent. - const nextParent = parentA.return; - if (nextParent !== null) { - a = b = nextParent; - continue; - } - // If there's no parent, we're at the root. - break; - } - - // If both copies of the parent fiber point to the same child, we can - // assume that the child is current. This happens when we bailout on low - // priority: the bailed out fiber's child reuses the current child. - if (parentA.child === parentB.child) { - let child = parentA.child; - while (child) { - if (child === a) { - // We've determined that A is the current branch. - assertIsMounted(parentA); - return fiber; - } - if (child === b) { - // We've determined that B is the current branch. - assertIsMounted(parentA); - return alternate; - } - child = child.sibling; - } - - // We should never have an alternate for any mounting node. So the only - // way this could possibly happen is if this was unmounted, if at all. - throw new Error('Unable to find node on an unmounted component.'); - } - - if (a.return !== b.return) { - // The return pointer of A and the return pointer of B point to different - // fibers. We assume that return pointers never criss-cross, so A must - // belong to the child set of A.return, and B must belong to the child - // set of B.return. - a = parentA; - b = parentB; - } else { - // The return pointers point to the same fiber. We'll have to use the - // default, slow path: scan the child sets of each parent alternate to see - // which child belongs to which set. - // - // Search parent A's child set - let didFindChild = false; - let child = parentA.child; - while (child) { - if (child === a) { - didFindChild = true; - a = parentA; - b = parentB; - break; - } - if (child === b) { - didFindChild = true; - b = parentA; - a = parentB; - break; - } - child = child.sibling; - } - if (!didFindChild) { - // Search parent B's child set - child = parentB.child; - while (child) { - if (child === a) { - didFindChild = true; - a = parentB; - b = parentA; - break; - } - if (child === b) { - didFindChild = true; - b = parentB; - a = parentA; - break; - } - child = child.sibling; - } - - if (!didFindChild) { - throw new Error( - 'Child was not found in either parent set. This indicates a bug ' + - 'in React related to the return pointer. Please file an issue.', - ); - } - } - } - - if (a.alternate !== b) { - throw new Error( - "Return fibers should always be each others' alternates. " + - 'This error is likely caused by a bug in React. Please file an issue.', - ); - } - } - - // If the root is not a host container, we're in a disconnected tree. I.e. - // unmounted. - if (a.tag !== HostRoot) { - throw new Error('Unable to find node on an unmounted component.'); - } - - if (a.stateNode.current === a) { - // We've determined that A is the current branch. - return fiber; - } - // Otherwise B has to be current branch. - return alternate; - } - - // END copied code - - function prepareViewAttributeSource( + function getElementAttributeByPath( id: number, path: Array, - ): void { + ): mixed { if (isMostRecentlyInspectedElement(id)) { - window.$attribute = getInObject( + return getInObject( ((mostRecentlyInspectedElement: any): InspectedElement), path, ); } + return undefined; } - function prepareViewElementSource(id: number): void { + function getElementSourceFunctionById(id: number): null | Function { const devtoolsInstance = idToDevToolsInstanceMap.get(id); if (devtoolsInstance === undefined) { console.warn(`Could not find DevToolsInstance with id "${id}"`); - return; + return null; } if (devtoolsInstance.kind !== FIBER_INSTANCE) { // TODO: Handle VirtualInstance. - return; + return null; } const fiber = devtoolsInstance.data; @@ -3906,21 +3754,16 @@ export function attach( case IncompleteFunctionComponent: case IndeterminateComponent: case FunctionComponent: - global.$type = type; - break; + return type; case ForwardRef: - global.$type = type.render; - break; + return type.render; case MemoComponent: case SimpleMemoComponent: - global.$type = - elementType != null && elementType.type != null - ? elementType.type - : type; - break; + return elementType != null && elementType.type != null + ? elementType.type + : type; default: - global.$type = null; - break; + return null; } } @@ -4062,8 +3905,7 @@ export function attach( return {instance, style}; } - const fiber = - findCurrentFiberUsingSlowPathByFiberInstance(devtoolsInstance); + const fiber = devtoolsInstance.data; if (fiber !== null) { instance = fiber.stateNode; @@ -4119,7 +3961,7 @@ export function attach( function inspectFiberInstanceRaw( fiberInstance: FiberInstance, ): InspectedElement | null { - const fiber = findCurrentFiberUsingSlowPathByFiberInstance(fiberInstance); + const fiber = fiberInstance.data; if (fiber == null) { return null; } @@ -4328,7 +4170,7 @@ export function attach( let source = null; if (canViewSource) { - source = getSourceForFiber(fiber); + source = getSourceForFiberInstance(fiberInstance); } return { @@ -4402,7 +4244,8 @@ export function attach( function inspectVirtualInstanceRaw( virtualInstance: VirtualInstance, ): InspectedElement | null { - const canViewSource = false; + const canViewSource = true; + const source = getSourceForInstance(virtualInstance); const componentInfo = virtualInstance.data; const key = @@ -4442,9 +4285,6 @@ export function attach( stylex: null, }; - // TODO: Support getting the source location from the owner stack. - const source = null; - return { id: virtualInstance.id, @@ -4881,8 +4721,7 @@ export function attach( // TODO: Handle VirtualInstance. return; } - const fiber = - findCurrentFiberUsingSlowPathByFiberInstance(devtoolsInstance); + const fiber = devtoolsInstance.data; if (fiber !== null) { const instance = fiber.stateNode; @@ -4947,8 +4786,7 @@ export function attach( // TODO: Handle VirtualInstance. return; } - const fiber = - findCurrentFiberUsingSlowPathByFiberInstance(devtoolsInstance); + const fiber = devtoolsInstance.data; if (fiber !== null) { const instance = fiber.stateNode; @@ -5023,8 +4861,7 @@ export function attach( // TODO: Handle VirtualInstance. return; } - const fiber = - findCurrentFiberUsingSlowPathByFiberInstance(devtoolsInstance); + const fiber = devtoolsInstance.data; if (fiber !== null) { const instance = fiber.stateNode; @@ -5098,8 +4935,8 @@ export function attach( let currentCommitProfilingMetadata: CommitProfilingData | null = null; let displayNamesByRootID: DisplayNamesByRootID | null = null; let idToContextsMap: Map | null = null; - let initialTreeBaseDurationsMap: Map | null = null; - let initialIDToRootMap: Map | null = null; + let initialTreeBaseDurationsMap: Map> | null = + null; let isProfiling: boolean = false; let profilingStartTime: number = 0; let recordChangeDescriptions: boolean = false; @@ -5118,24 +4955,15 @@ export function attach( rootToCommitProfilingMetadataMap.forEach( (commitProfilingMetadata, rootID) => { const commitData: Array = []; - const initialTreeBaseDurations: Array<[number, number]> = []; const displayName = (displayNamesByRootID !== null && displayNamesByRootID.get(rootID)) || 'Unknown'; - if (initialTreeBaseDurationsMap != null) { - initialTreeBaseDurationsMap.forEach((treeBaseDuration, id) => { - if ( - initialIDToRootMap != null && - initialIDToRootMap.get(id) === rootID - ) { - // We don't need to convert milliseconds to microseconds in this case, - // because the profiling summary is JSON serialized. - initialTreeBaseDurations.push([id, treeBaseDuration]); - } - }); - } + const initialTreeBaseDurations: Array<[number, number]> = + (initialTreeBaseDurationsMap !== null && + initialTreeBaseDurationsMap.get(rootID)) || + []; commitProfilingMetadata.forEach((commitProfilingData, commitIndex) => { const { @@ -5222,6 +5050,22 @@ export function attach( }; } + function snapshotTreeBaseDurations( + instance: DevToolsInstance, + target: Array<[number, number]>, + ) { + // We don't need to convert milliseconds to microseconds in this case, + // because the profiling summary is JSON serialized. + target.push([instance.id, instance.treeBaseDuration]); + for ( + let child = instance.firstChild; + child !== null; + child = child.nextSibling + ) { + snapshotTreeBaseDurations(child, target); + } + } + function startProfiling(shouldRecordChangeDescriptions: boolean) { if (isProfiling) { return; @@ -5234,16 +5078,19 @@ export function attach( // since either of these may change during the profiling session // (e.g. when a fiber is re-rendered or when a fiber gets removed). displayNamesByRootID = new Map(); - initialTreeBaseDurationsMap = new Map(idToTreeBaseDurationMap); - initialIDToRootMap = new Map(idToRootMap); + initialTreeBaseDurationsMap = new Map(); idToContextsMap = new Map(); hook.getFiberRoots(rendererID).forEach(root => { - const rootID = getFiberIDThrows(root.current); + const rootInstance = getFiberInstanceThrows(root.current); + const rootID = rootInstance.id; ((displayNamesByRootID: any): DisplayNamesByRootID).set( rootID, getDisplayNameForRoot(root.current), ); + const initialTreeBaseDurations: Array<[number, number]> = []; + snapshotTreeBaseDurations(rootInstance, initialTreeBaseDurations); + (initialTreeBaseDurationsMap: any).set(rootID, initialTreeBaseDurations); if (shouldRecordChangeDescriptions) { // Record all contexts at the time profiling is started. @@ -5668,39 +5515,63 @@ export function attach( return idToDevToolsInstanceMap.has(id); } - function getComponentStackForFiber(fiber: Fiber): string | null { - // TODO: This should really just take an DevToolsInstance directly. - let fiberInstance = fiberToFiberInstanceMap.get(fiber); - if (fiberInstance === undefined && fiber.alternate !== null) { - fiberInstance = fiberToFiberInstanceMap.get(fiber.alternate); - } - if (fiberInstance === undefined) { - // We're no longer tracking this instance. - return null; - } - if (fiberInstance.componentStack !== null) { - // Cached entry. - return fiberInstance.componentStack; + function getSourceForFiberInstance( + fiberInstance: FiberInstance, + ): Source | null { + const unresolvedSource = fiberInstance.source; + if ( + unresolvedSource !== null && + typeof unresolvedSource === 'object' && + !isError(unresolvedSource) + ) { + // $FlowFixMe: isError should have refined it. + return unresolvedSource; } const dispatcherRef = getDispatcherRef(renderer); - if (dispatcherRef == null) { - return null; + const stackFrame = + dispatcherRef == null + ? null + : getSourceLocationByFiber( + ReactTypeOfWork, + fiberInstance.data, + dispatcherRef, + ); + if (stackFrame === null) { + // If we don't find a source location by throwing, try to get one + // from an owned child if possible. This is the same branch as + // for virtual instances. + return getSourceForInstance(fiberInstance); } - - return (fiberInstance.componentStack = getStackByFiberInDevAndProd( - ReactTypeOfWork, - fiber, - dispatcherRef, - )); + const source = parseSourceFromComponentStack(stackFrame); + fiberInstance.source = source; + return source; } - function getSourceForFiber(fiber: Fiber): Source | null { - const componentStack = getComponentStackForFiber(fiber); - if (componentStack == null) { + function getSourceForInstance(instance: DevToolsInstance): Source | null { + let unresolvedSource = instance.source; + if (unresolvedSource === null) { + // We don't have any source yet. We can try again later in case an owned child mounts later. + // TODO: We won't have any information here if the child is filtered. return null; } - return parseSourceFromComponentStack(componentStack); + // If we have the debug stack (the creation stack of the JSX) for any owned child of this + // component, then at the bottom of that stack will be a stack frame that is somewhere within + // the component's function body. Typically it would be the callsite of the JSX unless there's + // any intermediate utility functions. This won't point to the top of the component function + // but it's at least somewhere within it. + if (isError(unresolvedSource)) { + unresolvedSource = formatOwnerStack((unresolvedSource: any)); + } + if (typeof unresolvedSource === 'string') { + const idx = unresolvedSource.lastIndexOf('\n'); + const lastLine = + idx === -1 ? unresolvedSource : unresolvedSource.slice(idx + 1); + return (instance.source = parseSourceFromComponentStack(lastLine)); + } + + // $FlowFixMe: refined. + return unresolvedSource; } return { @@ -5727,8 +5598,8 @@ export function attach( inspectElement, logElementToConsole, patchConsoleForStrictMode, - prepareViewAttributeSource, - prepareViewElementSource, + getElementAttributeByPath, + getElementSourceFunctionById, overrideError, overrideSuspense, overrideValueAtPath, diff --git a/packages/react-devtools-shared/src/backend/legacy/renderer.js b/packages/react-devtools-shared/src/backend/legacy/renderer.js index 1955465607..f8aa548a05 100644 --- a/packages/react-devtools-shared/src/backend/legacy/renderer.js +++ b/packages/react-devtools-shared/src/backend/legacy/renderer.js @@ -907,30 +907,31 @@ export function attach( } } - function prepareViewAttributeSource( + function getElementAttributeByPath( id: number, path: Array, - ): void { + ): mixed { const inspectedElement = inspectElementRaw(id); if (inspectedElement !== null) { - window.$attribute = getInObject(inspectedElement, path); + return getInObject(inspectedElement, path); } + return undefined; } - function prepareViewElementSource(id: number): void { + function getElementSourceFunctionById(id: number): null | Function { const internalInstance = idToInternalInstanceMap.get(id); if (internalInstance == null) { console.warn(`Could not find instance with id "${id}"`); - return; + return null; } const element = internalInstance._currentElement; if (element == null) { console.warn(`Could not find element with id "${id}"`); - return; + return null; } - global.$type = element.type; + return element.type; } function deletePath( @@ -1141,8 +1142,8 @@ export function attach( overrideValueAtPath, renamePath, patchConsoleForStrictMode, - prepareViewAttributeSource, - prepareViewElementSource, + getElementAttributeByPath, + getElementSourceFunctionById, renderer, setTraceUpdatesEnabled, setTrackedPath, diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index 2bd13a3a12..87b0f2048b 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -394,11 +394,11 @@ export type RendererInterface = { value: any, ) => void, patchConsoleForStrictMode: () => void, - prepareViewAttributeSource: ( + getElementAttributeByPath: ( id: number, path: Array, - ) => void, - prepareViewElementSource: (id: number) => void, + ) => mixed, + getElementSourceFunctionById: (id: number) => null | Function, renamePath: ( type: Type, id: number, diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/HoveredFiberInfo.js b/packages/react-devtools-shared/src/devtools/views/Profiler/HoveredFiberInfo.js index 51f82038d3..f1156a05ae 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/HoveredFiberInfo.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/HoveredFiberInfo.js @@ -93,7 +93,7 @@ export default function HoveredFiberInfo({fiberData}: Props): React.Node { )}
- {renderDurationInfo ||
Did not render.
} + {renderDurationInfo ||
Did not client render.
}
diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/SidebarSelectedFiberInfo.js b/packages/react-devtools-shared/src/devtools/views/Profiler/SidebarSelectedFiberInfo.js index 2eca7b0fda..d785cfebda 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/SidebarSelectedFiberInfo.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/SidebarSelectedFiberInfo.js @@ -142,7 +142,7 @@ export default function SidebarSelectedFiberInfo(): React.Node { )} {listItems.length === 0 && ( -
Did not render during this profiling session.
+
Did not render on the client during this profiling session.
)} diff --git a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js index 5a763ffe94..a109011b35 100644 --- a/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js +++ b/packages/react-dom/src/__tests__/ReactDOMFizzServer-test.js @@ -8677,4 +8677,65 @@ describe('ReactDOMFizzServer', () => { '\n in Bar (at **)' + '\n in Foo (at **)', ); }); + + it('can recover from very deep trees to avoid stack overflow', async () => { + function Recursive({n}) { + if (n > 0) { + return ; + } + return hi; + } + + // Recursively render a component tree deep enough to trigger stack overflow. + // Don't make this too short to not hit the limit but also not too deep to slow + // down the test. + await act(() => { + const {pipe} = renderToPipeableStream( +
+ +
, + ); + pipe(writable); + }); + + expect(getVisibleChildren(container)).toEqual( +
+ hi +
, + ); + }); + + it('handles stack overflows inside components themselves', async () => { + function StackOverflow() { + // This component is recursive inside itself and is therefore an error. + // Assuming no tail-call optimizations. + function recursive(n, a0, a1, a2, a3) { + if (n > 0) { + return recursive(n - 1, a0, a1, a2, a3) + a0 + a1 + a2 + a3; + } + return a0; + } + return recursive(10000, 'should', 'not', 'resolve', 'this'); + } + + let caughtError; + + await expect(async () => { + await act(() => { + const {pipe} = renderToPipeableStream( +
+ +
, + { + onError(error, errorInfo) { + caughtError = error; + }, + }, + ); + pipe(writable); + }); + }).rejects.toThrow('Maximum call stack size exceeded'); + + expect(caughtError.message).toBe('Maximum call stack size exceeded'); + }); }); diff --git a/packages/react-reconciler/src/__tests__/ReactSuspenseCallback-test.js b/packages/react-reconciler/src/__tests__/ReactSuspenseCallback-test.js index eae080d35c..6ddc94905a 100644 --- a/packages/react-reconciler/src/__tests__/ReactSuspenseCallback-test.js +++ b/packages/react-reconciler/src/__tests__/ReactSuspenseCallback-test.js @@ -46,7 +46,7 @@ describe('ReactSuspense', () => { // Warning don't fire in production, so this test passes in prod even if // the suspenseCallback feature is not enabled - // @gate www || !__DEV__ + // @gate enableSuspenseCallback || !__DEV__ it('check type', async () => { const {PromiseComp} = createThenable(); @@ -71,7 +71,7 @@ describe('ReactSuspense', () => { await expect(async () => await waitForAll([])).toErrorDev([]); }); - // @gate www + // @gate enableSuspenseCallback it('1 then 0 suspense callback', async () => { const {promise, resolve, PromiseComp} = createThenable(); @@ -98,7 +98,7 @@ describe('ReactSuspense', () => { expect(ops).toEqual([]); }); - // @gate www + // @gate enableSuspenseCallback it('2 then 1 then 0 suspense callback', async () => { const { promise: promise1, @@ -145,7 +145,7 @@ describe('ReactSuspense', () => { expect(ops).toEqual([]); }); - // @gate www + // @gate enableSuspenseCallback it('nested suspense promises are reported only for their tier', async () => { const {promise, PromiseComp} = createThenable(); @@ -177,7 +177,7 @@ describe('ReactSuspense', () => { expect(ops2).toEqual([new Set([promise])]); }); - // @gate www + // @gate enableSuspenseCallback it('competing suspense promises', async () => { const { promise: promise1, diff --git a/packages/react-server/src/ReactFizzServer.js b/packages/react-server/src/ReactFizzServer.js index 7ccbd65d16..aeffa6b813 100644 --- a/packages/react-server/src/ReactFizzServer.js +++ b/packages/react-server/src/ReactFizzServer.js @@ -3320,9 +3320,8 @@ function spawnNewSuspendedReplayTask( request: Request, task: ReplayTask, thenableState: ThenableState | null, - x: Wakeable, -): void { - const newTask = createReplayTask( +): ReplayTask { + return createReplayTask( request, thenableState, task.replay, @@ -3340,17 +3339,13 @@ function spawnNewSuspendedReplayTask( !disableLegacyContext ? task.legacyContext : emptyContextObject, __DEV__ && enableOwnerStacks ? task.debugTask : null, ); - - const ping = newTask.ping; - x.then(ping, ping); } function spawnNewSuspendedRenderTask( request: Request, task: RenderTask, thenableState: ThenableState | null, - x: Wakeable, -): void { +): RenderTask { // Something suspended, we'll need to create a new segment and resolve it later. const segment = task.blockedSegment; const insertionIndex = segment.chunks.length; @@ -3367,7 +3362,7 @@ function spawnNewSuspendedRenderTask( segment.children.push(newSegment); // Reset lastPushedText for current Segment since the new Segment "consumed" it segment.lastPushedText = false; - const newTask = createRenderTask( + return createRenderTask( request, thenableState, task.node, @@ -3385,9 +3380,6 @@ function spawnNewSuspendedRenderTask( !disableLegacyContext ? task.legacyContext : emptyContextObject, __DEV__ && enableOwnerStacks ? task.debugTask : null, ); - - const ping = newTask.ping; - x.then(ping, ping); } // This is a non-destructive form of rendering a node. If it suspends it spawns @@ -3436,13 +3428,47 @@ function renderNode( if (typeof x.then === 'function') { const wakeable: Wakeable = (x: any); const thenableState = getThenableStateAfterSuspending(); - spawnNewSuspendedReplayTask( + const newTask = spawnNewSuspendedReplayTask( request, // $FlowFixMe: Refined. task, thenableState, - wakeable, ); + const ping = newTask.ping; + wakeable.then(ping, ping); + + // Restore the context. We assume that this will be restored by the inner + // functions in case nothing throws so we don't use "finally" here. + task.formatContext = previousFormatContext; + if (!disableLegacyContext) { + task.legacyContext = previousLegacyContext; + } + task.context = previousContext; + task.keyPath = previousKeyPath; + task.treeContext = previousTreeContext; + task.componentStack = previousComponentStack; + if (__DEV__ && enableOwnerStacks) { + task.debugTask = previousDebugTask; + } + // Restore all active ReactContexts to what they were before. + switchContext(previousContext); + return; + } + if (x.message === 'Maximum call stack size exceeded') { + // This was a stack overflow. We do a lot of recursion in React by default for + // performance but it can lead to stack overflows in extremely deep trees. + // We do have the ability to create a trampoile if this happens which makes + // this kind of zero-cost. + const thenableState = getThenableStateAfterSuspending(); + const newTask = spawnNewSuspendedReplayTask( + request, + // $FlowFixMe: Refined. + task, + thenableState, + ); + + // Immediately schedule the task for retrying. + request.pingedTasks.push(newTask); // Restore the context. We assume that this will be restored by the inner // functions in case nothing throws so we don't use "finally" here. @@ -3493,13 +3519,14 @@ function renderNode( if (typeof x.then === 'function') { const wakeable: Wakeable = (x: any); const thenableState = getThenableStateAfterSuspending(); - spawnNewSuspendedRenderTask( + const newTask = spawnNewSuspendedRenderTask( request, // $FlowFixMe: Refined. task, thenableState, - wakeable, ); + const ping = newTask.ping; + wakeable.then(ping, ping); // Restore the context. We assume that this will be restored by the inner // functions in case nothing throws so we don't use "finally" here. @@ -3540,6 +3567,39 @@ function renderNode( ); trackPostpone(request, trackedPostpones, task, postponedSegment); + // Restore the context. We assume that this will be restored by the inner + // functions in case nothing throws so we don't use "finally" here. + task.formatContext = previousFormatContext; + if (!disableLegacyContext) { + task.legacyContext = previousLegacyContext; + } + task.context = previousContext; + task.keyPath = previousKeyPath; + task.treeContext = previousTreeContext; + task.componentStack = previousComponentStack; + if (__DEV__ && enableOwnerStacks) { + task.debugTask = previousDebugTask; + } + // Restore all active ReactContexts to what they were before. + switchContext(previousContext); + return; + } + if (x.message === 'Maximum call stack size exceeded') { + // This was a stack overflow. We do a lot of recursion in React by default for + // performance but it can lead to stack overflows in extremely deep trees. + // We do have the ability to create a trampoile if this happens which makes + // this kind of zero-cost. + const thenableState = getThenableStateAfterSuspending(); + const newTask = spawnNewSuspendedRenderTask( + request, + // $FlowFixMe: Refined. + task, + thenableState, + ); + + // Immediately schedule the task for retrying. + request.pingedTasks.push(newTask); + // Restore the context. We assume that this will be restored by the inner // functions in case nothing throws so we don't use "finally" here. task.formatContext = previousFormatContext; diff --git a/packages/shared/forks/ReactFeatureFlags.native-fb.js b/packages/shared/forks/ReactFeatureFlags.native-fb.js index 6ba5a79ff0..61138ddbee 100644 --- a/packages/shared/forks/ReactFeatureFlags.native-fb.js +++ b/packages/shared/forks/ReactFeatureFlags.native-fb.js @@ -80,7 +80,7 @@ export const enableScopeAPI = false; export const enableServerComponentLogs = true; export const enableSuspenseAvoidThisFallback = false; export const enableSuspenseAvoidThisFallbackFizz = false; -export const enableSuspenseCallback = false; +export const enableSuspenseCallback = true; export const enableTaint = true; export const enableTransitionTracing = false; export const enableTrustedTypesIntegration = false;