diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts index d2f2ba61d695a..94be2100f57e8 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -19,6 +19,7 @@ import { Environment, FunctionExpression, GeneratedSource, + getHookKind, HIRFunction, Hole, IdentifierId, @@ -198,6 +199,7 @@ export function inferMutationAliasingEffects( isFunctionExpression, fn, hoistedContextDeclarations, + findNonMutatedDestructureSpreads(fn), ); let iterationCount = 0; @@ -287,15 +289,18 @@ class Context { isFuctionExpression: boolean; fn: HIRFunction; hoistedContextDeclarations: Map; + nonMutatingSpreads: Set; constructor( isFunctionExpression: boolean, fn: HIRFunction, hoistedContextDeclarations: Map, + nonMutatingSpreads: Set, ) { this.isFuctionExpression = isFunctionExpression; this.fn = fn; this.hoistedContextDeclarations = hoistedContextDeclarations; + this.nonMutatingSpreads = nonMutatingSpreads; } cacheApplySignature( @@ -322,6 +327,161 @@ class Context { } } +/** + * Finds objects created via ObjectPattern spread destructuring + * (`const {x, ...spread} = ...`) where a) the rvalue is known frozen and + * b) the spread value cannot possibly be directly mutated. The idea is that + * for this set of values, we can treat the spread object as frozen. + * + * The primary use case for this is props spreading: + * + * ``` + * function Component({prop, ...otherProps}) { + * const transformedProp = transform(prop, otherProps.foo); + * // pass `otherProps` down: + * return ; + * } + * ``` + * + * Here we know that since `otherProps` cannot be mutated, we don't have to treat + * it as mutable: `otherProps.foo` only reads a value that must be frozen, so it + * can be treated as frozen too. + */ +function findNonMutatedDestructureSpreads(fn: HIRFunction): Set { + const knownFrozen = new Set(); + if (fn.fnType === 'Component') { + const [props] = fn.params; + if (props != null && props.kind === 'Identifier') { + knownFrozen.add(props.identifier.id); + } + } else { + for (const param of fn.params) { + if (param.kind === 'Identifier') { + knownFrozen.add(param.identifier.id); + } + } + } + + // Map of temporaries to identifiers for spread objects + const candidateNonMutatingSpreads = new Map(); + for (const block of fn.body.blocks.values()) { + if (candidateNonMutatingSpreads.size !== 0) { + for (const phi of block.phis) { + for (const operand of phi.operands.values()) { + const spread = candidateNonMutatingSpreads.get(operand.identifier.id); + if (spread != null) { + candidateNonMutatingSpreads.delete(spread); + } + } + } + } + for (const instr of block.instructions) { + const {lvalue, value} = instr; + switch (value.kind) { + case 'Destructure': { + if ( + !knownFrozen.has(value.value.identifier.id) || + !( + value.lvalue.kind === InstructionKind.Let || + value.lvalue.kind === InstructionKind.Const + ) || + value.lvalue.pattern.kind !== 'ObjectPattern' + ) { + continue; + } + for (const item of value.lvalue.pattern.properties) { + if (item.kind !== 'Spread') { + continue; + } + candidateNonMutatingSpreads.set( + item.place.identifier.id, + item.place.identifier.id, + ); + } + break; + } + case 'LoadLocal': { + const spread = candidateNonMutatingSpreads.get( + value.place.identifier.id, + ); + if (spread != null) { + candidateNonMutatingSpreads.set(lvalue.identifier.id, spread); + } + break; + } + case 'StoreLocal': { + const spread = candidateNonMutatingSpreads.get( + value.value.identifier.id, + ); + if (spread != null) { + candidateNonMutatingSpreads.set(lvalue.identifier.id, spread); + candidateNonMutatingSpreads.set( + value.lvalue.place.identifier.id, + spread, + ); + } + break; + } + case 'JsxFragment': + case 'JsxExpression': { + // Passing objects created with spread to jsx can't mutate them + break; + } + case 'PropertyLoad': { + // Properties must be frozen since the original value was frozen + break; + } + case 'CallExpression': + case 'MethodCall': { + const callee = + value.kind === 'CallExpression' ? value.callee : value.property; + if (getHookKind(fn.env, callee.identifier) != null) { + // Hook calls have frozen arguments, and non-ref returns are frozen + if (!isRefOrRefValue(lvalue.identifier)) { + knownFrozen.add(lvalue.identifier.id); + } + } else { + // Non-hook calls check their operands, since they are potentially mutable + if (candidateNonMutatingSpreads.size !== 0) { + // Otherwise any reference to the spread object itself may mutate + for (const operand of eachInstructionValueOperand(value)) { + const spread = candidateNonMutatingSpreads.get( + operand.identifier.id, + ); + if (spread != null) { + candidateNonMutatingSpreads.delete(spread); + } + } + } + } + break; + } + default: { + if (candidateNonMutatingSpreads.size !== 0) { + // Otherwise any reference to the spread object itself may mutate + for (const operand of eachInstructionValueOperand(value)) { + const spread = candidateNonMutatingSpreads.get( + operand.identifier.id, + ); + if (spread != null) { + candidateNonMutatingSpreads.delete(spread); + } + } + } + } + } + } + } + + const nonMutatingSpreads = new Set(); + for (const [key, value] of candidateNonMutatingSpreads) { + if (key === value) { + nonMutatingSpreads.add(key); + } + } + return nonMutatingSpreads; +} + function inferParam( param: Place | SpreadPattern, initialState: InferenceState, @@ -2054,7 +2214,9 @@ function computeSignatureForInstruction( kind: 'Create', into: place, reason: ValueReason.Other, - value: ValueKind.Mutable, + value: context.nonMutatingSpreads.has(place.identifier.id) + ? ValueKind.Frozen + : ValueKind.Mutable, }); effects.push({ kind: 'Capture', diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-hook-return.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-hook-return.expect.md new file mode 100644 index 0000000000000..9a4f0c179f9f3 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-hook-return.expect.md @@ -0,0 +1,63 @@ + +## Input + +```javascript +import {identity, Stringify, useIdentity} from 'shared-runtime'; + +function Component(props) { + const {x, ...rest} = useIdentity(props); + const z = rest.z; + identity(z); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{x: 'Hello', z: 'World'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { identity, Stringify, useIdentity } from "shared-runtime"; + +function Component(props) { + const $ = _c(6); + const t0 = useIdentity(props); + let rest; + let x; + if ($[0] !== t0) { + ({ x, ...rest } = t0); + $[0] = t0; + $[1] = rest; + $[2] = x; + } else { + rest = $[1]; + x = $[2]; + } + const z = rest.z; + identity(z); + let t1; + if ($[3] !== x || $[4] !== z) { + t1 = ; + $[3] = x; + $[4] = z; + $[5] = t1; + } else { + t1 = $[5]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ x: "Hello", z: "World" }], +}; + +``` + +### Eval output +(kind: ok)
{"x":"Hello","z":"World"}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-hook-return.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-hook-return.js new file mode 100644 index 0000000000000..c4447f7be6e44 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-hook-return.js @@ -0,0 +1,13 @@ +import {identity, Stringify, useIdentity} from 'shared-runtime'; + +function Component(props) { + const {x, ...rest} = useIdentity(props); + const z = rest.z; + identity(z); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{x: 'Hello', z: 'World'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-props-jsx.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-props-jsx.expect.md new file mode 100644 index 0000000000000..5335705c5d827 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-props-jsx.expect.md @@ -0,0 +1,57 @@ + +## Input + +```javascript +import {identity, Stringify} from 'shared-runtime'; + +function Component({x, ...rest}) { + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{x: 'Hello', z: 'World'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { identity, Stringify } from "shared-runtime"; + +function Component(t0) { + const $ = _c(6); + let rest; + let x; + if ($[0] !== t0) { + ({ x, ...rest } = t0); + $[0] = t0; + $[1] = rest; + $[2] = x; + } else { + rest = $[1]; + x = $[2]; + } + let t1; + if ($[3] !== rest || $[4] !== x) { + t1 = ; + $[3] = rest; + $[4] = x; + $[5] = t1; + } else { + t1 = $[5]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ x: "Hello", z: "World" }], +}; + +``` + +### Eval output +(kind: ok)
{"z":"World","x":"Hello"}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-props-jsx.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-props-jsx.js new file mode 100644 index 0000000000000..d9f24264d69bc --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-props-jsx.js @@ -0,0 +1,10 @@ +import {identity, Stringify} from 'shared-runtime'; + +function Component({x, ...rest}) { + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{x: 'Hello', z: 'World'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-props-local-indirection.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-props-local-indirection.expect.md new file mode 100644 index 0000000000000..7a435adcaad76 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-props-local-indirection.expect.md @@ -0,0 +1,63 @@ + +## Input + +```javascript +import {identity, Stringify} from 'shared-runtime'; + +function Component({x, ...rest}) { + const restAlias = rest; + const z = restAlias.z; + identity(z); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{x: 'Hello', z: 'World'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { identity, Stringify } from "shared-runtime"; + +function Component(t0) { + const $ = _c(6); + let rest; + let x; + if ($[0] !== t0) { + ({ x, ...rest } = t0); + $[0] = t0; + $[1] = rest; + $[2] = x; + } else { + rest = $[1]; + x = $[2]; + } + const restAlias = rest; + const z = restAlias.z; + identity(z); + let t1; + if ($[3] !== x || $[4] !== z) { + t1 = ; + $[3] = x; + $[4] = z; + $[5] = t1; + } else { + t1 = $[5]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ x: "Hello", z: "World" }], +}; + +``` + +### Eval output +(kind: ok)
{"x":"Hello","z":"World"}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-props-local-indirection.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-props-local-indirection.js new file mode 100644 index 0000000000000..b1d26ab7f7206 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-props-local-indirection.js @@ -0,0 +1,13 @@ +import {identity, Stringify} from 'shared-runtime'; + +function Component({x, ...rest}) { + const restAlias = rest; + const z = restAlias.z; + identity(z); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{x: 'Hello', z: 'World'}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-props.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-props.expect.md new file mode 100644 index 0000000000000..a8b1c2d747f0d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-props.expect.md @@ -0,0 +1,61 @@ + +## Input + +```javascript +import {identity, Stringify} from 'shared-runtime'; + +function Component({x, ...rest}) { + const z = rest.z; + identity(z); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{x: 'Hello', z: 'World'}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { identity, Stringify } from "shared-runtime"; + +function Component(t0) { + const $ = _c(6); + let rest; + let x; + if ($[0] !== t0) { + ({ x, ...rest } = t0); + $[0] = t0; + $[1] = rest; + $[2] = x; + } else { + rest = $[1]; + x = $[2]; + } + const z = rest.z; + identity(z); + let t1; + if ($[3] !== x || $[4] !== z) { + t1 = ; + $[3] = x; + $[4] = z; + $[5] = t1; + } else { + t1 = $[5]; + } + return t1; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{ x: "Hello", z: "World" }], +}; + +``` + +### Eval output +(kind: ok)
{"x":"Hello","z":"World"}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-props.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-props.js new file mode 100644 index 0000000000000..d3e83d560c253 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/nonmutated-spread-props.js @@ -0,0 +1,12 @@ +import {identity, Stringify} from 'shared-runtime'; + +function Component({x, ...rest}) { + const z = rest.z; + identity(z); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Component, + params: [{x: 'Hello', z: 'World'}], +}; diff --git a/packages/react-devtools-shared/src/devtools/constants.js b/packages/react-devtools-shared/src/devtools/constants.js index cfb73713b089e..d093a798bcd30 100644 --- a/packages/react-devtools-shared/src/devtools/constants.js +++ b/packages/react-devtools-shared/src/devtools/constants.js @@ -136,8 +136,6 @@ export const THEME_STYLES: {[style: Theme | DisplayDensity]: any, ...} = { '--color-timeline-text-dim-color': '#ccc', '--color-timeline-react-work-border': '#eeeeee', '--color-timebar-background': '#f6f6f6', - '--color-timespan-background': '#62bc6a', - '--color-timespan-background-errored': '#d57066', '--color-search-match': 'yellow', '--color-search-match-current': '#f7923b', '--color-selected-tree-highlight-active': 'rgba(0, 136, 250, 0.1)', @@ -156,6 +154,14 @@ export const THEME_STYLES: {[style: Theme | DisplayDensity]: any, ...} = { '--color-warning-text-color': '#ffffff', '--color-warning-text-color-inverted': '#fd4d69', + '--color-suspense-default': '#0088fa', + '--color-transition-default': '#6a51b2', + '--color-suspense-server': '#62bc6a', + '--color-transition-server': '#3f7844', + '--color-suspense-other': '#f3ce49', + '--color-transition-other': '#917b2c', + '--color-suspense-errored': '#d57066', + // The styles below should be kept in sync with 'root.css' // They are repeated there because they're used by e.g. tooltips or context menus // which get rendered outside of the DOM subtree (where normal theme/styles are written). @@ -290,8 +296,6 @@ export const THEME_STYLES: {[style: Theme | DisplayDensity]: any, ...} = { '--color-timeline-text-dim-color': '#555b66', '--color-timeline-react-work-border': '#3d424a', '--color-timebar-background': '#1d2129', - '--color-timespan-background': '#62bc6a', - '--color-timespan-background-errored': '#d57066', '--color-search-match': 'yellow', '--color-search-match-current': '#f7923b', '--color-selected-tree-highlight-active': 'rgba(23, 143, 185, 0.15)', @@ -311,6 +315,14 @@ export const THEME_STYLES: {[style: Theme | DisplayDensity]: any, ...} = { '--color-warning-text-color': '#ffffff', '--color-warning-text-color-inverted': '#ee1638', + '--color-suspense-default': '#61dafb', + '--color-transition-default': '#6a51b2', + '--color-suspense-server': '#62bc6a', + '--color-transition-server': '#3f7844', + '--color-suspense-other': '#f3ce49', + '--color-transition-other': '#917b2c', + '--color-suspense-errored': '#d57066', + // The styles below should be kept in sync with 'root.css' // They are repeated there because they're used by e.g. tooltips or context menus // which get rendered outside of the DOM subtree (where normal theme/styles are written). diff --git a/packages/react-devtools-shared/src/devtools/store.js b/packages/react-devtools-shared/src/devtools/store.js index f1aa61bfe9b86..b75e30d9c47ec 100644 --- a/packages/react-devtools-shared/src/devtools/store.js +++ b/packages/react-devtools-shared/src/devtools/store.js @@ -34,6 +34,7 @@ import { shallowDiffers, utfDecodeStringWithRanges, parseElementDisplayNameFromBackend, + unionOfTwoArrays, } from '../utils'; import {localStorageGetItem, localStorageSetItem} from '../storage'; import {__DEBUG__} from '../constants'; @@ -51,6 +52,7 @@ import type { ComponentFilter, ElementType, SuspenseNode, + SuspenseTimelineStep, Rect, } from 'react-devtools-shared/src/frontend/types'; import type { @@ -895,13 +897,10 @@ export default class Store extends EventEmitter<{ */ getSuspendableDocumentOrderSuspense( uniqueSuspendersOnly: boolean, - ): $ReadOnlyArray { + ): $ReadOnlyArray { + const target: Array = []; const roots = this.roots; - if (roots.length === 0) { - return []; - } - - const list: SuspenseNode['id'][] = []; + let rootStep: null | SuspenseTimelineStep = null; for (let i = 0; i < roots.length; i++) { const rootID = roots[i]; const root = this.getElementByID(rootID); @@ -912,44 +911,76 @@ export default class Store extends EventEmitter<{ const suspense = this.getSuspenseByID(rootID); if (suspense !== null) { - if (list.length === 0) { - // start with an arbitrary root that will allow inspection of the Screen - list.push(suspense.id); - } - - const stack = [suspense]; - while (stack.length > 0) { - const current = stack.pop(); - if (current === undefined) { - continue; - } - // Ignore any suspense boundaries that has no visual representation as this is not - // part of the visible loading sequence. - // TODO: Consider making visible meta data and other side-effects get virtual rects. - const hasRects = - current.rects !== null && - current.rects.length > 0 && - current.rects.some(isNonZeroRect); - if ( - hasRects && - (!uniqueSuspendersOnly || current.hasUniqueSuspenders) && - // Roots are already included as part of the Screen - current.id !== rootID - ) { - list.push(current.id); - } - // Add children in reverse order to maintain document order - for (let j = current.children.length - 1; j >= 0; j--) { - const childSuspense = this.getSuspenseByID(current.children[j]); - if (childSuspense !== null) { - stack.push(childSuspense); - } - } + const environments = suspense.environments; + const environmentName = + environments.length > 0 + ? environments[environments.length - 1] + : null; + if (rootStep === null) { + // Arbitrarily use the first root as the root step id. + rootStep = { + id: suspense.id, + environment: environmentName, + }; + target.push(rootStep); + } else if (rootStep.environment === null) { + // If any root has an environment name, then let's use it. + rootStep.environment = environmentName; } + this.pushTimelineStepsInDocumentOrder( + suspense.children, + target, + uniqueSuspendersOnly, + environments, + ); } } - return list; + return target; + } + + pushTimelineStepsInDocumentOrder( + children: Array, + target: Array, + uniqueSuspendersOnly: boolean, + parentEnvironments: Array, + ): void { + for (let i = 0; i < children.length; i++) { + const child = this.getSuspenseByID(children[i]); + if (child === null) { + continue; + } + // Ignore any suspense boundaries that has no visual representation as this is not + // part of the visible loading sequence. + // TODO: Consider making visible meta data and other side-effects get virtual rects. + const hasRects = + child.rects !== null && + child.rects.length > 0 && + child.rects.some(isNonZeroRect); + const childEnvironments = child.environments; + // Since children are blocked on the parent, they're also blocked by the parent environments. + // Only if we discover a novel environment do we add that and it becomes the name we use. + const unionEnvironments = unionOfTwoArrays( + parentEnvironments, + childEnvironments, + ); + const environmentName = + unionEnvironments.length > 0 + ? unionEnvironments[unionEnvironments.length - 1] + : null; + if (hasRects && (!uniqueSuspendersOnly || child.hasUniqueSuspenders)) { + target.push({ + id: child.id, + environment: environmentName, + }); + } + this.pushTimelineStepsInDocumentOrder( + child.children, + target, + uniqueSuspendersOnly, + unionEnvironments, + ); + } } getRendererIDForElement(id: number): number | null { @@ -1627,6 +1658,7 @@ export default class Store extends EventEmitter<{ rects, hasUniqueSuspenders: false, isSuspended: isSuspended, + environments: [], }); hasSuspenseTreeChanged = true; @@ -1812,7 +1844,10 @@ export default class Store extends EventEmitter<{ envIndex++ ) { const environmentNameStringID = operations[i++]; - environmentNames.push(stringTable[environmentNameStringID]); + const environmentName = stringTable[environmentNameStringID]; + if (environmentName != null) { + environmentNames.push(environmentName); + } } const suspense = this._idToSuspense.get(id); @@ -1836,7 +1871,7 @@ export default class Store extends EventEmitter<{ suspense.hasUniqueSuspenders = hasUniqueSuspenders; suspense.isSuspended = isSuspended; - // TODO: Recompute the environment names. + suspense.environments = environmentNames; } hasSuspenseTreeChanged = true; diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSharedStyles.css b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSharedStyles.css index 413554008fa53..6c56aec689986 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSharedStyles.css +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSharedStyles.css @@ -128,13 +128,13 @@ .TimeBarSpan, .TimeBarSpanErrored { position: absolute; border-radius: 0.125rem; - background-color: var(--color-timespan-background); + background-color: var(--color-suspense); width: 100%; height: 100%; } .TimeBarSpanErrored { - background-color: var(--color-timespan-background-errored); + background-color: var(--color-suspense-errored); } .SmallHeader { diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSuspendedBy.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSuspendedBy.js index fa2b6a95bed53..78c137deaf37c 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSuspendedBy.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSuspendedBy.js @@ -22,6 +22,8 @@ import OwnerView from './OwnerView'; import {meta} from '../../../hydration'; import useInferredName from '../useInferredName'; +import {getClassNameForEnvironment} from '../SuspenseTab/SuspenseEnvironmentColors.js'; + import type { InspectedElement, SerializedAsyncInfo, @@ -169,7 +171,7 @@ function SuspendedByRow({ type={isOpen ? 'expanded' : 'collapsed'} /> - {skipName ? shortDescription : name} + {skipName && shortDescription !== '' ? shortDescription : name} {skipName || shortDescription === '' ? null : ( <> @@ -181,7 +183,12 @@ function SuspendedByRow({ )}
-
+
{pluralizedName}
{isOpen ? null : ( -
+
.SuspenseRectsRect, .SuspenseRectsBoundary[data-highlighted='true'] > .SuspenseRectsRect { - background-color: var(--color-background-hover); +.SuspenseRectsBoundary[data-hovered='true'] > .SuspenseRectsRect { + background-color: color-mix(in srgb, var(--color-background) 50%, var(--color-suspense) 50%); + transition: background-color 0.2s ease-out; +} + +.SuspenseRectsBoundary[data-selected='true'] { + box-shadow: var(--elevation-4); +} + +.SuspenseRectOutline { + outline-color: var(--color-suspense); + outline-style: solid; + outline-width: 4px; + border-radius: 0.125rem; + pointer-events: none; } -.SuspenseRectsRect[data-highlighted='true'] { - background-color: var(--color-selected-tree-highlight-active); +.SuspenseRectsBoundary[data-selected='true'] > .SuspenseRectsRect { + box-shadow: none; } diff --git a/packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseRects.js b/packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseRects.js index c2a131916504c..e6beca7c22973 100644 --- a/packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseRects.js +++ b/packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseRects.js @@ -30,12 +30,15 @@ import { SuspenseTreeStateContext, SuspenseTreeDispatcherContext, } from './SuspenseTreeContext'; +import {getClassNameForEnvironment} from './SuspenseEnvironmentColors.js'; function ScaledRect({ className, rect, visible, suspended, + selected, + hovered, adjust, ...props }: { @@ -43,6 +46,8 @@ function ScaledRect({ rect: Rect, visible: boolean, suspended: boolean, + selected?: boolean, + hovered?: boolean, adjust?: boolean, ... }): React$Node { @@ -58,6 +63,8 @@ function ScaledRect({ className={styles.SuspenseRectsScaledRect + ' ' + className} data-visible={visible} data-suspended={suspended} + data-selected={selected} + data-hovered={hovered} style={{ // Shrink one pixel so that the bottom outline will line up with the top outline of the next one. width: adjust ? 'calc(' + width + ' - 1px)' : width, @@ -77,7 +84,9 @@ function SuspenseRects({ const store = useContext(StoreContext); const treeDispatch = useContext(TreeDispatcherContext); const suspenseTreeDispatch = useContext(SuspenseTreeDispatcherContext); - const {uniqueSuspendersOnly} = useContext(SuspenseTreeStateContext); + const {uniqueSuspendersOnly, timeline, hoveredTimelineIndex} = useContext( + SuspenseTreeStateContext, + ); const {inspectedElementID} = useContext(TreeStateContext); @@ -145,14 +154,33 @@ function SuspenseRects({ // TODO: Use the nearest Suspense boundary const selected = inspectedElementID === suspenseID; + const hovered = + hoveredTimelineIndex > -1 && + timeline[hoveredTimelineIndex].id === suspenseID; + + let environment: null | string = null; + for (let i = 0; i < timeline.length; i++) { + const timelineStep = timeline[i]; + if (timelineStep.id === suspenseID) { + environment = timelineStep.environment; + break; + } + } + const boundingBox = getBoundingBox(suspense.rects); return ( + selected={selected} + suspended={suspense.isSuspended} + hovered={hovered}> {visible && suspense.rects !== null && @@ -162,7 +190,6 @@ function SuspenseRects({ key={index} className={styles.SuspenseRectsRect} rect={rect} - data-highlighted={selected} adjust={true} onClick={handleClick} onDoubleClick={handleDoubleClick} @@ -182,6 +209,13 @@ function SuspenseRects({ })} )} + {selected ? ( + + ) : null} ); @@ -307,7 +341,8 @@ function SuspenseRectsContainer(): React$Node { const treeDispatch = useContext(TreeDispatcherContext); const suspenseTreeDispatch = useContext(SuspenseTreeDispatcherContext); // TODO: This relies on a full re-render of all children when the Suspense tree changes. - const {roots} = useContext(SuspenseTreeStateContext); + const {roots, timeline, hoveredTimelineIndex, uniqueSuspendersOnly} = + useContext(SuspenseTreeStateContext); // TODO: bbox does not consider uniqueSuspendersOnly filter const boundingBox = getDocumentBoundingRect(store, roots); @@ -351,13 +386,37 @@ function SuspenseRectsContainer(): React$Node { } const isRootSelected = roots.includes(inspectedElementID); + const isRootHovered = hoveredTimelineIndex === 0; + + let hasRootSuspenders = false; + if (!uniqueSuspendersOnly) { + hasRootSuspenders = true; + } else { + for (let i = 0; i < roots.length; i++) { + const rootID = roots[i]; + const root = store.getSuspenseByID(rootID); + if (root !== null && root.hasUniqueSuspenders) { + hasRootSuspenders = true; + break; + } + } + } + + const rootEnvironment = + timeline.length === 0 ? null : timeline[0].environment; return (
+ data-highlighted={isRootSelected} + data-hovered={isRootHovered}>
.SuspenseScrubberBead, -.SuspenseScrubberStep:hover > .SuspenseScrubberBead { +.SuspenseScrubberStepHighlight > .SuspenseScrubberBead { height: 0.75rem; + transition: all 0.3s ease-out; } diff --git a/packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseScrubber.js b/packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseScrubber.js index f1998aff4b16c..2c7e1bc1cddea 100644 --- a/packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseScrubber.js +++ b/packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseScrubber.js @@ -7,6 +7,8 @@ * @flow */ +import type {SuspenseTimelineStep} from 'react-devtools-shared/src/frontend/types'; + import typeof {SyntheticEvent} from 'react-dom-bindings/src/events/SyntheticEvent'; import * as React from 'react'; @@ -14,11 +16,14 @@ import {useRef} from 'react'; import styles from './SuspenseScrubber.css'; +import {getClassNameForEnvironment} from './SuspenseEnvironmentColors.js'; + import Tooltip from '../Components/reach-ui/tooltip'; export default function SuspenseScrubber({ min, max, + timeline, value, highlight, onBlur, @@ -29,6 +34,7 @@ export default function SuspenseScrubber({ }: { min: number, max: number, + timeline: $ReadOnlyArray, value: number, highlight: number, onBlur?: () => void, @@ -54,17 +60,18 @@ export default function SuspenseScrubber({ } const steps = []; for (let index = min; index <= max; index++) { + const environment = timeline[index].environment; + const label = + index === min + ? // The first step in the timeline is always a Transition (Initial Paint). + 'Initial Paint' + + (environment === null ? '' : ' (' + environment + ')') + : // TODO: Consider adding the name of this specific boundary if this step has only one. + environment === null + ? 'Suspense' + : environment; steps.push( - +
diff --git a/packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseTimeline.js b/packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseTimeline.js index 4712397632c11..89f349ae6ea7d 100644 --- a/packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseTimeline.js +++ b/packages/react-devtools-shared/src/devtools/views/SuspenseTab/SuspenseTimeline.js @@ -34,7 +34,7 @@ function SuspenseTimelineInput() { const max = timeline.length > 0 ? timeline.length - 1 : 0; function switchSuspenseNode(nextTimelineIndex: number) { - const nextSelectedSuspenseID = timeline[nextTimelineIndex]; + const nextSelectedSuspenseID = timeline[nextTimelineIndex].id; treeDispatch({ type: 'SELECT_ELEMENT_BY_ID', payload: nextSelectedSuspenseID, @@ -53,13 +53,22 @@ function SuspenseTimelineInput() { switchSuspenseNode(timelineIndex); } - function handleHoverSegment(hoveredValue: number) { - // TODO: Consider highlighting the rect instead. + function handleHoverSegment(hoveredIndex: number) { + const nextSelectedSuspenseID = timeline[hoveredIndex].id; + suspenseTreeDispatch({ + type: 'HOVER_TIMELINE_FOR_ID', + payload: nextSelectedSuspenseID, + }); + } + function handleUnhoverSegment() { + suspenseTreeDispatch({ + type: 'HOVER_TIMELINE_FOR_ID', + payload: -1, + }); } - function handleUnhoverSegment() {} function skipPrevious() { - const nextSelectedSuspenseID = timeline[timelineIndex - 1]; + const nextSelectedSuspenseID = timeline[timelineIndex - 1].id; treeDispatch({ type: 'SELECT_ELEMENT_BY_ID', payload: nextSelectedSuspenseID, @@ -71,7 +80,7 @@ function SuspenseTimelineInput() { } function skipForward() { - const nextSelectedSuspenseID = timeline[timelineIndex + 1]; + const nextSelectedSuspenseID = timeline[timelineIndex + 1].id; treeDispatch({ type: 'SELECT_ELEMENT_BY_ID', payload: nextSelectedSuspenseID, @@ -97,7 +106,7 @@ function SuspenseTimelineInput() { // anything suspended in the root. The step after that should have one less // thing suspended. I.e. the first suspense boundary should be unsuspended // when it's selected. This also lets you show everything in the last step. - const suspendedSet = timeline.slice(timelineIndex + 1); + const suspendedSet = timeline.slice(timelineIndex + 1).map(step => step.id); bridge.send('overrideSuspenseMilestone', { suspendedSet, }); @@ -164,6 +173,7 @@ function SuspenseTimelineInput() { | null, roots: $ReadOnlyArray, selectedSuspenseID: SuspenseNode['id'] | null, - timeline: $ReadOnlyArray, + timeline: $ReadOnlyArray, timelineIndex: number | -1, hoveredTimelineIndex: number | -1, uniqueSuspendersOnly: boolean, @@ -49,7 +52,7 @@ type ACTION_SELECT_SUSPENSE_BY_ID = { type ACTION_SET_SUSPENSE_TIMELINE = { type: 'SET_SUSPENSE_TIMELINE', payload: [ - $ReadOnlyArray, + $ReadOnlyArray, // The next Suspense ID to select in the timeline SuspenseNode['id'] | null, // Whether this timeline includes only unique suspenders @@ -111,7 +114,7 @@ function getInitialState(store: Store): SuspenseTreeState { store.getSuspendableDocumentOrderSuspense(uniqueSuspendersOnly); const timelineIndex = timeline.length - 1; const selectedSuspenseID = - timelineIndex === -1 ? null : timeline[timelineIndex]; + timelineIndex === -1 ? null : timeline[timelineIndex].id; const lineage = selectedSuspenseID !== null ? store.getSuspenseLineage(selectedSuspenseID) @@ -164,16 +167,18 @@ function SuspenseTreeContextController({children}: Props): React.Node { selectedSuspenseID = null; } - let selectedTimelineID = - state.timeline === null + const selectedTimelineStep = + state.timeline === null || state.timelineIndex === -1 ? null : state.timeline[state.timelineIndex]; - while ( - selectedTimelineID !== null && - removedIDs.has(selectedTimelineID) - ) { - // $FlowExpectedError[incompatible-type] - selectedTimelineID = removedIDs.get(selectedTimelineID); + let selectedTimelineID: null | number = null; + if (selectedTimelineStep !== null) { + selectedTimelineID = selectedTimelineStep.id; + // $FlowFixMe + while (removedIDs.has(selectedTimelineID)) { + // $FlowFixMe + selectedTimelineID = removedIDs.get(selectedTimelineID); + } } // TODO: Handle different timeline modes (e.g. random order) @@ -181,20 +186,25 @@ function SuspenseTreeContextController({children}: Props): React.Node { state.uniqueSuspendersOnly, ); - let nextTimelineIndex = - selectedTimelineID === null || nextTimeline.length === 0 - ? -1 - : nextTimeline.indexOf(selectedTimelineID); + let nextTimelineIndex = -1; + if (selectedTimelineID !== null && nextTimeline.length !== 0) { + for (let i = 0; i < nextTimeline.length; i++) { + if (nextTimeline[i].id === selectedTimelineID) { + nextTimelineIndex = i; + break; + } + } + } if ( nextTimeline.length > 0 && (nextTimelineIndex === -1 || state.autoSelect) ) { nextTimelineIndex = nextTimeline.length - 1; - selectedSuspenseID = nextTimeline[nextTimelineIndex]; + selectedSuspenseID = nextTimeline[nextTimelineIndex].id; } if (selectedSuspenseID === null && nextTimeline.length > 0) { - selectedSuspenseID = nextTimeline[nextTimeline.length - 1]; + selectedSuspenseID = nextTimeline[nextTimeline.length - 1].id; } const nextLineage = @@ -256,12 +266,12 @@ function SuspenseTreeContextController({children}: Props): React.Node { nextMilestoneIndex = nextTimeline.indexOf(previousMilestoneID); if (nextMilestoneIndex === -1 && nextTimeline.length > 0) { nextMilestoneIndex = nextTimeline.length - 1; - nextSelectedSuspenseID = nextTimeline[nextMilestoneIndex]; + nextSelectedSuspenseID = nextTimeline[nextMilestoneIndex].id; nextLineage = store.getSuspenseLineage(nextSelectedSuspenseID); } } else if (nextRootID !== null) { nextMilestoneIndex = nextTimeline.length - 1; - nextSelectedSuspenseID = nextTimeline[nextMilestoneIndex]; + nextSelectedSuspenseID = nextTimeline[nextMilestoneIndex].id; nextLineage = store.getSuspenseLineage(nextSelectedSuspenseID); } @@ -276,7 +286,7 @@ function SuspenseTreeContextController({children}: Props): React.Node { } case 'SUSPENSE_SET_TIMELINE_INDEX': { const nextTimelineIndex = action.payload; - const nextSelectedSuspenseID = state.timeline[nextTimelineIndex]; + const nextSelectedSuspenseID = state.timeline[nextTimelineIndex].id; const nextLineage = store.getSuspenseLineage( nextSelectedSuspenseID, ); @@ -301,7 +311,7 @@ function SuspenseTreeContextController({children}: Props): React.Node { ) { return state; } - const nextSelectedSuspenseID = state.timeline[nextTimelineIndex]; + const nextSelectedSuspenseID = state.timeline[nextTimelineIndex].id; const nextLineage = store.getSuspenseLineage( nextSelectedSuspenseID, ); @@ -329,7 +339,7 @@ function SuspenseTreeContextController({children}: Props): React.Node { ) { // If we're restarting at the end. Then loop around and start again from the beginning. nextTimelineIndex = 0; - nextSelectedSuspenseID = state.timeline[nextTimelineIndex]; + nextSelectedSuspenseID = state.timeline[nextTimelineIndex].id; nextLineage = store.getSuspenseLineage(nextSelectedSuspenseID); } @@ -352,7 +362,7 @@ function SuspenseTreeContextController({children}: Props): React.Node { if (nextTimelineIndex > state.timeline.length - 1) { return state; } - const nextSelectedSuspenseID = state.timeline[nextTimelineIndex]; + const nextSelectedSuspenseID = state.timeline[nextTimelineIndex].id; const nextLineage = store.getSuspenseLineage( nextSelectedSuspenseID, ); @@ -369,8 +379,14 @@ function SuspenseTreeContextController({children}: Props): React.Node { } case 'TOGGLE_TIMELINE_FOR_ID': { const suspenseID = action.payload; - const timelineIndexForSuspenseID = - state.timeline.indexOf(suspenseID); + + let timelineIndexForSuspenseID = -1; + for (let i = 0; i < state.timeline.length; i++) { + if (state.timeline[i].id === suspenseID) { + timelineIndexForSuspenseID = i; + break; + } + } if (timelineIndexForSuspenseID === -1) { // This boundary is no longer in the timeline. return state; @@ -387,7 +403,7 @@ function SuspenseTreeContextController({children}: Props): React.Node { timelineIndexForSuspenseID : // Otherwise, if we're currently showing it, jump to right before to hide it. timelineIndexForSuspenseID - 1; - const nextSelectedSuspenseID = state.timeline[nextTimelineIndex]; + const nextSelectedSuspenseID = state.timeline[nextTimelineIndex].id; const nextLineage = store.getSuspenseLineage( nextSelectedSuspenseID, ); @@ -403,8 +419,13 @@ function SuspenseTreeContextController({children}: Props): React.Node { } case 'HOVER_TIMELINE_FOR_ID': { const suspenseID = action.payload; - const timelineIndexForSuspenseID = - state.timeline.indexOf(suspenseID); + let timelineIndexForSuspenseID = -1; + for (let i = 0; i < state.timeline.length; i++) { + if (state.timeline[i].id === suspenseID) { + timelineIndexForSuspenseID = i; + break; + } + } return { ...state, hoveredTimelineIndex: timelineIndexForSuspenseID, diff --git a/packages/react-devtools-shared/src/frontend/types.js b/packages/react-devtools-shared/src/frontend/types.js index 2a012ce33a17d..4eed49e6bac8f 100644 --- a/packages/react-devtools-shared/src/frontend/types.js +++ b/packages/react-devtools-shared/src/frontend/types.js @@ -193,6 +193,11 @@ export type Rect = { height: number, }; +export type SuspenseTimelineStep = { + id: SuspenseNode['id'], // TODO: Will become a group. + environment: null | string, +}; + export type SuspenseNode = { id: Element['id'], parentID: SuspenseNode['id'] | 0, @@ -201,6 +206,7 @@ export type SuspenseNode = { rects: null | Array, hasUniqueSuspenders: boolean, isSuspended: boolean, + environments: Array, }; // Serialized version of ReactIOInfo diff --git a/packages/react-devtools-shared/src/utils.js b/packages/react-devtools-shared/src/utils.js index 29ff6d566bd6f..6d31888cd9d0c 100644 --- a/packages/react-devtools-shared/src/utils.js +++ b/packages/react-devtools-shared/src/utils.js @@ -1305,3 +1305,18 @@ export function onReloadAndProfileFlagsReset(): void { sessionStorageRemoveItem(SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY); sessionStorageRemoveItem(SESSION_STORAGE_RECORD_TIMELINE_KEY); } + +export function unionOfTwoArrays(a: Array, b: Array): Array { + let result = a; + for (let i = 0; i < b.length; i++) { + const value = b[i]; + if (a.indexOf(value) === -1) { + if (result === a) { + // Lazily copy + result = a.slice(0); + } + result.push(value); + } + } + return result; +} diff --git a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMNode-test.js b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMNode-test.js index f63f82333116e..d59f298f99496 100644 --- a/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMNode-test.js +++ b/packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMNode-test.js @@ -10,11 +10,11 @@ 'use strict'; +import fs from 'fs'; +import os from 'os'; +import path from 'path'; import {patchSetImmediate} from '../../../../scripts/jest/patchSetImmediate'; -global.ReadableStream = - require('web-streams-polyfill/ponyfill/es6').ReadableStream; - let clientExports; let webpackMap; let webpackModules; @@ -1136,4 +1136,37 @@ describe('ReactFlightDOMNode', () => { 'Switched to client rendering because the server rendering errored:\n\nssr-throw', ); }); + + // This is a regression test for a specific issue where byte Web Streams are + // detaching ArrayBuffers, which caused downstream issues (e.g. "Cannot + // perform Construct on a detached ArrayBuffer") for chunks that are using + // Node's internal Buffer pool. + it('should not corrupt the Node.js Buffer pool by detaching ArrayBuffers when using Web Streams', async () => { + // Create a temp file smaller than 4KB to ensure it uses the Buffer pool. + const file = path.join(os.tmpdir(), 'test.bin'); + fs.writeFileSync(file, Buffer.alloc(4095)); + const fileChunk = fs.readFileSync(file); + fs.unlinkSync(file); + + // Verify this chunk uses the Buffer pool (8192 bytes for files < 4KB). + expect(fileChunk.buffer.byteLength).toBe(8192); + + const readable = await serverAct(() => + ReactServerDOMServer.renderToReadableStream(fileChunk, webpackMap), + ); + + // Create a Web Streams WritableStream that tries to use Buffer operations. + const writable = new WritableStream({ + write(chunk) { + // Only write one byte to ensure Node.js is not creating a new Buffer + // pool. Typically, library code (e.g. a compression middleware) would + // call Buffer.from(chunk) or similar, instead of allocating a new + // Buffer directly. With that, the test file could only be ~2600 bytes. + Buffer.allocUnsafe(1); + }, + }); + + // Must not throw an error. + await readable.pipeTo(writable); + }); }); diff --git a/packages/react-server/src/ReactServerStreamConfigEdge.js b/packages/react-server/src/ReactServerStreamConfigEdge.js index dbe6d6f90cab1..90affdc6b8aca 100644 --- a/packages/react-server/src/ReactServerStreamConfigEdge.js +++ b/packages/react-server/src/ReactServerStreamConfigEdge.js @@ -37,7 +37,11 @@ export function flushBuffered(destination: Destination) { // transform streams. https://github.com/whatwg/streams/issues/960 } -const VIEW_SIZE = 2048; +// Chunks larger than VIEW_SIZE are written directly, without copying into the +// internal view buffer. This must be at least half of Node's internal Buffer +// pool size (8192) to avoid corrupting the pool when using +// renderToReadableStream, which uses a byte stream that detaches ArrayBuffers. +const VIEW_SIZE = 4096; let currentView = null; let writtenBytes = 0; @@ -147,14 +151,7 @@ export function typedArrayToBinaryChunk( // If we passed through this straight to enqueue we wouldn't have to convert it but since // we need to copy the buffer in that case, we need to convert it to copy it. // When we copy it into another array using set() it needs to be a Uint8Array. - const buffer = new Uint8Array( - content.buffer, - content.byteOffset, - content.byteLength, - ); - // We clone large chunks so that we can transfer them when we write them. - // Others get copied into the target buffer. - return content.byteLength > VIEW_SIZE ? buffer.slice() : buffer; + return new Uint8Array(content.buffer, content.byteOffset, content.byteLength); } export function byteLengthOfChunk(chunk: Chunk | PrecomputedChunk): number { diff --git a/packages/react-server/src/ReactServerStreamConfigNode.js b/packages/react-server/src/ReactServerStreamConfigNode.js index 3fb698411721e..90609da2c45d6 100644 --- a/packages/react-server/src/ReactServerStreamConfigNode.js +++ b/packages/react-server/src/ReactServerStreamConfigNode.js @@ -38,7 +38,11 @@ export function flushBuffered(destination: Destination) { } } -const VIEW_SIZE = 2048; +// Chunks larger than VIEW_SIZE are written directly, without copying into the +// internal view buffer. This must be at least half of Node's internal Buffer +// pool size (8192) to avoid corrupting the pool when using +// renderToReadableStream, which uses a byte stream that detaches ArrayBuffers. +const VIEW_SIZE = 4096; let currentView = null; let writtenBytes = 0; let destinationHasCapacity = true;