Mirror of upstream PR #33942 #273
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
…onLane (facebook#33188) When we're entangled with an async action lane we use that lane instead of the currentEventTransitionLane. Conversely, if we start a new async action lane we reuse the currentEventTransitionLane. So they're basically supposed to be in sync but they're not if you resolve the async action and then schedule new stuff in the same event. Then you end up with two transitions in the same event with different lanes. By stashing it like this we fix that but it also gives us an opportunity to check just the currentEventTransitionLane to see if this event scheduled any regular Transition updates or Async Transitions.
Stacked on facebook#33159. This implements `onDefaultTransitionIndicator`. The sequence is: 1) In `markRootUpdated` we schedule Transition updates as needing `indicatorLanes` on the root. This tracks the lanes that currently need an indicator to either start or remain going until this lane commits. 2) Track mutations during any commit. We use the same hook that view transitions use here but instead of tracking it just per view transition scope, we also track a global boolean for the whole root. 3) If a sync/default commit had any mutations, then we clear the indicator lane for the `currentEventTransitionLane`. This requires that the lane is still active while we do these commits. See facebook#33159. In other words, a sync update gets associated with the current transition and it is assumed to be rendering the loading state for that corresponding transition so we don't need a default indicator for this lane. 4) At the end of `processRootScheduleInMicrotask`, right before we're about to enter a new "event transition lane" scope, it is no longer possible to render any more loading states for the current transition lane. That's when we invoke `onDefaultTransitionIndicator` for any roots that have new indicator lanes. 5) When we commit, we remove the finished lanes from `indicatorLanes` and once that reaches zero again, then we can clean up the default indicator. This approach means that you can start multiple different transitions while an indicator is still going but it won't stop/restart each time. Instead, it'll wait until all are done before stopping. Follow ups: - [x] Default updates are currently not enough to cancel because those aren't flush in the same microtask. That's unfortunate. facebook#33186 - [x] Handle async actions before the setState. Since these don't necessarily have a root this is tricky. facebook#33190 - [x] Disable for `useDeferredValue`. ~Since it also goes through `markRootUpdated` and schedules a Transition lane it'll get a default indicator even though it probably shouldn't have one.~ EDIT: Turns out this just works because it doesn't go through `markRootUpdated` when work is left behind. - [x] Implement built-in DOM version by default. facebook#33162
…n was scheduled (facebook#33186) Stacked on facebook#33160. The purpose of this is to avoid calling `onDefaultTransitionIndicator` when a Default priority update acts as the loading indicator, but still call it when unrelated Default updates happens nearby. When we schedule Default priority work that gets batched with other events in the same frame more or less. This helps optimize by doing less work. However, that batching means that we can't separate work from one setState from another. If we would consider all Default priority work in a frame when determining whether to show the default we might never show it in cases like when you have a recurring timer updating something. This instead flushes the Default priority work eagerly along with the sync work at the end of the event, if this event scheduled any Transition work. This is then used to determine if the default indicator needs to be shown.
…acebook#33162) Stacked on facebook#33160. By default, if `onDefaultTransitionIndicator` is not overridden, this will trigger a fake Navigation event using the Navigation API. This is intercepted to create an on-going navigation until we complete the Transition. Basically each default Transition is simulated as a Navigation. This triggers the native browser loading state (in Chrome at least). So now by default the browser spinner spins during a Transition if no other loading state is provided. Firefox and Safari hasn't shipped Navigation API yet and even in the flag Safari has, it doesn't actually trigger the native loading state. To ensures that you can still use other Navigations concurrently, we don't start our fake Navigation if there's one on-going already. Similarly if our fake Navigation gets interrupted by another. We wait for on-going ones to finish and then start a new fake one if we're supposed to be still pending. There might be other routers on the page that might listen to intercept Navigation Events. Typically you'd expect them not to trigger a refetch when navigating to the same state. However, if they want to detect this we provide the `"react-transition"` string in the `info` field for this purpose.
…o root associated (facebook#33190) Stacked on facebook#33160, facebook#33162, facebook#33186 and facebook#33188. We have a special case that's awkward for default indicators. When you start a new async Transition from `React.startTransition` then there's not yet any associated root with the Transition because you haven't necessarily `setState` on anything yet until the promise resolves. That's what `entangleAsyncAction` handles by creating a lane that everything entangles with until all async actions are done. If there are no sync updates before the end of the event, we should trigger a default indicator until either the async action completes without update or if it gets entangled with some roots we should keep it going until those roots are done.
Not sure where this was coming from.
…sition (facebook#33191) And that doesn't disable with `update="none"`. The principle here is that we want the content of a Portal to animate if other things are animating with it but if other things aren't animating then we don't.
…ook#33200) This is a partial revert of facebook#33094. It's true that we don't need the server and client ViewTransition names to line up. However the server does need to be able to generate deterministic names for itself. The cheapest way to do that is using the useId algorithm. When it's used by the server, the client needs to also materialize an ID even if it doesn't use it.
…3194) Removes the `isFallback` flag on Tasks and tracks it on the formatContext instead. Less memory and avoids passing and tracking extra arguments to all the pushStartInstance branches that doesn't need it. We'll need to be able to track more Suspense related contexts on this for View Transitions anyway.
…facebook#33206) Stacked on facebook#33194 and facebook#33200. When Suspense boundaries reveal during streaming, the Fizz runtime will be responsible for animating the reveal if necessary (not in this PR). However, for the future runtime to know what to do it needs to know about the `<ViewTransition>` configuration to apply. Ofc, these are virtual nodes that disappear from the HTML. We could model them as comments like we do with other virtual nodes like Suspense and Activity. However, that doesn't let us target them with querySelector and CSS (for no-JS transitions). We also don't have to model every ViewTransition since not every combination can happen using only the server runtime. So instead this collapses `<ViewTransition>` and applies the configuration to the inner DOM nodes. ```js <ViewTransition name="hi"> <div /> <div /> </ViewTransition> ``` Becomes: ```html <div vt-name="hi" vt-update="auto"></div> <div vt-name="hi_1" vt-update="auto"></div> ``` I use `vt-` prefix as opposed to `data-` to keep these virtual attributes away from user specific ones but we're effectively claiming this namespace. There are four triggers `vt-update`, `vt-enter`, `vt-exit` and `vt-share`. The server resolves which ones might apply to this DOM node. The value represents the class name (after resolving view-transition-type mappings) or `"auto"` if no specific class name is needed but this is still a trigger. The value can also be `"none"`. This is different from missing because for example an `vt-update="none"` will block mutations inside it from triggering the boundary where as a missing `vt-update` would bubble up to be handled by a parent. `vt-name` is technically only necessary when `vt-share` is specified to find a pair. However, since an explicit name can also be used to target specific CSS selectors, we include it even for other cases. We want to exclude as many of these annotations as possible. `vt-enter` can only affect the first DOM node inside a Suspense boundary's content since the reveal would cause it to enter but nothing deeper inside. Similarly `vt-exit` can only affect the first DOM node inside a fallback. So for every other case we can exclude them. (For future MPA ViewTransitions of the whole document it might also be something we annotate to children inside the `<body>` as well.) Ideally we'd only include `vt-enter` for Suspense boundaries that actually flushed a fallback but since we prepare all that content earlier it's hard to know. `vt-share` can be anywhere inside an fallback or content. Technically we don't have to include it outside the root most Suspense boundary or for boundaries that are inlined into the root shell. However, this is tricky to detect. It would also not be correct for future MPA ViewTransitions because in that case the shared scenario can affect anything in the two documents so it needs to be in every node everywhere which is effectively what we do. If a `share` class is specified but it has no explicit name, we can exclude it since it can't match anything. `vt-update` is only necessary if something below or a sibling might update like a Suspense boundary. However, since we don't know when rendering a segment if it'll later asynchronously add a Suspense boundary later we have to assume that anywhere might have a child. So these are always included. We collapse to use the inner most one when directly nested though since that's the one that ends up winning. There are some weird edge cases that can't be fully modeled by the lack of virtual nodes.
Update the changelog.
For debugging purposes, log author_association
Noop detection for xplat syncs broke because `eslint-plugin-react-hooks` uses versions like: - `0.0.0-experimental-d85f86cf-20250514` But xplat expects them to be of the form: - `19.2.0-native-fb-63d664b2-20250514` This PR fixes the noop by ignoring `eslint-plugin-react-hooks/package.json` changes. This means we won't create a sync if only that package.json changes, but that should be rare and we can follow up with better detection if needed. [Example failed action](https://github.com/facebook/react/actions/runs/15032346805/job/42247414406): <img width="1031" alt="Screenshot 2025-05-15 at 11 31 17 AM" src="https://github.com/user-attachments/assets/d902079c-1afe-4e18-af1d-25e60e28929e" /> I believe the regression was caused by facebook#33104
…cebook#33295) We decremented `allPendingTasks` after invoking `onShellReady`. Which means that in that scope it wasn't considered fully complete. Since the pattern for flushing in Node.js is to start piping in `onShellReady` and that's how you can get sync behavior, this led us to think that we had more work left to do. For example we emitted the `writeShellTimeInstruction` in this scenario before.
…ion (facebook#33293) When needed. For the external runtime we always include this wrapper. For others, we only include it if we have an ViewTransitions affecting. If we discover the ViewTransitions late, then we can upgrade an already emitted instruction. This doesn't yet do anything useful with it, that's coming in a follow up. This is just the mechanism for how it gets installed.
So they can be shared by server. Incorporates the types from definitely typed too.
…ebook#33306) Basically we track a `SuspenseListRow` on the task. These keep track of "pending tasks" that block the row. A row is blocked by: - First itself completing rendering. - A previous row completing. - Any tasks inside the row and before the Suspense boundary inside the row. This is mainly because we don't yet know if we'll discover more SuspenseBoundaries. - Previous row's SuspenseBoundaries completing. If a boundary might get outlined, then we can't consider it completed until we have written it because it determined whether other future boundaries in the row can finish. This is just handling basic semantics. Features not supported yet that need follow ups later: - CSS dependencies of previous rows should be added as dependencies of future row's suspense boundary. Because otherwise if the client is blocked on CSS then a previous row could be blocked but the server doesn't know it. - I need a second pass on nested SuspenseList semantics. - `revealOrder="together"` - `tail="hidden"`/`tail="collapsed"`. This needs some new runtime semantics to the Fizz runtime and to allow the hydration to handle missing rows in the HTML. This should also be future compatible with AsyncIterable where we don't know how many rows upfront. - Need to double check resuming semantics. --------- Co-authored-by: Sebastian "Sebbie" Silbermann <[email protected]>
We were printing "Custom" instead of "hook".
We support AsyncIterable (more so when it's a cached form like in coming from Flight) as children. This fixes some warnings and bugs when passed to SuspenseList. Ideally SuspenseList with `tail="hidden"` should support unblocking before the full result has resolved but that's an optimization on top. We also might want to change semantics for this for `revealOrder="backwards"` so it becomes possible to stream items in reverse order.
Follow up to facebook#33306. If we're nested inside a SuspenseList and we have a row, then we can point our last row to block the parent row and unblock the parent when the last child unblocks.
For now we removed Rust from the codebase, remove this leftover script. Also remove some dupes and Rust related files from `.gitignore`.
Stacked on facebook#33308. For "together" mode, we can be a self-blocking row that adds all its boundaries to the blocked set, but there's no parent row that unblocks it. A particular quirk of this mode is that it's not enough to just unblock them all on the server together. Because if one boundary downloads all its html and then issues a complete instruction it'll appear before the others while streaming in. What we actually want is to reveal them all in a single batch. This implementation takes a short cut by unblocking the rows in `flushPartialBoundary`. That ensures that all the segments of every boundary has a chance to flush before we start emitting any of the complete boundary instructions. Once the last one unblocks, all the complete boundary instructions are queued. Ideally this would be a single `<script>` tag so that they can't be split up even if we get a chunk containing some of them. ~A downside of this approach is that we always outline these boundaries. We could inline them if they all complete before the parent flushes. E.g. by checking if the row is blocked only by its own boundaries and if all the boundaries would fit without getting outlined, then we can inline them all at once.~ I went ahead and did this because it solves an issue with `renderToString` where it doesn't support the script runtime so it can only handle this if inlined.
…future rows (facebook#33312) Stacked on facebook#33311. When a row contains Suspense boundaries that themselves depend on CSS, they will not resolve until the CSS has loaded on the client. We need future rows in a list to be blocked until this happens. We could do something in the runtime but a simpler approach is to just add those CSS dependencies to all those boundaries as well. To do this, we first hoist the HoistableState from a completed boundary onto its parent row. Then when the row finishes do we hoist it onto the next row and onto any boundaries within that row.
…#33318) Fixes a bug that we caused us to hang after an abort because we didn't manage the ref count correctly.
I missed setting the `keyPath` because the `renderChildrenArray` that this is forked from doesn't need to set a path but since this is rendered from the `SuspenseList` element it needs it.
…ebook#33205) We have many cases internally where the `containerInstance` resolves to a comment node. `restoreRootViewTransitionName` is called when `enableViewTransition` is on, even without introducing a `<ViewTransition />`. So that means it can crash pages because `containerInstance.style` is `undefined` just by turning on the flag. This skips cancel/restore of root view transition name if a comment node is the root.
There's an interesting case when a SuspenseList is partially prerendered but some of the completed boundaries are blocked by rows to be resumed. This handles it but just unblocking the future rows to avoid stalling. However, the correct semantics will need special handling in the postponed state.
When a new child of a fragment instance is inserted, we need to notify the instance to keep any relevant tracking up to date. For example, we automatically observe the new child with any active IntersectionObserver. For mutable renderers (DOM), we reuse the existing traversal in `commitPlacement` that does the insertions for HostComponents. Immutable renderers (Fabric) exit this path before the traversal though, so currently we can't notify the fragment instances. Here I've created a separate traversal in `commitPlacement`, specifically for immutable renders when `enableFragmentRefs` is on.
…listeners (facebook#33743) This is the same as we do for currently rendering tasks. They get effectively sync aborted when the listener is invoked. We potentially miss out on some debug info in that case but that would only apply to any entries inside the stream which doesn't really have their own debug info anyway.
In playground it's helpful to show all errors, even those that don't completely abort compilation. For example, to help demonstrate that the compiler catches things like setState in effects. This detects these errors and ensures we show them.
…ebook#33746) * Error for `eval()` * More specific error message for `with (expr) { ... }` syntax * More specific error message for class declarations --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33746). * facebook#33752 * facebook#33751 * facebook#33750 * facebook#33748 * facebook#33747 * __->__ facebook#33746
…k#33747) Supports inline enum declarations in both Flow and TS by treating the node as pass-through (enums can't capture values mutably). Related, this PR extends the set of type-related declarations that we ignore. Previously we threw a todo for things like DeclareClass or DeclareVariable, but these are type related and can simply be dropped just like we dropped TypeAlias. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33747). * facebook#33753 * facebook#33752 * facebook#33751 * facebook#33750 * facebook#33748 * __->__ facebook#33747
…atements (facebook#33748) import, export, and TS namespace statements can only be used at the top-level of a module, which is enforced by parsers already. Here we add a backup validation of that. As of this PR, we now have only major statement type (class declarations) listed as a todo. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33748). * facebook#33753 * facebook#33752 * facebook#33751 * facebook#33750 * __->__ facebook#33748
We use this variant for syntax we intentionally don't support: with statements, eval, and inline class declarations. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33750). * facebook#33753 * facebook#33752 * facebook#33751 * __->__ facebook#33750 * facebook#33748
…ook#33755) When postponing the root we encode the segment Id into the postponed state but we should really be reseting it to zero so we can restart the counter from the beginning when the resume is actually just a re-render. This also no longer assigns the root segment id based on the postponed state when resuming the root for the same reason. In the future we may use the embedded replay segment id if we implement resuming the root without re-rendering everything but that is not yet implemented or planned.
We typically treat an empty message as closing the debug channel stream but for the Noop renderer we don't use an intermediate stream but just pass the message through. https://github.com/facebook/react/blob/bbc13fa17be8eebef3e6ee47f48c76c0c44e2f36/packages/react-server-dom-webpack/src/client/ReactFlightDOMClientBrowser.js#L59-L60 For that simple case we should just treat it as a close without an intermediate stream.
This lets us pass a writable on the server side and readable on the client side to send debug info through a separate channel so that it doesn't interfere with the main payload as much. The main payload refers to chunks defined in the debug info which means it's still blocked on it though. This ensures that the debug data has loaded by the time the value is rendered so that the next step can forward the data. This will be a bit fragile to race conditions until facebook#33665 lands. Another follow up needed is the ability to skip the debug channel on the receiving side. Right now it'll block forever if you don't provide one since we're blocking on the debug data.
…osures (facebook#33544) Summary: useEffectEvent is meant to be used specifically in combination with useEffect, and using the feature in arbitrary closures can lead to surprising reactivity semantics. In order to minimize risk in the experimental rollout, we are going to restrict its usage to being called directly inside an effect or another useEffectEvent, effectively enforcing the function coloring statically. Without an effect system this is the best we can do.
…33757) React Elements reference debug data (their stack and owner) in the debug channel. If the debug channel isn't wired up this can block the client from resolving. We can infer that if there's no debug channel wired up and the reference wasn't emitted before the element, then it's probably because it's in the debug channel. So we can skip it. This should also apply to debug chunks but they're not yet blocking until facebook#33665 lands.
facebook#33788) ## Summary The `TSAsExpression` and `TSNonNullExpression` nodes are supported by `lowerExpression()` but `isReorderableExpression()` does not check if they can be reordered. This PR updates `isReorderableExpression()` to handle these two node types by adding cases that fall through to the existing `TypeCastExpression` case. We ran `react-compiler-healthcheck` at scale on several of our repos and found dozens of `` (BuildHIR::node.lowerReorderableExpression) Expression type `TSAsExpression` cannot be safely reordered`` errors and a handful for `TSNonNullExpression`. ## How did you test this change? In this case I added two fixture tests
…acebook#33797) This fixes displaying incorrect component render entries on a timeline, when we are reconnecting passive effects. ### Before <img width="2318" height="1127" alt="1" src="https://github.com/user-attachments/assets/9b6b2824-d2de-43a3-8615-2c45d67c3668" /> The cloned nodes will persist original `actualStartTime`, when these were first mounted. When we "replay", the end time will be "now" or whatever the actual start time of the sibling. Depending on when this is being recorded, the diff between end and start could be tens of seconds and doesn't represent what React was doing. We shouldn't log these entries at all. ### After We are only logging newly finished renders, but could potentially loose renders that never commit.
…it Promises (facebook#33798) We already do this with `"new Promise"` and `"Promise.then"`. There are also many helpers that both create promises and awaits other promises inside of it like `Promise.all`. The way this is filtered is different from just filtering out all anonymous stacks since they're used to determine where the boundary is between ignore listed and user space. Ideally we'd cover more wrappers that are internal to Promise libraries.
--- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/33799). * facebook#33800 * __->__ facebook#33799
…facebook#33665) This resolves an outstanding issue where it was possible for debug info and console logs to become out of order if they up blocked. E.g. by a future reference or a client reference that hasn't loaded yet. Such as if you console.log a client reference followed by one that doesn't. This encodes the order similar to how the stream chunks work. This also blocks the main chunk from resolving until the last debug info has fully loaded, including future references and client references. This also ensures that we could send some of that data in a different stream, since then it can come out of order.
…k#33943) ## Summary We have thoroughly tested this flag in production and proved stability and performance, so we can clean it up and "ship it".
…acebook#33947) If you pass a promise to a client component to be rendered `<Client promise={promise} />` then there's an internal await inside Flight. There might also be user space awaits but those awaits may already have happened before we render this component. Conceptually they were part of the parent component and not this component. It's tricky to attribute which await should be used for the stack in this case. If we can't find an await we can use the JSX callsite as the stack frame. However, we don't want to do this for simple cases like if you return a non-native Promise from a Server Component. Since that would now use the stack of the thing that rendered the Server Component which is worse than the stack of the I/O. To fix this, I update the `debugOwner`/`debugTask`/`debugStack` when we start rendering inside the Server Component. Conceptually these represent the "parent" component and is used for errors referring to the parent like when we serialize client component props the parent is the JSX of the client component. However, when we're directly inside the Server Component we don't have a callsite of the parent really. Conceptually it would be the return call of the Server Component. This might negatively affect other types of errors but I think this is ok since this feature mainly exists for the case when you enter the child JSX.
…acebook#33948) We need a "value" to represent the I/O that was loaded. We don't normally actually use the Promise at the callsite that started the I/O because that's usually deep inside internals. Instead we override the value of the I/O entry with the Promise that was first awaited in user space. This means that you could potentially have different values depending on if multiple things await the same I/O. We just take one of them. (Maybe we should actually just write the first user space awaited Promise as the I/O entry? This might instead have other implications like less deduping.) When you pass a Promise forward, we may skip the awaits that happened in earlier components because they're not part of the currently rendering component. That's mainly for the stack and time stamps though. The value is still probably conceptually the best value because it represents the I/O value as far user space is concerned. This writes the I/O early with the first await we find in user space even if we're not going to use that particular await for the stack.
This makes it so you can click the source location itself to view the source. This is similar styling as the link to jump to function props like events and actions. We're going to need a lot more linkifying to jump to various source locations. Also, I always was trying to click this file anyway. Hover state: <img width="485" height="382" alt="Screenshot 2025-07-21 at 4 36 10 PM" src="https://github.com/user-attachments/assets/1f0f8f8c-6866-4e62-ab84-1fb5ba012986" />
facebook#33955) In RSC and other stacks now we use a lot of `ReactFunctionLocation` type to represent the location of a function. I.e. the location of the beginning of the function (the enclosing line/col) that is represented by the "Source" of the function. This is also what the parent Component Stacks represents. As opposed to `ReactCallSite` which is what normal stack traces and owner stacks represent. I.e. the line/column number of the callsite into the next function. We can start sharing more code by using the `ReactFunctionLocation` type to represent the component source location and it also helps clarify which ones are function locations and which ones are callsites as we start adding more stack traces (e.g. for async debug info and owner stack traces).
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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#33942