diff --git a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts index cecbb49e44360..869799073e49c 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/HIR/PrintHIR.ts @@ -943,7 +943,10 @@ export function printAliasingEffect(effect: AliasingEffect): string { return `Assign ${printPlaceForAliasEffect(effect.into)} = ${printPlaceForAliasEffect(effect.from)}`; } case 'Alias': { - return `Alias ${printPlaceForAliasEffect(effect.into)} = ${printPlaceForAliasEffect(effect.from)}`; + return `Alias ${printPlaceForAliasEffect(effect.into)} <- ${printPlaceForAliasEffect(effect.from)}`; + } + case 'MaybeAlias': { + return `MaybeAlias ${printPlaceForAliasEffect(effect.into)} <- ${printPlaceForAliasEffect(effect.from)}`; } case 'Capture': { return `Capture ${printPlaceForAliasEffect(effect.into)} <- ${printPlaceForAliasEffect(effect.from)}`; diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/AliasingEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/AliasingEffects.ts index 95f4e0f5bb1a0..c492e2835460f 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/AliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/AliasingEffects.ts @@ -90,6 +90,23 @@ export type AliasingEffect = * c could be mutating a. */ | {kind: 'Alias'; from: Place; into: Place} + + /** + * Indicates the potential for information flow from `from` to `into`. This is used for a specific + * case: functions with unknown signatures. If the compiler sees a call such as `foo(x)`, it has to + * consider several possibilities (which may depend on the arguments): + * - foo(x) returns a new mutable value that does not capture any information from x. + * - foo(x) returns a new mutable value that *does* capture information from x. + * - foo(x) returns x itself, ie foo is the identity function + * + * The same is true of functions that take multiple arguments: `cond(a, b, c)` could conditionally + * return b or c depending on the value of a. + * + * To represent this case, MaybeAlias represents the fact that an aliasing relationship could exist. + * Any mutations that flow through this relationship automatically become conditional. + */ + | {kind: 'MaybeAlias'; from: Place; into: Place} + /** * Records direct assignment: `into = from`. */ @@ -183,7 +200,8 @@ export function hashEffect(effect: AliasingEffect): string { case 'ImmutableCapture': case 'Assign': case 'Alias': - case 'Capture': { + case 'Capture': + case 'MaybeAlias': { return [ effect.kind, effect.from.identifier.id, diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts index 7052cb2dd8a52..78d82c0c461d5 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/AnalyseFunctions.ts @@ -85,7 +85,8 @@ function lowerWithMutationAliasing(fn: HIRFunction): void { case 'Assign': case 'Alias': case 'Capture': - case 'CreateFrom': { + case 'CreateFrom': + case 'MaybeAlias': { capturedOrMutated.add(effect.from.identifier.id); break; } 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 ca6c6ebae96f6..2adf78fe058e8 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingEffects.ts @@ -691,6 +691,7 @@ function applyEffect( } break; } + case 'MaybeAlias': case 'Alias': case 'Capture': { CompilerError.invariant( @@ -955,7 +956,7 @@ function applyEffect( context, state, // OK: recording information flow - {kind: 'Alias', from: operand, into: effect.into}, + {kind: 'MaybeAlias', from: operand, into: effect.into}, initialized, effects, ); @@ -1323,7 +1324,7 @@ class InferenceState { return 'mutate-global'; } case ValueKind.MaybeFrozen: { - return 'none'; + return 'mutate-frozen'; } default: { assertExhaustive(kind, `Unexpected kind ${kind}`); @@ -2376,6 +2377,7 @@ function computeEffectsForSignature( // Apply substitutions for (const effect of signature.effects) { switch (effect.kind) { + case 'MaybeAlias': case 'Assign': case 'ImmutableCapture': case 'Alias': diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts index cd344342225ce..9d1733c77a9a7 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/InferMutationAliasingRanges.ts @@ -160,6 +160,8 @@ export function inferMutationAliasingRanges( state.assign(index++, effect.from, effect.into); } else if (effect.kind === 'Alias') { state.assign(index++, effect.from, effect.into); + } else if (effect.kind === 'MaybeAlias') { + state.maybeAlias(index++, effect.from, effect.into); } else if (effect.kind === 'Capture') { state.capture(index++, effect.from, effect.into); } else if ( @@ -247,6 +249,7 @@ export function inferMutationAliasingRanges( } for (const param of [...fn.context, ...fn.params]) { const place = param.kind === 'Identifier' ? param : param.place; + const node = state.nodes.get(place.identifier); if (node == null) { continue; @@ -346,7 +349,8 @@ export function inferMutationAliasingRanges( case 'Assign': case 'Alias': case 'Capture': - case 'CreateFrom': { + case 'CreateFrom': + case 'MaybeAlias': { const isMutatedOrReassigned = effect.into.identifier.mutableRange.end > instr.id; if (isMutatedOrReassigned) { @@ -567,7 +571,12 @@ type Node = { createdFrom: Map; captures: Map; aliases: Map; - edges: Array<{index: number; node: Identifier; kind: 'capture' | 'alias'}>; + maybeAliases: Map; + edges: Array<{ + index: number; + node: Identifier; + kind: 'capture' | 'alias' | 'maybeAlias'; + }>; transitive: {kind: MutationKind; loc: SourceLocation} | null; local: {kind: MutationKind; loc: SourceLocation} | null; lastMutated: number; @@ -585,6 +594,7 @@ class AliasingState { createdFrom: new Map(), captures: new Map(), aliases: new Map(), + maybeAliases: new Map(), edges: [], transitive: null, local: null, @@ -630,6 +640,18 @@ class AliasingState { } } + maybeAlias(index: number, from: Place, into: Place): void { + const fromNode = this.nodes.get(from.identifier); + const toNode = this.nodes.get(into.identifier); + if (fromNode == null || toNode == null) { + return; + } + fromNode.edges.push({index, node: into.identifier, kind: 'maybeAlias'}); + if (!toNode.maybeAliases.has(from.identifier)) { + toNode.maybeAliases.set(from.identifier, index); + } + } + render(index: number, start: Identifier, errors: CompilerError): void { const seen = new Set(); const queue: Array = [start]; @@ -673,22 +695,24 @@ class AliasingState { // Null is used for simulated mutations end: InstructionId | null, transitive: boolean, - kind: MutationKind, + startKind: MutationKind, loc: SourceLocation, errors: CompilerError, ): void { - const seen = new Set(); + const seen = new Map(); const queue: Array<{ place: Identifier; transitive: boolean; direction: 'backwards' | 'forwards'; - }> = [{place: start, transitive, direction: 'backwards'}]; + kind: MutationKind; + }> = [{place: start, transitive, direction: 'backwards', kind: startKind}]; while (queue.length !== 0) { - const {place: current, transitive, direction} = queue.pop()!; - if (seen.has(current)) { + const {place: current, transitive, direction, kind} = queue.pop()!; + const previousKind = seen.get(current); + if (previousKind != null && previousKind >= kind) { continue; } - seen.add(current); + seen.set(current, kind); const node = this.nodes.get(current); if (node == null) { continue; @@ -724,13 +748,18 @@ class AliasingState { if (edge.index >= index) { break; } - queue.push({place: edge.node, transitive, direction: 'forwards'}); + queue.push({place: edge.node, transitive, direction: 'forwards', kind}); } for (const [alias, when] of node.createdFrom) { if (when >= index) { continue; } - queue.push({place: alias, transitive: true, direction: 'backwards'}); + queue.push({ + place: alias, + transitive: true, + direction: 'backwards', + kind, + }); } if (direction === 'backwards' || node.value.kind !== 'Phi') { /** @@ -747,7 +776,25 @@ class AliasingState { if (when >= index) { continue; } - queue.push({place: alias, transitive, direction: 'backwards'}); + queue.push({place: alias, transitive, direction: 'backwards', kind}); + } + /** + * MaybeAlias indicates potential data flow from unknown function calls, + * so we downgrade mutations through these aliases to consider them + * conditional. This means we'll consider them for mutation *range* + * purposes but not report validation errors for mutations, since + * we aren't sure that the `from` value could actually be aliased. + */ + for (const [alias, when] of node.maybeAliases) { + if (when >= index) { + continue; + } + queue.push({ + place: alias, + transitive, + direction: 'backwards', + kind: MutationKind.Conditional, + }); } } /** @@ -758,7 +805,12 @@ class AliasingState { if (when >= index) { continue; } - queue.push({place: capture, transitive, direction: 'backwards'}); + queue.push({ + place: capture, + transitive, + direction: 'backwards', + kind, + }); } } } diff --git a/compiler/packages/babel-plugin-react-compiler/src/Inference/MUTABILITY_ALIASING_MODEL.md b/compiler/packages/babel-plugin-react-compiler/src/Inference/MUTABILITY_ALIASING_MODEL.md index 60ef16a6245f3..ab327c255b109 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Inference/MUTABILITY_ALIASING_MODEL.md +++ b/compiler/packages/babel-plugin-react-compiler/src/Inference/MUTABILITY_ALIASING_MODEL.md @@ -153,6 +153,10 @@ This is somewhat the inverse of `Capture`. The `CreateFrom` effect describes tha Describes immutable data flow from one value to another. This is not currently used for anything, but is intended to eventually power a more sophisticated escape analysis. +### MaybeAlias + +Describes potential data flow that the compiler knows may occur behind a function call, but cannot be sure about. For example, `foo(x)` _may_ be the identity function and return `x`, or `cond(a, b, c)` may conditionally return `b` or `c` depending on the value of `a`, but those functions could just as easily return new mutable values and not capture any information from their arguments. MaybeAlias represents that we have to consider the potential for data flow when deciding mutable ranges, but should be conservative about reporting errors. For example, `foo(someFrozenValue).property = true` should not error since we don't know for certain that foo returns its input. + ### State-Changing Effects The following effects describe state changes to specific values, not data flow. In many cases, JavaScript semantics will involve a combination of both data-flow effects *and* state-change effects. For example, `object.property = value` has data flow (`Capture object <- value`) and mutation (`Mutate object`). @@ -347,6 +351,17 @@ a.b = b; // capture mutate(a); // can transitively mutate b ``` +### MaybeAlias makes mutation conditional + +Because we don't know for certain that the aliasing occurs, we consider the mutation conditional against the source. + +``` +MaybeAlias a <- b +Mutate a +=> +MutateConditional b +``` + ### Freeze Does Not Freeze the Value Freeze does not freeze the value itself: diff --git a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts index 6a764d674012f..596244b3834c9 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/TypeInference/InferTypes.ts @@ -14,6 +14,7 @@ import { Identifier, IdentifierId, Instruction, + InstructionKind, makePropertyLiteral, makeType, PropType, @@ -194,12 +195,29 @@ function* generateInstructionTypes( break; } - // We intentionally do not infer types for context variables + // We intentionally do not infer types for most context variables case 'DeclareContext': - case 'StoreContext': case 'LoadContext': { break; } + case 'StoreContext': { + /** + * The caveat is StoreContext const, where we know the value is + * assigned once such that everywhere the value is accessed, it + * must have the same type from the rvalue. + * + * A concrete example where this is useful is `const ref = useRef()` + * where the ref is referenced before its declaration in a function + * expression, causing it to be converted to a const context variable. + */ + if (value.lvalue.kind === InstructionKind.Const) { + yield equation( + value.lvalue.place.identifier.type, + value.value.identifier.type, + ); + } + break; + } case 'StoreLocal': { if (env.config.enableUseTypeAnnotations) { diff --git a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts index 3c810025a7e5c..9c4efa380cd40 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts +++ b/compiler/packages/babel-plugin-react-compiler/src/Validation/ValidateNoSetStateInEffects.ts @@ -97,16 +97,21 @@ export function validateNoSetStateInEffects( errors.pushDiagnostic( CompilerDiagnostic.create({ category: - 'Calling setState within an effect can trigger cascading renders', + 'Calling setState synchronously within an effect can trigger cascading renders', description: - 'Calling setState directly within a useEffect causes cascading renders that can hurt performance, and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect)', + 'Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. ' + + 'In general, the body of an effect should do one or both of the following:\n' + + '* Update external systems with the latest state from React.\n' + + '* Subscribe for updates from some external system, calling setState in a callback function when external state changes.\n\n' + + 'Calling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. ' + + '(https://react.dev/learn/you-might-not-need-an-effect)', severity: ErrorSeverity.InvalidReact, suggestions: null, }).withDetail({ kind: 'error', loc: setState.loc, message: - 'Avoid calling setState() in the top-level of an effect', + 'Avoid calling setState() directly within an effect', }), ); } diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-phi-which-could-be-frozen.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-phi-which-could-be-frozen.expect.md new file mode 100644 index 0000000000000..3ea3d017179fd --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-phi-which-could-be-frozen.expect.md @@ -0,0 +1,39 @@ + +## Input + +```javascript +import {useHook} from 'shared-runtime'; + +function Component(props) { + const frozen = useHook(); + let x; + if (props.cond) { + x = frozen; + } else { + x = {}; + } + x.property = true; +} + +``` + + +## Error + +``` +Found 1 error: + +Error: This value cannot be modified + +Modifying a value returned from a hook is not allowed. Consider moving the modification into the hook where the value is constructed. + +error.invalid-mutate-phi-which-could-be-frozen.ts:11:2 + 9 | x = {}; + 10 | } +> 11 | x.property = true; + | ^ value cannot be modified + 12 | } + 13 | +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-phi-which-could-be-frozen.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-phi-which-could-be-frozen.js new file mode 100644 index 0000000000000..f1f4691006b2b --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.invalid-mutate-phi-which-could-be-frozen.js @@ -0,0 +1,12 @@ +import {useHook} from 'shared-runtime'; + +function Component(props) { + const frozen = useHook(); + let x; + if (props.cond) { + x = frozen; + } else { + x = {}; + } + x.property = true; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-for-loop-with-context-variable-iterator.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-for-loop-with-context-variable-iterator.expect.md new file mode 100644 index 0000000000000..e56f70b29eb4c --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-for-loop-with-context-variable-iterator.expect.md @@ -0,0 +1,61 @@ + +## Input + +```javascript +import {Stringify, useIdentity} from 'shared-runtime'; + +function Component() { + const data = useIdentity( + new Map([ + [0, 'value0'], + [1, 'value1'], + ]) + ); + const items = []; + // NOTE: `i` is a context variable because it's reassigned and also referenced + // within a closure, the `onClick` handler of each item + // TODO: for loops create a unique environment on each iteration, which means + // that if the iteration variable is only updated in the updater, the variable + // is effectively const within the body and the "update" acts more like + // a re-initialization than a reassignment. + // Until we model this "new environment" semantic, we allow this case to error + for (let i = MIN; i <= MAX; i += INCREMENT) { + items.push( + data.get(i)} shouldInvokeFns={true} /> + ); + } + return <>{items}; +} + +const MIN = 0; +const MAX = 3; +const INCREMENT = 1; + +export const FIXTURE_ENTRYPOINT = { + params: [], + fn: Component, +}; + +``` + + +## Error + +``` +Found 1 error: + +Error: This value cannot be modified + +Modifying a value used previously in JSX is not allowed. Consider moving the modification before the JSX. + +error.todo-for-loop-with-context-variable-iterator.ts:18:30 + 16 | // a re-initialization than a reassignment. + 17 | // Until we model this "new environment" semantic, we allow this case to error +> 18 | for (let i = MIN; i <= MAX; i += INCREMENT) { + | ^ `i` cannot be modified + 19 | items.push( + 20 | data.get(i)} shouldInvokeFns={true} /> + 21 | ); +``` + + \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-loop-with-context-variable-iterator.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-for-loop-with-context-variable-iterator.js similarity index 63% rename from compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-loop-with-context-variable-iterator.js rename to compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-for-loop-with-context-variable-iterator.js index f7dbd76a6449e..5942071d6c194 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-loop-with-context-variable-iterator.js +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/error.todo-for-loop-with-context-variable-iterator.js @@ -10,6 +10,11 @@ function Component() { const items = []; // NOTE: `i` is a context variable because it's reassigned and also referenced // within a closure, the `onClick` handler of each item + // TODO: for loops create a unique environment on each iteration, which means + // that if the iteration variable is only updated in the updater, the variable + // is effectively const within the body and the "update" acts more like + // a re-initialization than a reassignment. + // Until we model this "new environment" semantic, we allow this case to error for (let i = MIN; i <= MAX; i += INCREMENT) { items.push( data.get(i)} shouldInvokeFns={true} /> diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-loop-with-context-variable-iterator.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-loop-with-context-variable-iterator.expect.md deleted file mode 100644 index db9ea1d5b628c..0000000000000 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/for-loop-with-context-variable-iterator.expect.md +++ /dev/null @@ -1,89 +0,0 @@ - -## Input - -```javascript -import {Stringify, useIdentity} from 'shared-runtime'; - -function Component() { - const data = useIdentity( - new Map([ - [0, 'value0'], - [1, 'value1'], - ]) - ); - const items = []; - // NOTE: `i` is a context variable because it's reassigned and also referenced - // within a closure, the `onClick` handler of each item - for (let i = MIN; i <= MAX; i += INCREMENT) { - items.push( - data.get(i)} shouldInvokeFns={true} /> - ); - } - return <>{items}; -} - -const MIN = 0; -const MAX = 3; -const INCREMENT = 1; - -export const FIXTURE_ENTRYPOINT = { - params: [], - fn: Component, -}; - -``` - -## Code - -```javascript -import { c as _c } from "react/compiler-runtime"; -import { Stringify, useIdentity } from "shared-runtime"; - -function Component() { - const $ = _c(3); - let t0; - if ($[0] === Symbol.for("react.memo_cache_sentinel")) { - t0 = new Map([ - [0, "value0"], - [1, "value1"], - ]); - $[0] = t0; - } else { - t0 = $[0]; - } - const data = useIdentity(t0); - let t1; - if ($[1] !== data) { - const items = []; - for (let i = MIN; i <= MAX; i = i + INCREMENT, i) { - items.push( - data.get(i)} - shouldInvokeFns={true} - />, - ); - } - - t1 = <>{items}; - $[1] = data; - $[2] = t1; - } else { - t1 = $[2]; - } - return t1; -} - -const MIN = 0; -const MAX = 3; -const INCREMENT = 1; - -export const FIXTURE_ENTRYPOINT = { - params: [], - fn: Component, -}; - -``` - -### Eval output -(kind: ok)
{"onClick":{"kind":"Function","result":"value0"},"shouldInvokeFns":true}
{"onClick":{"kind":"Function","result":"value1"},"shouldInvokeFns":true}
{"onClick":{"kind":"Function"},"shouldInvokeFns":true}
{"onClick":{"kind":"Function"},"shouldInvokeFns":true}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect-transitive.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect-transitive.expect.md index 4e9e1f2c3a799..b629bfef27369 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect-transitive.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect-transitive.expect.md @@ -65,7 +65,7 @@ function _temp(s) { ## Logs ``` -{"kind":"CompileError","detail":{"options":{"category":"Calling setState within an effect can trigger cascading renders","description":"Calling setState directly within a useEffect causes cascading renders that can hurt performance, and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect)","severity":"InvalidReact","suggestions":null,"details":[{"kind":"error","loc":{"start":{"line":13,"column":4,"index":265},"end":{"line":13,"column":5,"index":266},"filename":"invalid-setState-in-useEffect-transitive.ts","identifierName":"g"},"message":"Avoid calling setState() in the top-level of an effect"}]}},"fnLoc":null} +{"kind":"CompileError","detail":{"options":{"category":"Calling setState synchronously within an effect can trigger cascading renders","description":"Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. In general, the body of an effect should do one or both of the following:\n* Update external systems with the latest state from React.\n* Subscribe for updates from some external system, calling setState in a callback function when external state changes.\n\nCalling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. (https://react.dev/learn/you-might-not-need-an-effect)","severity":"InvalidReact","suggestions":null,"details":[{"kind":"error","loc":{"start":{"line":13,"column":4,"index":265},"end":{"line":13,"column":5,"index":266},"filename":"invalid-setState-in-useEffect-transitive.ts","identifierName":"g"},"message":"Avoid calling setState() directly within an effect"}]}},"fnLoc":null} {"kind":"CompileSuccess","fnLoc":{"start":{"line":4,"column":0,"index":92},"end":{"line":16,"column":1,"index":293},"filename":"invalid-setState-in-useEffect-transitive.ts"},"fnName":"Component","memoSlots":2,"memoBlocks":2,"memoValues":2,"prunedMemoBlocks":0,"prunedMemoValues":0} ``` diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect.expect.md index 8d902c2c7d649..c16890e52e5fc 100644 --- a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect.expect.md +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/invalid-setState-in-useEffect.expect.md @@ -45,7 +45,7 @@ function _temp(s) { ## Logs ``` -{"kind":"CompileError","detail":{"options":{"category":"Calling setState within an effect can trigger cascading renders","description":"Calling setState directly within a useEffect causes cascading renders that can hurt performance, and is not recommended. Consider alternatives to useEffect. (https://react.dev/learn/you-might-not-need-an-effect)","severity":"InvalidReact","suggestions":null,"details":[{"kind":"error","loc":{"start":{"line":7,"column":4,"index":180},"end":{"line":7,"column":12,"index":188},"filename":"invalid-setState-in-useEffect.ts","identifierName":"setState"},"message":"Avoid calling setState() in the top-level of an effect"}]}},"fnLoc":null} +{"kind":"CompileError","detail":{"options":{"category":"Calling setState synchronously within an effect can trigger cascading renders","description":"Effects are intended to synchronize state between React and external systems such as manually updating the DOM, state management libraries, or other platform APIs. In general, the body of an effect should do one or both of the following:\n* Update external systems with the latest state from React.\n* Subscribe for updates from some external system, calling setState in a callback function when external state changes.\n\nCalling setState synchronously within an effect body causes cascading renders that can hurt performance, and is not recommended. (https://react.dev/learn/you-might-not-need-an-effect)","severity":"InvalidReact","suggestions":null,"details":[{"kind":"error","loc":{"start":{"line":7,"column":4,"index":180},"end":{"line":7,"column":12,"index":188},"filename":"invalid-setState-in-useEffect.ts","identifierName":"setState"},"message":"Avoid calling setState() directly within an effect"}]}},"fnLoc":null} {"kind":"CompileSuccess","fnLoc":{"start":{"line":4,"column":0,"index":92},"end":{"line":10,"column":1,"index":225},"filename":"invalid-setState-in-useEffect.ts"},"fnName":"Component","memoSlots":1,"memoBlocks":1,"memoValues":1,"prunedMemoBlocks":0,"prunedMemoValues":0} ``` diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-ref-in-function-passed-to-hook.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-ref-in-function-passed-to-hook.expect.md new file mode 100644 index 0000000000000..d8431d5bcacc2 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-ref-in-function-passed-to-hook.expect.md @@ -0,0 +1,114 @@ + +## Input + +```javascript +// @flow +component Example() { + const fooRef = useRef(); + + function updateStyles() { + const foo = fooRef.current; + // The access of `barRef` here before its declaration causes it be hoisted... + if (barRef.current == null || foo == null) { + return; + } + foo.style.height = '100px'; + } + + // ...which previously meant that we didn't infer a type... + const barRef = useRef(null); + + const resizeRef = useResizeObserver( + rect => { + const {width} = rect; + // ...which meant that we failed to ignore the mutation here... + barRef.current = width; + } // ...which caused this to fail with "can't freeze a mutable function" + ); + + useLayoutEffect(() => { + const observer = new ResizeObserver(_ => { + updateStyles(); + }); + + return () => { + observer.disconnect(); + }; + }, []); + + return
; +} + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +function Example() { + const $ = _c(6); + const fooRef = useRef(); + let t0; + if ($[0] === Symbol.for("react.memo_cache_sentinel")) { + t0 = function updateStyles() { + const foo = fooRef.current; + if (barRef.current == null || foo == null) { + return; + } + + foo.style.height = "100px"; + }; + $[0] = t0; + } else { + t0 = $[0]; + } + const updateStyles = t0; + + const barRef = useRef(null); + let t1; + if ($[1] === Symbol.for("react.memo_cache_sentinel")) { + t1 = (rect) => { + const { width } = rect; + + barRef.current = width; + }; + $[1] = t1; + } else { + t1 = $[1]; + } + const resizeRef = useResizeObserver(t1); + let t2; + let t3; + if ($[2] === Symbol.for("react.memo_cache_sentinel")) { + t2 = () => { + const observer = new ResizeObserver((_) => { + updateStyles(); + }); + return () => { + observer.disconnect(); + }; + }; + + t3 = []; + $[2] = t2; + $[3] = t3; + } else { + t2 = $[2]; + t3 = $[3]; + } + useLayoutEffect(t2, t3); + let t4; + if ($[4] !== resizeRef) { + t4 =
; + $[4] = resizeRef; + $[5] = t4; + } else { + t4 = $[5]; + } + return t4; +} + +``` + +### Eval output +(kind: exception) Fixture not implemented \ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-ref-in-function-passed-to-hook.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-ref-in-function-passed-to-hook.js new file mode 100644 index 0000000000000..7aab1a5c40e1d --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-ref-in-function-passed-to-hook.js @@ -0,0 +1,36 @@ +// @flow +component Example() { + const fooRef = useRef(); + + function updateStyles() { + const foo = fooRef.current; + // The access of `barRef` here before its declaration causes it be hoisted... + if (barRef.current == null || foo == null) { + return; + } + foo.style.height = '100px'; + } + + // ...which previously meant that we didn't infer a type... + const barRef = useRef(null); + + const resizeRef = useResizeObserver( + rect => { + const {width} = rect; + // ...which meant that we failed to ignore the mutation here... + barRef.current = width; + } // ...which caused this to fail with "can't freeze a mutable function" + ); + + useLayoutEffect(() => { + const observer = new ResizeObserver(_ => { + updateStyles(); + }); + + return () => { + observer.disconnect(); + }; + }, []); + + return
; +} diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-function-call-with-frozen-argument-in-function-expression.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-function-call-with-frozen-argument-in-function-expression.expect.md new file mode 100644 index 0000000000000..320b252bb5cbf --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-function-call-with-frozen-argument-in-function-expression.expect.md @@ -0,0 +1,77 @@ + +## Input + +```javascript +import {identity, makeObject_Primitives, Stringify} from 'shared-runtime'; + +function Example(props) { + const object = props.object; + const f = () => { + // The argument maybe-aliases into the return + const obj = identity(object); + obj.property = props.value; + return obj; + }; + const obj = f(); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Example, + params: [{object: makeObject_Primitives(), value: 42}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { identity, makeObject_Primitives, Stringify } from "shared-runtime"; + +function Example(props) { + const $ = _c(7); + const object = props.object; + let t0; + if ($[0] !== object || $[1] !== props.value) { + t0 = () => { + const obj = identity(object); + obj.property = props.value; + return obj; + }; + $[0] = object; + $[1] = props.value; + $[2] = t0; + } else { + t0 = $[2]; + } + const f = t0; + let t1; + if ($[3] !== f) { + t1 = f(); + $[3] = f; + $[4] = t1; + } else { + t1 = $[4]; + } + const obj_0 = t1; + let t2; + if ($[5] !== obj_0) { + t2 = ; + $[5] = obj_0; + $[6] = t2; + } else { + t2 = $[6]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Example, + params: [{ object: makeObject_Primitives(), value: 42 }], +}; + +``` + +### Eval output +(kind: ok)
{"obj":{"a":0,"b":"value1","c":true,"property":42}}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-function-call-with-frozen-argument-in-function-expression.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-function-call-with-frozen-argument-in-function-expression.js new file mode 100644 index 0000000000000..88070806119a3 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-function-call-with-frozen-argument-in-function-expression.js @@ -0,0 +1,18 @@ +import {identity, makeObject_Primitives, Stringify} from 'shared-runtime'; + +function Example(props) { + const object = props.object; + const f = () => { + // The argument maybe-aliases into the return + const obj = identity(object); + obj.property = props.value; + return obj; + }; + const obj = f(); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Example, + params: [{object: makeObject_Primitives(), value: 42}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-in-function-expression.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-in-function-expression.expect.md new file mode 100644 index 0000000000000..d3dbb86711a79 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-in-function-expression.expect.md @@ -0,0 +1,77 @@ + +## Input + +```javascript +import {makeObject_Primitives, Stringify} from 'shared-runtime'; + +function Example(props) { + const object = props.object; + const f = () => { + // The receiver maybe-aliases into the return + const obj = object.makeObject(); + obj.property = props.value; + return obj; + }; + const obj = f(); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Example, + params: [{object: {makeObject: makeObject_Primitives}, value: 42}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { makeObject_Primitives, Stringify } from "shared-runtime"; + +function Example(props) { + const $ = _c(7); + const object = props.object; + let t0; + if ($[0] !== object || $[1] !== props.value) { + t0 = () => { + const obj = object.makeObject(); + obj.property = props.value; + return obj; + }; + $[0] = object; + $[1] = props.value; + $[2] = t0; + } else { + t0 = $[2]; + } + const f = t0; + let t1; + if ($[3] !== f) { + t1 = f(); + $[3] = f; + $[4] = t1; + } else { + t1 = $[4]; + } + const obj_0 = t1; + let t2; + if ($[5] !== obj_0) { + t2 = ; + $[5] = obj_0; + $[6] = t2; + } else { + t2 = $[6]; + } + return t2; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Example, + params: [{ object: { makeObject: makeObject_Primitives }, value: 42 }], +}; + +``` + +### Eval output +(kind: ok)
{"obj":{"a":0,"b":"value1","c":true,"property":42}}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-in-function-expression.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-in-function-expression.js new file mode 100644 index 0000000000000..92834df138d30 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-in-function-expression.js @@ -0,0 +1,18 @@ +import {makeObject_Primitives, Stringify} from 'shared-runtime'; + +function Example(props) { + const object = props.object; + const f = () => { + // The receiver maybe-aliases into the return + const obj = object.makeObject(); + obj.property = props.value; + return obj; + }; + const obj = f(); + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Example, + params: [{object: {makeObject: makeObject_Primitives}, value: 42}], +}; diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-is-allowed.expect.md b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-is-allowed.expect.md new file mode 100644 index 0000000000000..0c91a935ed7da --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-is-allowed.expect.md @@ -0,0 +1,57 @@ + +## Input + +```javascript +import {makeObject_Primitives, Stringify} from 'shared-runtime'; + +function Example(props) { + const obj = props.object.makeObject(); + obj.property = props.value; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Example, + params: [{object: {makeObject: makeObject_Primitives}, value: 42}], +}; + +``` + +## Code + +```javascript +import { c as _c } from "react/compiler-runtime"; +import { makeObject_Primitives, Stringify } from "shared-runtime"; + +function Example(props) { + const $ = _c(5); + let obj; + if ($[0] !== props.object || $[1] !== props.value) { + obj = props.object.makeObject(); + obj.property = props.value; + $[0] = props.object; + $[1] = props.value; + $[2] = obj; + } else { + obj = $[2]; + } + let t0; + if ($[3] !== obj) { + t0 = ; + $[3] = obj; + $[4] = t0; + } else { + t0 = $[4]; + } + return t0; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Example, + params: [{ object: { makeObject: makeObject_Primitives }, value: 42 }], +}; + +``` + +### Eval output +(kind: ok)
{"obj":{"a":0,"b":"value1","c":true,"property":42}}
\ No newline at end of file diff --git a/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-is-allowed.js b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-is-allowed.js new file mode 100644 index 0000000000000..d5ed97e159b27 --- /dev/null +++ b/compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/repro-mutate-result-of-method-call-on-frozen-value-is-allowed.js @@ -0,0 +1,12 @@ +import {makeObject_Primitives, Stringify} from 'shared-runtime'; + +function Example(props) { + const obj = props.object.makeObject(); + obj.property = props.value; + return ; +} + +export const FIXTURE_ENTRYPOINT = { + fn: Example, + params: [{object: {makeObject: makeObject_Primitives}, value: 42}], +}; diff --git a/packages/react-devtools-core/src/standalone.js b/packages/react-devtools-core/src/standalone.js index 369dc43a242f9..f8286783a9b44 100644 --- a/packages/react-devtools-core/src/standalone.js +++ b/packages/react-devtools-core/src/standalone.js @@ -26,7 +26,7 @@ import { import {localStorageSetItem} from 'react-devtools-shared/src/storage'; import type {FrontendBridge} from 'react-devtools-shared/src/bridge'; -import type {ReactFunctionLocation} from 'shared/ReactTypes'; +import type {ReactFunctionLocation, ReactCallSite} from 'shared/ReactTypes'; export type StatusTypes = 'server-connected' | 'devtools-connected' | 'error'; export type StatusListener = (message: string, status: StatusTypes) => void; @@ -144,8 +144,8 @@ async function fetchFileWithCaching(url: string) { } function canViewElementSourceFunction( - _source: ReactFunctionLocation, - symbolicatedSource: ReactFunctionLocation | null, + _source: ReactFunctionLocation | ReactCallSite, + symbolicatedSource: ReactFunctionLocation | ReactCallSite | null, ): boolean { if (symbolicatedSource == null) { return false; @@ -156,8 +156,8 @@ function canViewElementSourceFunction( } function viewElementSourceFunction( - _source: ReactFunctionLocation, - symbolicatedSource: ReactFunctionLocation | null, + _source: ReactFunctionLocation | ReactCallSite, + symbolicatedSource: ReactFunctionLocation | ReactCallSite | null, ): void { if (symbolicatedSource == null) { return; diff --git a/packages/react-devtools-extensions/src/main/index.js b/packages/react-devtools-extensions/src/main/index.js index d7756ca991bef..f21bdc7a9a9d8 100644 --- a/packages/react-devtools-extensions/src/main/index.js +++ b/packages/react-devtools-extensions/src/main/index.js @@ -18,7 +18,12 @@ import { LOCAL_STORAGE_TRACE_UPDATES_ENABLED_KEY, } from 'react-devtools-shared/src/constants'; import {logEvent} from 'react-devtools-shared/src/Logger'; -import {normalizeUrlIfValid} from 'react-devtools-shared/src/utils'; +import { + getAlwaysOpenInEditor, + getOpenInEditorURL, + normalizeUrlIfValid, +} from 'react-devtools-shared/src/utils'; +import {checkConditions} from 'react-devtools-shared/src/devtools/views/Editor/utils'; import { setBrowserSelectionFromReact, @@ -326,7 +331,7 @@ function createSourcesEditorPanel() { editorPane = createdPane; createdPane.setPage('panel.html'); - createdPane.setHeight('42px'); + createdPane.setHeight('75px'); createdPane.onShown.addListener(portal => { editorPortalContainer = portal.container; @@ -530,3 +535,45 @@ if (__IS_FIREFOX__) { connectExtensionPort(); mountReactDevToolsWhenReactHasLoaded(); + +function onThemeChanged(themeName) { + // Rerender with the new theme + render(); +} + +if (chrome.devtools.panels.setThemeChangeHandler) { + // Chrome + chrome.devtools.panels.setThemeChangeHandler(onThemeChanged); +} else if (chrome.devtools.panels.onThemeChanged) { + // Firefox + chrome.devtools.panels.onThemeChanged.addListener(onThemeChanged); +} + +// Firefox doesn't support resources handlers yet. +if (chrome.devtools.panels.setOpenResourceHandler) { + chrome.devtools.panels.setOpenResourceHandler( + ( + resource, + lineNumber = 1, + // The column is a new feature so we have to specify a default if it doesn't exist + columnNumber = 1, + ) => { + const alwaysOpenInEditor = getAlwaysOpenInEditor(); + const editorURL = getOpenInEditorURL(); + if (alwaysOpenInEditor && editorURL) { + const location = ['', resource.url, lineNumber, columnNumber]; + const {url, shouldDisableButton} = checkConditions(editorURL, location); + if (!shouldDisableButton) { + window.open(url); + return; + } + } + // Otherwise fallback to the built-in behavior. + chrome.devtools.panels.openResource( + resource.url, + lineNumber - 1, + columnNumber - 1, + ); + }, + ); +} diff --git a/packages/react-devtools-fusebox/src/frontend.d.ts b/packages/react-devtools-fusebox/src/frontend.d.ts index 5a06aec7e49c4..a1142178f47a7 100644 --- a/packages/react-devtools-fusebox/src/frontend.d.ts +++ b/packages/react-devtools-fusebox/src/frontend.d.ts @@ -34,17 +34,26 @@ export type ReactFunctionLocation = [ number, // enclosing line number number, // enclosing column number ]; +export type ReactCallSite = [ + string, // function name + string, // file name TODO: model nested eval locations as nested arrays + number, // line number + number, // column number + number, // enclosing line number + number, // enclosing column number + boolean, // async resume +]; export type ViewElementSource = ( - source: ReactFunctionLocation, - symbolicatedSource: ReactFunctionLocation | null, + source: ReactFunctionLocation | ReactCallSite, + symbolicatedSource: ReactFunctionLocation | ReactCallSite | null, ) => void; export type ViewAttributeSource = ( id: number, path: Array, ) => void; export type CanViewElementSource = ( - source: ReactFunctionLocation, - symbolicatedSource: ReactFunctionLocation | null, + source: ReactFunctionLocation | ReactCallSite, + symbolicatedSource: ReactFunctionLocation | ReactCallSite | null, ) => boolean; export type InitializationOptions = { diff --git a/packages/react-devtools-shared/src/__tests__/__serializers__/inspectedElementSerializer.js b/packages/react-devtools-shared/src/__tests__/__serializers__/inspectedElementSerializer.js index 870ad35e4b03c..55029252843c8 100644 --- a/packages/react-devtools-shared/src/__tests__/__serializers__/inspectedElementSerializer.js +++ b/packages/react-devtools-shared/src/__tests__/__serializers__/inspectedElementSerializer.js @@ -15,8 +15,7 @@ export function test(maybeInspectedElement) { hasOwnProperty('canEditFunctionProps') && hasOwnProperty('canEditHooks') && hasOwnProperty('canToggleSuspense') && - hasOwnProperty('canToggleError') && - hasOwnProperty('canViewSource') + hasOwnProperty('canToggleError') ); } diff --git a/packages/react-devtools-shared/src/backend/fiber/renderer.js b/packages/react-devtools-shared/src/backend/fiber/renderer.js index 937fe8d352fd9..96c7a38863b2e 100644 --- a/packages/react-devtools-shared/src/backend/fiber/renderer.js +++ b/packages/react-devtools-shared/src/backend/fiber/renderer.js @@ -4374,8 +4374,6 @@ export function attach( (fiber.alternate !== null && forceFallbackForFibers.has(fiber.alternate))), - // Can view component source location. - canViewSource, source, // Does the component have legacy context attached to it. @@ -4416,7 +4414,6 @@ export function attach( function inspectVirtualInstanceRaw( virtualInstance: VirtualInstance, ): InspectedElement | null { - const canViewSource = true; const source = getSourceForInstance(virtualInstance); const componentInfo = virtualInstance.data; @@ -4470,8 +4467,6 @@ export function attach( canToggleSuspense: supportsTogglingSuspense && hasSuspenseBoundary, - // Can view component source location. - canViewSource, source, // Does the component have legacy context attached to it. diff --git a/packages/react-devtools-shared/src/backend/legacy/renderer.js b/packages/react-devtools-shared/src/backend/legacy/renderer.js index cbcc37e319ee6..cc097c8379090 100644 --- a/packages/react-devtools-shared/src/backend/legacy/renderer.js +++ b/packages/react-devtools-shared/src/backend/legacy/renderer.js @@ -830,8 +830,6 @@ export function attach( // Suspense did not exist in legacy versions canToggleSuspense: false, - // Can view component source location. - canViewSource: type === ElementTypeClass || type === ElementTypeFunction, source: null, // Only legacy context exists in legacy versions. diff --git a/packages/react-devtools-shared/src/backend/types.js b/packages/react-devtools-shared/src/backend/types.js index c86298850973e..c9d6284b2f424 100644 --- a/packages/react-devtools-shared/src/backend/types.js +++ b/packages/react-devtools-shared/src/backend/types.js @@ -264,9 +264,6 @@ export type InspectedElement = { // Is this Suspense, and can its value be overridden now? canToggleSuspense: boolean, - // Can view component source location. - canViewSource: boolean, - // Does the component have legacy context attached to it. hasLegacyContext: boolean, diff --git a/packages/react-devtools-shared/src/backendAPI.js b/packages/react-devtools-shared/src/backendAPI.js index bbb171bce0da0..20b4e99a101e7 100644 --- a/packages/react-devtools-shared/src/backendAPI.js +++ b/packages/react-devtools-shared/src/backendAPI.js @@ -222,7 +222,6 @@ export function convertInspectedElementBackendToFrontend( canToggleError, isErrored, canToggleSuspense, - canViewSource, hasLegacyContext, id, type, @@ -252,7 +251,6 @@ export function convertInspectedElementBackendToFrontend( canToggleError, isErrored, canToggleSuspense, - canViewSource, hasLegacyContext, id, key, diff --git a/packages/react-devtools-shared/src/constants.js b/packages/react-devtools-shared/src/constants.js index b08738165906c..fa32ead1e934a 100644 --- a/packages/react-devtools-shared/src/constants.js +++ b/packages/react-devtools-shared/src/constants.js @@ -37,6 +37,8 @@ export const LOCAL_STORAGE_OPEN_IN_EDITOR_URL = 'React::DevTools::openInEditorUrl'; export const LOCAL_STORAGE_OPEN_IN_EDITOR_URL_PRESET = 'React::DevTools::openInEditorUrlPreset'; +export const LOCAL_STORAGE_ALWAYS_OPEN_IN_EDITOR = + 'React::DevTools::alwaysOpenInEditor'; export const LOCAL_STORAGE_PARSE_HOOK_NAMES_KEY = 'React::DevTools::parseHookNames'; export const SESSION_STORAGE_RECORD_CHANGE_DESCRIPTIONS_KEY = diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js index 8210e12331b1b..cc37953f4d271 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElement.js @@ -18,13 +18,14 @@ import Toggle from '../Toggle'; import {ElementTypeSuspense} from 'react-devtools-shared/src/frontend/types'; import InspectedElementView from './InspectedElementView'; import {InspectedElementContext} from './InspectedElementContext'; -import {getOpenInEditorURL} from '../../../utils'; -import {LOCAL_STORAGE_OPEN_IN_EDITOR_URL} from '../../../constants'; +import {getAlwaysOpenInEditor} from '../../../utils'; +import {LOCAL_STORAGE_ALWAYS_OPEN_IN_EDITOR} from '../../../constants'; import FetchFileWithCachingContext from './FetchFileWithCachingContext'; import {symbolicateSourceWithCache} from 'react-devtools-shared/src/symbolicateSource'; import OpenInEditorButton from './OpenInEditorButton'; import InspectedElementViewSourceButton from './InspectedElementViewSourceButton'; import Skeleton from './Skeleton'; +import useEditorURL from '../useEditorURL'; import styles from './InspectedElement.css'; @@ -118,18 +119,21 @@ export default function InspectedElementWrapper(_: Props): React.Node { inspectedElement != null && inspectedElement.canToggleSuspense; - const editorURL = useSyncExternalStore( - function subscribe(callback) { - window.addEventListener(LOCAL_STORAGE_OPEN_IN_EDITOR_URL, callback); + const alwaysOpenInEditor = useSyncExternalStore( + useCallback(function subscribe(callback) { + window.addEventListener(LOCAL_STORAGE_ALWAYS_OPEN_IN_EDITOR, callback); return function unsubscribe() { - window.removeEventListener(LOCAL_STORAGE_OPEN_IN_EDITOR_URL, callback); + window.removeEventListener( + LOCAL_STORAGE_ALWAYS_OPEN_IN_EDITOR, + callback, + ); }; - }, - function getState() { - return getOpenInEditorURL(); - }, + }, []), + getAlwaysOpenInEditor, ); + const editorURL = useEditorURL(); + const toggleErrored = useCallback(() => { if (inspectedElement == null) { return; @@ -217,7 +221,8 @@ export default function InspectedElementWrapper(_: Props): React.Node {
- {!!editorURL && + {!alwaysOpenInEditor && + !!editorURL && inspectedElement != null && inspectedElement.source != null && symbolicatedSourcePromise != null && ( @@ -271,8 +276,7 @@ export default function InspectedElementWrapper(_: Props): React.Node { {!hideViewSourceAction && ( )} diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSourcePanel.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSourcePanel.js index 91780cdc13d75..585361cedcf43 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSourcePanel.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementSourcePanel.js @@ -8,7 +8,6 @@ */ import * as React from 'react'; -import {useCallback, useContext} from 'react'; import {copy} from 'clipboard-js'; import {toNormalUrl} from 'jsc-safe-url'; @@ -17,7 +16,7 @@ import ButtonIcon from '../ButtonIcon'; import Skeleton from './Skeleton'; import {withPermissionsCheck} from 'react-devtools-shared/src/frontend/utils/withPermissionsCheck'; -import ViewElementSourceContext from './ViewElementSourceContext'; +import useOpenResource from '../useOpenResource'; import type {ReactFunctionLocation} from 'shared/ReactTypes'; import styles from './InspectedElementSourcePanel.css'; @@ -91,24 +90,11 @@ function CopySourceButton({source, symbolicatedSourcePromise}: Props) { function FormattedSourceString({source, symbolicatedSourcePromise}: Props) { const symbolicatedSource = React.use(symbolicatedSourcePromise); - const {canViewElementSourceFunction, viewElementSourceFunction} = useContext( - ViewElementSourceContext, + const [linkIsEnabled, viewSource] = useOpenResource( + source, + symbolicatedSource, ); - // In some cases (e.g. FB internal usage) the standalone shell might not be able to view the source. - // To detect this case, we defer to an injected helper function (if present). - const linkIsEnabled = - viewElementSourceFunction != null && - source != null && - (canViewElementSourceFunction == null || - canViewElementSourceFunction(source, symbolicatedSource)); - - const viewSource = useCallback(() => { - if (viewElementSourceFunction != null && source != null) { - viewElementSourceFunction(source, symbolicatedSource); - } - }, [source, symbolicatedSource]); - const [, sourceURL, line] = symbolicatedSource == null ? source : symbolicatedSource; diff --git a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementViewSourceButton.js b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementViewSourceButton.js index 6043bb61df92f..23d4cf96c8277 100644 --- a/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementViewSourceButton.js +++ b/packages/react-devtools-shared/src/devtools/views/Components/InspectedElementViewSourceButton.js @@ -11,79 +11,48 @@ import * as React from 'react'; import ButtonIcon from '../ButtonIcon'; import Button from '../Button'; -import ViewElementSourceContext from './ViewElementSourceContext'; import Skeleton from './Skeleton'; import type {ReactFunctionLocation} from 'shared/ReactTypes'; -import type { - CanViewElementSource, - ViewElementSource, -} from 'react-devtools-shared/src/devtools/views/DevTools'; -const {useCallback, useContext} = React; +import useOpenResource from '../useOpenResource'; type Props = { - canViewSource: ?boolean, - source: ?ReactFunctionLocation, + source: null | ReactFunctionLocation, symbolicatedSourcePromise: Promise | null, }; function InspectedElementViewSourceButton({ - canViewSource, source, symbolicatedSourcePromise, }: Props): React.Node { - const {canViewElementSourceFunction, viewElementSourceFunction} = useContext( - ViewElementSourceContext, - ); - return ( }> ); } type ActualSourceButtonProps = { - canViewSource: ?boolean, - source: ?ReactFunctionLocation, + source: null | ReactFunctionLocation, symbolicatedSourcePromise: Promise | null, - canViewElementSourceFunction: CanViewElementSource | null, - viewElementSourceFunction: ViewElementSource | null, }; function ActualSourceButton({ - canViewSource, source, symbolicatedSourcePromise, - canViewElementSourceFunction, - viewElementSourceFunction, }: ActualSourceButtonProps): React.Node { const symbolicatedSource = symbolicatedSourcePromise == null ? null : React.use(symbolicatedSourcePromise); - // In some cases (e.g. FB internal usage) the standalone shell might not be able to view the source. - // To detect this case, we defer to an injected helper function (if present). - const buttonIsEnabled = - !!canViewSource && - viewElementSourceFunction != null && - source != null && - (canViewElementSourceFunction == null || - canViewElementSourceFunction(source, symbolicatedSource)); - - const viewSource = useCallback(() => { - if (viewElementSourceFunction != null && source != null) { - viewElementSourceFunction(source, symbolicatedSource); - } - }, [source, symbolicatedSource]); - + const [buttonIsEnabled, viewSource] = useOpenResource( + source, + symbolicatedSource, + ); return ( +
+ + ); + } + + let editorToolbar; + if (showSettings) { + editorToolbar = ( +
); + } else { + editorToolbar = ( +
+ +
+ +
+ ); } return (
- -
- + {editorToolbar} +
+ {editorURL ? ( + { + if (alwaysOpenInEditor) { + startTransition(() => setShowLinkInfo(true)); + } + }} + /> + ) : ( + 'Configure an external editor to open local files.' + )} +
); } diff --git a/packages/react-devtools-shared/src/devtools/views/Editor/utils.js b/packages/react-devtools-shared/src/devtools/views/Editor/utils.js index 89e697bd60c8d..a107c517cb8cb 100644 --- a/packages/react-devtools-shared/src/devtools/views/Editor/utils.js +++ b/packages/react-devtools-shared/src/devtools/views/Editor/utils.js @@ -7,16 +7,16 @@ * @flow */ -import type {ReactFunctionLocation} from 'shared/ReactTypes'; +import type {ReactFunctionLocation, ReactCallSite} from 'shared/ReactTypes'; export function checkConditions( editorURL: string, - source: ReactFunctionLocation, + source: ReactFunctionLocation | ReactCallSite, ): {url: URL | null, shouldDisableButton: boolean} { try { const url = new URL(editorURL); - const [, sourceURL, line] = source; + const [, sourceURL, line, column] = source; let filePath; // Check if sourceURL is a correct URL, which has a protocol specified @@ -47,12 +47,15 @@ export function checkConditions( } const lineNumberAsString = String(line); + const columnNumberAsString = String(column); url.href = url.href .replace('{path}', filePath) .replace('{line}', lineNumberAsString) + .replace('{column}', columnNumberAsString) .replace('%7Bpath%7D', filePath) - .replace('%7Bline%7D', lineNumberAsString); + .replace('%7Bline%7D', lineNumberAsString) + .replace('%7Bcolumn%7D', columnNumberAsString); return {url, shouldDisableButton: false}; } catch (e) { diff --git a/packages/react-devtools-shared/src/devtools/views/Profiler/SidebarEventInfo.js b/packages/react-devtools-shared/src/devtools/views/Profiler/SidebarEventInfo.js index b7b031a990caf..77f0d13feb72f 100644 --- a/packages/react-devtools-shared/src/devtools/views/Profiler/SidebarEventInfo.js +++ b/packages/react-devtools-shared/src/devtools/views/Profiler/SidebarEventInfo.js @@ -12,7 +12,6 @@ import type {SchedulingEvent} from 'react-devtools-timeline/src/types'; import * as React from 'react'; import Button from '../Button'; import ButtonIcon from '../ButtonIcon'; -import ViewElementSourceContext from '../Components/ViewElementSourceContext'; import {useContext} from 'react'; import {TimelineContext} from 'react-devtools-timeline/src/TimelineContext'; import { @@ -22,6 +21,7 @@ import { import {stackToComponentLocations} from 'react-devtools-shared/src/devtools/utils'; import {copy} from 'clipboard-js'; import {withPermissionsCheck} from 'react-devtools-shared/src/frontend/utils/withPermissionsCheck'; +import useOpenResource from '../useOpenResource'; import styles from './SidebarEventInfo.css'; @@ -32,9 +32,6 @@ type SchedulingEventProps = { }; function SchedulingEventInfo({eventInfo}: SchedulingEventProps) { - const {canViewElementSourceFunction, viewElementSourceFunction} = useContext( - ViewElementSourceContext, - ); const {componentName, timestamp} = eventInfo; const componentStack = eventInfo.componentStack || null; @@ -79,15 +76,10 @@ function SchedulingEventInfo({eventInfo}: SchedulingEventProps) { // TODO: We should support symbolication here as well, but // symbolicating the whole stack can be expensive - const canViewSource = - canViewElementSourceFunction == null || - canViewElementSourceFunction(location, null); - - const viewSource = - !canViewSource || viewElementSourceFunction == null - ? () => null - : () => viewElementSourceFunction(location, null); - + const [canViewSource, viewSource] = useOpenResource( + location, + null, + ); return (