Mirror of upstream PR #35288 #609
Open
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Stacked on facebook#34637 `useEffectEvent` is now in canary so we need to remove this `__EXPERIMENTAL__` gating on the rules and tests
Temporarily disables the compiler rules in eslint-plugin-react-hooks. Will revert this later.
Reset EventTime when clearing timers. We need to track repeat updates separately. Typically we always reset all timers when we've logged an update. The same update shouldn't be logged again. I was trying to be clever and not reset the XEventTime because we also need the timestamp to know if it's a repeat event. However, because of this it looked like we had an event schedule an update even after we had reset them. This always resets the XEventTime to -1.1 and then stashes the old time on EventRepeatTime which is our indication whether the next update was a repeat of the old event. --------- Co-authored-by: Ruslan Lesiutin <[email protected]> Co-authored-by: Ricky <[email protected]>
Co-authored-by: Ruslan Lesiutin <[email protected]>
…ofiling mode (facebook#34667) We need this to be able to log the renders that happened inside. This is the same thing we do here but for the offscreen special cases: https://github.com/facebook/react/blob/main/packages/react-reconciler/src/ReactFiberCommitWork.js#L3452-L3457
Called Before: > `logEvent` is a function created with React Hook "useEffectEvent", and can only be called from the same component. Called After: > `logEvent` is a function created with React Hook "useEffectEvent", and can only be called from Effects and Effect Events in the same component. Referenced Before: > `logEvent` is a function created with React Hook "useEffectEvent", and can only be called from the same component. They cannot be assigned to variables or passed down. Referenced After: > `logEvent` is a function created with React Hook "useEffectEvent", and can only be called from Effects and Effect Events in the same component. It cannot be assigned to a variable or passed down.
Co-authored-by: Jack Pope <[email protected]> Co-authored-by: Rick Hanlon <[email protected]>
Fixes a bug where insertion effects were not cleaned up if a hidden Activity is unmounted.
The canaries have been published depending on 0.27-canary. Bumping scheduler just in case to be sure.
…ebook#34672) Follow up to facebook#34649. This adds the compiler rules back so they can be opted-in 6.1.0, but aren't included in the presets as that would be a breaking change.
This change allows it so that tabs that were open before a compiler error are automatically opened again when the error is resolved. Quality of life change for those especially working with the advanced view of the playground. https://github.com/user-attachments/assets/cd2dc117-e6fc-4f57-a08f-259757c4f5e8
The View Transition docs were unclear about this but apparently the `finished` promise never settles if the animation never started. So if there's an error that rejects the `ready` promise, we'll never run the clean up which can cause it to stall. Fixes facebook#34662. However, ultimately that is caused by Chrome stalling our default `onDefaultTransitionIndicator` but it should be unblocked after 10 seconds, not a minute.
…ine profiler is not supported (facebook#34684)
…ebook#34503) The `@enablePreserveExistingMemoizationGuarantees` mode can still fail to preserve manual memoization due to mismtached dependencies. Specifically, where the user's dependencies are more precise than the compiler infers bc the compiler is being conservative about what might be nullable. In this mode though we're intentionally using information from the manual memoization and can also rely on the deps as a signal for what's non-nullable. The idea of the PR is that we treat manual memo deps just like other inferred-as-non-nullable objects during PropagateScopeDeps. We're careful to not treat the full path as non-nullable, only up to the last property index. So `x.y.z` as a manual dep treats `x` and `x.y` as non-nullable, allowing us to preserve a conditional dependency on `x.y.z`. Optionals within manual dependencies are a bit trickier and aren't handled yet, but hopefully that's less common and something we can improve in a follow-up. Not handling them just means that developers may hit false positives on validating existing memoization if they use optional chains in manual dependencies. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34503). * facebook#34689 * __->__ facebook#34503
…lt (facebook#34689) This enables `@enablePreserveExistingMemoizationGuarantees` by default. As of the previous PR (facebook#34503), this mode now enables the following behaviors: - Treating variables referenced within a `useMemo()` or `useCallback()` as "frozen" (immutable) as of the start of the call. Ie, the compiler will assume that the values you reference are not mutated by the body of the useMemo, not are they mutated later. Directly modifying them (eg `var.property = true`) will be an error. - Similarly, the results of the useMemo/useCallback are treated as frozen (immutable) after the call. These two rules match the behavior for other hooks: this means that developers will see similar behavior to swapping out `useMemo()` for a custom `useMyMemo()` wrapper/alias. Additionally, as of facebook#34503 the compiler uses information from the manual dependencies to know which variables are non-nullable. Even if a useMemo block conditionally accesses a nested property — `if (cond) { log(x.y.z) }` — where the compiler would not usually know that `x` is non-nullable, if the user specifies `x.y.z` as a manual dependency then the compiler knows that `x` and `x.y` are non-nullable and can infer a more precise dependency. Finally, this mode also ensures that we always memoize function calls that return primitives. See facebook#34343 for more details. For now, I've explicitly opted out of this feature in all test fixtures where the behavior changed.
…efault (facebook#34654) Rebased on facebook#34454. Always include the root in the timeline even if it has no unique suspenders, since even if it won't suspend, we have to be able to see that and step to one step before the next boundary to see the first boundary that does suspend in its fallback state. Also, if there's no current selection on initial mount, select the last entry in the timeline. We usually do this with `selectedSuspenseID` but that doesn't happen on initial load. So this does it on initial load if nothing else is selected by then. That way when you reload you get the initial root selected. There's a problem here because we should really use one source of truth and `selectedSuspenseID` doesn't really do anything now. Either it should be its separate source of truth and you can't show components in the side-panel or it should be derived from the other state. If it's derived, once there's a selection, e.g. in the root, then even if new timelines load it will never change but that's probably a good thing.
Stacked on facebook#34654. The root is special since it represents "Initial Paint" (or a "Transition" when an Activity is selected). This gives it a different color in the timeline as well as gives it an outline that's clickable. Hovering the timeline now shows "Initial Paint" or "Suspense". Also made the cursor a pointer to invite you to try to click things and some rounded corners. <img width="1219" height="420" alt="Screenshot 2025-10-02 at 1 26 38 PM" src="https://github.com/user-attachments/assets/12451f93-8917-4f3b-8f01-930129e5fc13" /> <img width="1217" height="419" alt="Screenshot 2025-10-02 at 1 26 54 PM" src="https://github.com/user-attachments/assets/02b5e94c-3fbe-488d-b0f2-225b73578608" /> <img width="1215" height="419" alt="Screenshot 2025-10-02 at 1 27 24 PM" src="https://github.com/user-attachments/assets/c24e8861-e74a-4ccc-8643-ee9d04bef43c" /> <img width="1216" height="419" alt="Screenshot 2025-10-02 at 1 27 10 PM" src="https://github.com/user-attachments/assets/d5cc2b62-fa64-41bf-b485-116b1cd67467" />
…#34630) We're showing too much noise in the side-panel when selecting a Suspense boundary. The interesting thing to see directly is the "Suspended by". The "props" are mostly useless because the `"name"` prop is already in the tree. I'm now also showing it in the title bar of the selected element panel. The "children" and "fallback" props are just the thing that you can see in the tree view anyway. The "state" is this weird section with just one field in it, which we already have duplicated in the top toolbar as well. We can just delete this. I make sure to show the icon and a "suspended..." section while the boundary is still loading but now yet resuspended by force suspending. While still loading: <img width="600" height="193" alt="Screenshot 2025-09-27 at 11 54 37 PM" src="https://github.com/user-attachments/assets/1c3f3a96-46e0-4b11-806f-032569c7d5b5" /> After loading: <img width="602" height="266" alt="Screenshot 2025-09-27 at 11 54 53 PM" src="https://github.com/user-attachments/assets/c43cc4cb-036f-4ced-9b0d-226c6320cd76" /> Resuspended after loading: <img width="602" height="300" alt="Screenshot 2025-09-27 at 11 55 07 PM" src="https://github.com/user-attachments/assets/0be01735-48a7-47dc-b5cf-e72ec71e0148" />
…acebook#34695) Somehow my last commit didn't make it in facebook#34630.
…k#34694) When we flush a Suspense boundary we might not flush the fallback segment, it might only flush a placeholder instead. In this case the segment can flush again but we do not want to flush the boundary itself a second time. We now detach the boundary after flushing it. better solution to: facebook#34668
…e on (facebook#34698) This auto updates to select the last entry in the timeline until we make the first selection. That way when new content loads in, we show the last timeline of what is visible.
…cebook#35148) I've been trying out LLM agents for compiler development, and one thing i found is that the agent naturally wants to run `yarn snap <pattern>` to test a specific fixture, and I want to be able to tell it (directly or in rules/skills) to do this in order to get the debug output from all the compiler passes. Agents can figure out our current testfilter.txt file system but that's just tedious. So here we add support for `yarn snap -p <pattern>`. If you pass in a pattern with an extension, we target that extension specifically. If you pass in a .expect.md file, we look at that specific fixture. And if the pattern doesn't have extensions, we search for `<pattern>{.js,.jsx,.ts,.tsx}`. When patterns are enabled we automatically log as in debug mode (if there is a single match), and disable watch mode. Open to feedback!
When dealing with optimistic state, a common problem is not knowing the
id of the thing we're waiting on. Items in lists need keys (and single
items should often have keys too to reset their state). As a result you
have to generate fake keys. It's a pain to manage those and when the
real item comes in, you often end up rendering that with a different
`key` which resets the state of the component tree. That in turns works
against the grain of React and a lot of negatives fall out of it.
This adds a special `optimisticKey` symbol that can be used in place of
a `string` key.
```js
import {optimisticKey} from 'react';
...
const [optimisticItems, setOptimisticItems] = useOptimistic([]);
const children = savedItems.concat(
optimisticItems.map(item =>
<Item key={optimisticKey} item={item} />
)
);
return <div>{children}</div>;
```
The semantics of this `optimisticKey` is that the assumption is that the
newly saved item will be rendered in the same slot as the previous
optimistic items. State is transferred into whatever real key ends up in
the same slot.
This might lead to some incorrect transferring of state in some cases
where things don't end up lining up - but it's worth it for simplicity
in many cases since dealing with true matching of optimistic state is
often very complex for something that only lasts a blink of an eye.
If a new item matches a `key` elsewhere in the set, then that's favored
over reconciling against the old slot.
One quirk with the current algorithm is if the `savedItems` has items
removed, then the slots won't line up by index anymore and will be
skewed. We might be able to add something where the optimistic set is
always reconciled against the end. However, it's probably better to just
assume that the set will line up perfectly and otherwise it's just best
effort that can lead to weird artifacts.
An `optimisticKey` will match itself for updates to the same slot, but
it will not match any existing slot that is not an `optimisticKey`. So
it's not an `any`, which I originally called it, because it doesn't
match existing real keys against new optimistic keys. Only one
direction.
Co-authored-by: Vordgi <[email protected]>
…sions on no-derived-computation-in-effects (facebook#35173) Summary: The operands of a function expression are the elements passed as context. This means that it doesn't make sense to record mutations for them. The relevant mutations will happen in the function body, so we need to prevent FunctionExpression type instruction from running the logic for effect mutations. This was also causing some values to depend on themselves in some cases triggering an infinite loop. Also added n invariant to prevent this issue Test Plan: Added fixture test --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35173). * facebook#35174 * __->__ facebook#35173
…ns-in-effects (facebook#35174) Summary: I missed this conditional messing things up for undefined useState() calls. We should be tracking them. I also missed a test that expect an error was not throwing. Test Plan: Update broken test --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35174). * __->__ facebook#35174 * facebook#35173
…#35102) Just a quick poc: * Inline useState when the initializer is known to not be a function. The heuristic could be improved but will handle a large number of cases already. * Prune effects * Prune useRef if the ref is unused, by pruning 'ref' props on primitive components. Then DCE does the rest of the work - with a small change to allow `useRef()` calls to be dropped since function calls aren't normally eligible for dropping. * Prune event handlers, by pruning props whose names start w "on" from primitive components. Then DCE removes the functions themselves. Per the fixture, this gets pretty far. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35102). * facebook#35112 * __->__ facebook#35102
This deprecates the `noEmit: boolean` flag and adds `outputMode: 'client' | 'client-no-memo' | 'ssr' | 'lint'` as the replacement. OutputMode defaults to null and takes precedence if specified, otherwise we use 'client' mode for noEmit=false and 'lint' mode for noEmit=true. Key points: * Retrying failed compilation switches from 'client' mode to 'client-no-memo' * Validations are enabled behind Environment.proto.shouldEnableValidations, enabled for all modes except 'client-no-memo'. Similar for dropping manual memoization. * OptimizeSSR is now gated by the outputMode==='ssr', not a feature flag * Creation of reactive scopes, and related codegen logic, is now gated by outputMode==='client'
…ation (facebook#34394) The compiler currently drops manual memoization and rewrites it using its own inference. If the existing manual memo dependencies has missing or extra dependencies, compilation can change behavior by running the computation more often (if deps were missing) or less often (if there were extra deps). We currently address this by relying on the developer to use the ESLint plugin and have `eslint-disable-next-line react-hooks/exhaustive-deps` suppressions in their code. If a suppression exists, we skip compilation. But not everyone is using the linter! Relying on the linter is also imprecise since it forces us to bail out on exhaustive-deps checks that only effect (ahem) effects — and while it isn't good to have incorrect deps on effects, it isn't a problem for compilation. So this PR is a rough sketch of validating manual memoization dependencies in the compiler. Long-term we could use this to also check effect deps and replace the ExhaustiveDeps lint rule, but for now I'm focused specifically on manual memoization use-cases. If this works, we can stop bailing out on ESLint suppressions, since the compiler will implement all the appropriate checks (we already check rules of hooks). --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34394). * facebook#34472 * facebook#34471 * __->__ facebook#34394
Records more information in DropManualMemoization so that we know the full span of the manual dependencies array (if present). This allows ValidateExhaustiveDeps to include a suggestion with the correct deps. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34471). * facebook#34472 * __->__ facebook#34471
…deps (facebook#34472) Just to be consistent, we disallow unnecessary deps even if they're known to be non-reactive.
## Summary The built-in browser profiler supports starting/stopping with Cmd+E. For Symmetry this adds the same hotkey for react devtools profiler. ## How did you test this change? yarn build:\<browser name\> yarn run test:\<browser name\> <img width="483" height="135" alt="Screenshot 2025-11-17 at 14 30 34" src="https://github.com/user-attachments/assets/426939aa-15da-4c21-87a4-e949e6949482" /> firefox: https://github.com/user-attachments/assets/6f225b90-828f-4e79-a364-59d6bc942f83 edge: https://github.com/user-attachments/assets/5b2e9242-f0e8-481b-99a2-2dd78099f3ac chrome: https://github.com/user-attachments/assets/790aab02-2867-4499-aec1-e32e38c763f9 --------- Co-authored-by: Ruslan Lesiutin <[email protected]>
…acebook#35184) With `ValidateExhaustiveMemoDependencies` we can now check exhaustive dependencies for useMemo and useCallback within the compiler, without relying on the separate exhaustive-deps rule. Until now we've bailed out of any component/hook that suppresses this rule, since the suppression _might_ affect a memoization value. Compiling code with incorrect memo deps can change behavior so this wasn't safe. The downside was that a suppression within a useEffect could prevent memoization, even though non-exhaustive deps for effects do not cause problems for memoization specifically. So here, we change to ignore ESLint suppressions if we have both the compiler's hooks validation and memo deps validations enabled. Now we just have to test out the new validation and refine before we can enable this by default. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35184). * facebook#35201 * facebook#35202 * facebook#35192 * facebook#35190 * facebook#35186 * facebook#35185 * __->__ facebook#35184
…k#35185) When checking ValidateExhaustiveDeps internally, this seems to be the most common case that it flags. The current exhaustive-deps rule allows extraneous deps if they are a set of stable types. So here we reuse our existing isStableType() util in the compiler to allow this case. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35185). * facebook#35201 * facebook#35202 * facebook#35192 * facebook#35190 * facebook#35186 * __->__ facebook#35185
…an inferred deps (facebook#35186) Since adding this validation we've already changed our inference to use knowledge from manual memoization to inform when values are frozen and which values are non-nullable. To align with that, if the user chooses to use different optionality btw the deps and the memo block/callback, that's fine. The key is that eg `x?.y` will invalidate whenever `x.y` would, so from a memoization correctness perspective its fine. It's not our job to be a type checker: if a value is potentially nullable, it should likely use a nullable property access in both places but TypeScript/Flow can check that. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35186). * facebook#35201 * facebook#35202 * facebook#35192 * facebook#35190 * __->__ facebook#35186
The existing exhaustive-deps rule allows omitting non-reactive dependencies, even if they're not memoized. Conceptually, if a value is non-reactive then it cannot semantically change. Even if the value is a new object, that object represents the exact same value and doesn't necessitate redoing downstream computation. Thus its fine to exclude nonreactive dependencies, whether they're a stable type or not. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35190). * facebook#35201 * facebook#35202 * facebook#35192 * __->__ facebook#35190
…rule (facebook#35192) Similar to ValidateHookUsage, we implement this check in the compiler for safety but (for now) continue to rely on the existing rule for actually reporting errors to users. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35192). * facebook#35201 * facebook#35202 * __->__ facebook#35192
…cebook#35202) --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35202). * facebook#35201 * __->__ facebook#35202
In ValidateExhaustiveDependencies, I previously changed to allow extraneous dependencies as long as they were non-reactive. Here we make that more precise, and distinguish between values that are definitely referenced in the memo function but optional as dependencies vs values that are not even referenced in the memo function. The latter now error as extraneous even if they're non-reactive. This also turned up a case where constant-folded primitives could show up as false positives of the latter category, so now we track manual deps which quality for constant folding and don't error on them. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35204). * facebook#35213 * facebook#35201 * __->__ facebook#35204
facebook#35201) Enables `@validateExhaustiveMemoizationDependencies` feature flag by default, and disables it in select tests that failed due to the change. Some of our tests intentionally use incorrect memo dependencies in order to test edge cases. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/35201). * facebook#35213 * __->__ facebook#35201
…#35213) First, this adds some more tests and organizes them into an `exhaustive-deps/` subdirectory. Second, the diagnostics are overhauled. For each memo block we now report a single diagnostic which summarizes the issue, plus individual errors for each missing/extra dependency. Within the extra deps, we distinguish whether it's truly extra vs whether its just a more (too) precise version of an inferred dep. For example, if you depend on `x.y.z` but the inferred dep was `x.y`. Finally, we print the full inferred deps at the end as a hint (it's also a suggestion, but this makes it more clear what would be suggested).
…ix (facebook#35215) Fixes some issues i ran into w my recent snap changes: * Correctly match against patterns that contain subdirectories, eg `fbt/fbt-call` * When checking if the input pattern has an extension, only prune known supported extensions. Our convention of `error.<name>` for fixtures that error makes the rest of the test name look like an extension to `path.extname()`. Tested with lots of different patterns including `error.` examples at the top level and in nested directories, etc.
…cebook#35214) ValidateNoSetStateInEffects already supports transitive setter functions. This PR marks any synchonous state setter useEffectEvent function so we can validate that uEE isn't being used only as misdirection to avoid the validation within an effect body. The error points to the call of the effect event. Example: ```js export default function MyApp() { const [count, setCount] = useState(0) const effectEvent = useEffectEvent(() => { setCount(10) }) useEffect(() => { effectEvent() }, []) return <div>{count}</div>; ``` ``` Found 1 error: Error: Calling setState synchronously within an effect can trigger cascading renders 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: * Update external systems with the latest state from React. * Subscribe for updates from some external system, calling setState in a callback function when external state changes. 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). 5 | }) 6 | useEffect(() => { > 7 | effectEvent() | ^^^^^^^^^^^ Avoid calling setState() directly within an effect 8 | }, []) 9 | return <div>{count}</div>; 10 | } ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
20 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Mirrored from facebook/react PR facebook#35288