Flights tests are failing locally and in CI non-deterministically
because we're not disabling async hooks after tests, and GC can clear
WeakRefs non-deterministically.
This PR fixes the issue by adding an afterEach to disable installed
hooks, and normalizing the `value` to `value: {value: undefined}}` when
snapshotting.
I was experimenting with animations in SuspenseList and hit a crash
using ViewTransition as a direct child with `revealOrder="together"`
```
TypeError: Cannot read properties of null (reading 'autoName')
33 | return props.name;
34 | }
> 35 | if (instance.autoName !== null) {
| ^
36 | return instance.autoName;
37 | }
```
When ViewTransition is direct child of SuspenseList, the second render
pass calls resetChildFibers, setting stateNode to null. Other fibers
create stateNode in completeWork. ViewTransition does not, so stateNode
is lost.
Followed the pattern used for Offscreen to update stateNode in beginWork
if it is null.
Also added a regression test.
When `renderModelDestructive` unwraps a lazy element and subsequently
calls `renderModelDestructive` again with the resolved model, we should
preserve the parent connection so that cyclic references can be
serialized properly. This can occur in an advanced scenario where the
result from the Flight Client is serialized again with the Flight
Server, e.g. for slicing a precomputed payload into multiple parts.
Note: The added test only fails when run with `--prod`. In dev mode, the
component info outlining prevents the issue from occurring.
When a lazy element or component is initialized a thenable is returned
which was only be conditionally instrumented in dev when asyncDebugInfo
was enabled. When instrumented these thenables can be used in
conjunction with the SuspendOnImmediate optimization where if a thenable
resolves before the stack unwinds we can continue rendering from the
last suspended fiber. Without this change a recent fix to the useId
implementation cannot be easily tested in production because this
optimization pathway isn't available to regular React.lazy thenables. To
land the prior PR I changed the thenables to a custom type so I could
instrument manually in the test. WIth this change we can just use a
regular Promise since ReactLazy will instrument in all
environments/flags now
Stacked on #35487.
This is slightly different because the first suspended commit is on
blockers that prevent us from committing which still needs to be
resolved first.
If a gesture lane has to be rerendered while the gesture is happening
then it reenters this state with a new tree. (Currently this doesn't
happen for a ping I think which is not really how it usually works but
better in this case.)
If an initial value is specified, then it's always used regardless as
part of the gesture render.
If a gesture render causes an update, then previously that was not
treated as deferred and could therefore be blocking the render. However,
a gesture is supposed to flush synchronously ideally. Therefore we
should consider these as urgent.
The effect is that useDeferredValue renders the previous state.
Stacked on #35486.
When a Gesture commits, it leaves behind work on a Transition lane
(`revertLane`). This entangles that lane with whatever lane we're using
in the event that cancels the Gesture. This ensures that the revert and
the result of any resulting Action commits as one batch. Typically the
Action would apply a new state that is similar or the same as the revert
of the Gesture.
This makes it resilient to unbatching in #35392.
Stacked on #35485.
Before this PR, the `startGestureTransition` API would itself never
commit its state. After the gesture releases it stops the animation in
the next commit which just leaves the DOM tree in the original state. If
there's an actual state change from the Action then that's committed as
the new DOM tree. To avoid animating from the original state to the new
state again, this is DOM without an animation. However, this means that
you can't have the actual action committing be in a slightly different
state and animate between the final gesture state and into the new
action.
Instead, we now actually keep the render tree around and commit it in
the end. Basically we assume that if the Timeline was closer to the end
then visually you're already there and we can commit into that state.
Most of the time this will be at the actual end state when you release
but if you have something else cancelling the gesture (e.g.
`touchcancel`) it can still commit this state even though your gesture
recognizer might not consider this an Action. I think this is ok and
keeps it simple.
When the gesture lane commits, it'll leave a Transition behind as work
from the revert lanes on the Optimistic updates. This means that if you
don't do anything in the Action this will cause another commit right
after which reverts. This revert can animate the snap back.
There's a few fixes needed in follow up PRs:
- Fixed in #35487. ~To support unentangled Transitions we need to
explicitly entangle the revert lane with the Action to avoid committing
a revert followed by a forward instead of committing the forward
entangled with the revert. This just works now since everything is
entangled but won't work with #35392.~
- Fixed in #35510. ~This currently rerenders the gesture lane once
before committing if it was already completed but blocked. We should be
able to commit the already completed tree as is.~
When Fiber replays work after suspending and resolving in a microtask it
stripped the Forked flag from Fibers because this flag type was not
considered a Static flag. The Forked nature of a Fiber is not render
dependent and should persist after unwinding work. By making this change
the replay correctly generates the necessary tree context.
When a View Transition might not need to update we add it to a queue. If
the parent are able to be reverted, we then cancel the already started
view transitions. We do this by adding an animation that hides the "old"
state and remove the view transition name from the old state.
There was a bug where if you have more than one child in a
`<ViewTransition>` we didn't add the right suffix to the name we added
in the queue so it wasn't adding an animation that hides the old state.
The effect was that it playing an exit animation instead of being
cancelled.
Requires full error message in assert helpers.
Some of the error messages we asset on add a native javascript stack
trace, which would be a pain to add to the messages and maintain. This
PR allows you to just add `\n in <stack>` placeholder to the error
message to denote a native stack trace is present in the message.
---
Note: i vibe coded this so it was a pain to backtrack this to break this
into a stack, I tried and gave up, sorry.
When hydrating if something suspends and then resolves in a microtask it
is possible that React will resume the render without fully unwinding
work in progress. This can cause hydration cursors to be offset and lead
to hydration errors. This change adds a restore step when replaying
HostComponent to ensure the hydration cursor is in the appropriate
position when replaying.
fixes: #35210
When the Fizz runtime runs a view-transition we apply
`view-transition-name` and `view-transition-class` to the `style`. These
can be observed by Fiber when hydrating which incorrectly leads to
hydration errors.
More over, even after we remove them, the `style` attribute has now been
normalized which we are unable to diff because we diff against the SSR
generated `style` attribute string and not the normalized form. So if
there are other inline styles defined, we have to skip diffing them in
this scenario.
## Summary
This PR improves cyclic thenable detection in
`ReactFlightReplyServer.js`. Fixes#35368.
The previous fix only detected direct self-references (`inspectedValue
=== chunk`) and relied on the `cycleProtection` counter to eventually
bail out of longer cycles. This change keeps the existing
MAX_THENABLE_CYCLE_DEPTH ($1000$) `cycleProtection` cap as a hard
guardrail and adds a visited set so that we can detect self-cycles and
multi-node cycles as soon as any `ReactPromise` is revisited and while
still bounding the amount of work we do for deep acyclic chains via
`cycleProtection`.
## How did you test this change?
- Ran the existing test suite for the server renderer:
```bash
yarn test react-server
yarn test --prod react-server
yarn flow dom-node
yarn linc
```
---------
Co-authored-by: Hendrik Liebau <mail@hendrik-liebau.de>
`react-hooks/exhaustive-effect-dependencies` from
`ValidateExhaustiveDeps` reports errors for both missing and extra
effect deps. We already have `react-hooks/exhaustive-deps` that errors
on missing dependencies. In the future we'd like to consolidate this all
to the compiler based error, but for now there's a lot of overlap. Let's
enable testing the extra dep warning by splitting out reporting modes.
This PR
- Creates `on`, `off`, `missing-only`, and `extra-only` reporting modes
for the effect dep validation flag
- Temporarily enables the new rule with `extra-only` in
`eslint-plugin-react-hooks`
- Adds additional null checking to `manualMemoLoc` to fix a bug found
when running against the fixture
Server Functions can be stringified (sometimes implicitly) when passed
as data. This adds an override to hide the source code in that case -
just in case someone puts sensitive information in there.
Note that this still preserves the `name` field but this is also
available on the export but in practice is likely minified anyway.
There's nothing else on these referenes we'd consider unsafe unless you
explicitly expose expandos which are part of the `"use server"` export.
This adds a safety check to ensure you don't encode cyclic Promises.
This isn't a parser bug per se. Promises do have a safety mechanism that
avoids them infinite looping. However, since we use custom Thenables,
what can happen is that every time a native Promise awaits it, another
Promise wrapper is created around the Thenable which foils the
ECMAScript Promise cycle detection which can lead to an infinite loop.
This also ensures that embedded `ReadableStream` and `AsyncIterable`
streams are properly closed if the source stream closes early both on
the Server and Client. This doesn't cause an infinite loop but just to
make sure resource clean up can proceed properly.
We're also adding some more explicit clear errors for invalid payloads
since we no longer need to obfuscate the original issue.
`Error.prepareStackTrace` is non-standard feature and not all JavaScript
runtimes implement the methods that we are using in React DevTools
backend.
This PR adds additional checks for the presence of the methods that we
are using.
Follow-up to https://github.com/facebook/react/pull/34653.
React Native doesn't implement `getClientRect`, since this is applicable
to CSS box, which is not a concept for Native (maybe yet).
I am loosening the condition that gates `showOverlay()` call to pass if
`getClientRect` is not implemented.
Conceptually, everything that is inside `react-devtools-shared/backend`
should be Host-agnostic, because both on Web and Native it is installed
inside the Host JavaScript runtime, be it main frame of the page, or RN
instance. Since overlay & highlighting logic also lives there, it should
also follow these principles.
Continue attaching `internalInstanceKey` to DOM nodes in DEV. This
prevents breaking some internal dev tooling while we experiment with the
broader change. Note that this does not reference the DOM handle within
the flag, just attaches it and deletes it. Internals tracking is still
done through the private map.
## Summary
Add keyboard shortcuts (Cmd/Ctrl + Left/Right arrow keys) to navigate
between commits in the Profiler's snapshot view.
Moved `filteredCommitIndices` management and commit navigation logic
(`selectNextCommitIndex`, `selectPrevCommitIndex`) from
`SnapshotSelector` into `useCommitFilteringAndNavigation` used by
`ProfilerContext` to enable keyboard shortcuts from the top-level
Profiler component.
## How did you test this change?
- New tests in ProfilerContext-tests
- Built browser extension: `yarn build:<browser name>`
- tested in browser: `yarn run test:<browser name>`
- Manually verified Left/Right arrow navigation cycles through commits
- Verified navigation respects commit duration filter
- Verified reload-and-profile button unaffected
Chrome:
https://github.com/user-attachments/assets/01d2a749-13dc-4d08-8bcb-3d4d45a5f97c
Edge with duration filter:
https://github.com/user-attachments/assets/a7f76ff7-2a0b-4b9c-a0ce-d4449373308b
firefox mixing hotkey with clicking arrow buttons:
https://github.com/user-attachments/assets/48912d68-7c75-40f2-a203-5e6d7e6b2d99
Speculative fix to https://github.com/facebook/react/issues/35336
written by Claude.
I have verified that applying a similar patch locally to the repro from
#35336 does fix the crash.
I'm not familiar enough with the underlying APIs to tell whether the fix
is correct or sufficient.
Follow-up to https://github.com/facebook/react/pull/34641.
Similar to https://github.com/facebook/react/pull/35293,
https://github.com/facebook/react/pull/35294.
React DevTools backend can be used in non-DOM environments, so we have
to feature-check some DOM APIs.
For now I am just no-oping newly added commands for Native, we should
revisit this decision once we would roll out Suspense panel there, if
needed. I am not sure if scrolling will be required as much as it is
needed on Web.
`isReactNativeEnvironment()` check is kinda clowny, but we've been
relying on it for quite some time already.
AFAIK this is not needed to prevent any exploit but we don't really need
this. We allow functions on pretty much any other object anyway but
never on the "then" property since those would be serialized as Promises
by the client anyway.
FlightReplyServer are for client->server and ReactFlightClient is for
server->client. They're not 100% symmetrical.
We did a number of refactors to ReactFlightClient in PRs like #29823 and
#33664 to change the structure of the resolution. This PR brings those
changes to synchronize the two approaches. Which addresses deep
resolution of cycles and deferred error handling.
This also fixes a critical security vulnerability.
