Mirror of upstream PR #33438 #132
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
This is a fix for a problem where React retains shadow nodes longer than it needs to. The behaviour is shown in React Native test: https://github.com/facebook/react-native/blob/main/packages/react-native/src/private/__tests__/utilities/__tests__/ShadowNodeReferenceCounter-itest.js#L169 # Problem When React commits a new shadow tree, old shadow nodes are stored inside `fiber.alternate.stateNode`. This is not cleared up until React clones the node again. This may be problematic if mutation deletes a subtree, in that case `fiber.alternate.stateNode` will retain entire subtree until next update. In case of image nodes, this means retaining entire images. So when React goes from revision A: `<View><View /></View>` to revision B: `<View />`, `fiber.alternate.stateNode` will be pointing to Shadow Node that represents revision A..  # Fix To fix this, this PR adds a new feature flag `enableEagerAlternateStateNodeCleanup`. When enabled, `alternate.stateNode` is proactively pointed towards finishedWork's stateNode, releasing resources sooner. I have verified this fixes the issue [demonstrated by React Native tests](https://github.com/facebook/react-native/blob/main/packages/react-native/src/private/__tests__/utilities/__tests__/ShadowNodeReferenceCounter-itest.js#L169). All existing React tests pass when the flag is enabled.
…ook#33028) ## Summary Our builds generate files with a `.mjs` file extension. These are currently filtered out by `ReactFlightWebpackPlugin` so I am updating it to support this file extension. This fixes facebook#33155 ## How did you test this change? I built the plugin with this change and used `yalc` to test it in my project. I confirmed the expected files now show up in `react-client-manifest.json`
…ackTrace (facebook#33143) When we get the source location for "View source for this element" we should be using the enclosing function of the callsite of the child. So that we don't just point to some random line within the component. This is similar to the technique in facebook#33136. This technique is now really better than the fake throw technique, when available. So I now favor the owner technique. The only problem it's only available in DEV and only if it has a child that's owned (and not filtered). We could implement this same technique for the error that's thrown in the fake throwing solution. However, we really shouldn't need that at all because for client components we should be able to call `inspect(fn)` at least in Chrome which is even better.
…3159) This keeps track of the transition lane allocated for this event. I want to be able to use the current one within sync work flushing to know which lane needs its loading indicator cleared. It's also a bit weird that transition work scheduled inside sync updates in the same event aren't entangled with other transitions in that event when `flushSync` is. Therefore this moves it to reset after flushing. It should have no impact. Just splitting it out into a separate PR for an abundance of caution. The only thing this might affect would be if the React internals throws and it doesn't reset after. But really it doesn't really have to reset and they're all entangled anyway.
…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 <silbermann.sebastian@gmail.com>
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.
We highly recommend using Node Streams in Node.js because it's much faster and it is less likely to cause issues when chained in things like compression algorithms that need explicit flushing which the Web Streams ecosystem doesn't have a good solution for. However, that said, people want to be able to use the worse option for various reasons. The `.edge` builds aren't technically intended for Node.js. A Node.js environments needs to be patched in various ways to support it. It's also less optimal since it can't use [Node.js exclusive features](facebook#33388) and have to use [the lowest common denominator](facebook#27399) such as JS implementations instead of native. This adds a Web Streams build of Fizz but exclusively for Node.js so that in it we can rely on Node.js modules. The main difference compared to Edge is that SSR now uses `createHash` from the `"crypto"` module and imports `TextEncoder` from `"util"`. We use `setImmediate` instead of `setTimeout`. The public API is just `react-dom/server` which in Node.js automatically imports `react-dom/server.node` which re-exports the legacy bundle, Node Streams bundle and Node Web Streams bundle. The main downside is if your bundler isn't smart to DCE this barrel file. With Flight the difference is larger but that's a bigger lift.
…book#33443) This should allow us to visualize what facebook#33438 is trying to convey. An uncached 3rd-party component is displayed like this in the dev tools: <img width="1072" alt="Screenshot 2025-06-05 at 12 57 32" src="https://github.com/user-attachments/assets/d418ae23-d113-4dc9-98b8-ab426710454a" /> However, when the component is restored from a cache, it looks like this: <img width="1072" alt="Screenshot 2025-06-05 at 12 56 56" src="https://github.com/user-attachments/assets/a0e34379-d8c0-4b14-8b54-b5c06211232b" /> The `Server Components ⚛` track is missing completely here, and the `Loading profile...` phase also took way longer than without caching the 3rd-party component. On `main`, the `Server Components ⚛` track is not missing: <img width="1072" alt="Screenshot 2025-06-05 at 14 31 20" src="https://github.com/user-attachments/assets/c35e405d-27ca-4b04-a34c-03bd959a7687" /> The cached 3rd-party component starts before the current render, and is also not excluded here, which is of course expected without facebook#33438.
… for Webpack (facebook#33442) Like facebook#33441 but for Flight. This is just one of the many combinations needed. I'm just starting with one.
When I added the `ready_for_review` event in facebook#32344, no notifications for opened draft PRs were sent due to some other condition. This is not the case anymore, so we need to exclude draft PRs from triggering a notification when the workflow is run because of an `opened` event. This event is still needed because the `ready_for_review` event only fires when an existing draft PR is converted to a non-draft state. It does not trigger for pull requests that are opened directly as ready-for-review.
…ook#33447) ## Summary Enables the `enableEagerAlternateStateNodeCleanup` feature flag for all variants, while maintaining the `__VARIANT__` for the internal React Native flavor for backtesting reasons. ## How did you test this change? ``` $ yarn test ```
This shouldn't be needed now that the lint rule was move
Block the view transition on suspensey images Up to 500ms just like the client. We can't use `decode()` because a bug in Chrome where those are blocked on `startViewTransition` finishing we instead rely on sync decoding but also that the image is live when it's animating in and we assume it doesn't start visible. However, we can block the View Transition from starting on the `"load"` or `"error"` events. The nice thing about blocking inside `startViewTransition` is that we have already done the layout so we can only wait on images that are within the viewport at this point. We might want to do that in Fiber too. If many image doesn't have fixed size but need to load first, they can all end up in the viewport. We might consider only doing this for images that have a fixed size or only a max number that doesn't have a fixed size.
…undaries (facebook#33454) We want to make sure that we can block the reveal of a well designed complete shell reliably. In the Suspense model, client transitions don't have any way to implicitly resolve. This means you need to use Suspense or SuspenseList to explicitly split the document. Relying on implicit would mean you can't add a Suspense boundary later where needed. So we highly encourage the use of them around large content. However, if you have constructed a too large shell (e.g. by not adding any Suspense boundaries at all) then that might take too long to render on the client. We shouldn't punish users (or overzealous metrics tracking tools like search engines) in that scenario. This opts out of render blocking if the shell ends up too large to be intentional and too slow to load. Instead it deopts to showing the content split up in arbitrary ways (browser default). It only does this for SSR, and not client navs so it's not reliable. In fact, we issue an error to `onError`. This error is recoverable in that the document is still produced. It's up to your framework to decide if this errors the build or just surface it for action later. What should be the limit though? There's a trade off here. If this limit is too low then you can't fit a reasonably well built UI within it without getting errors. If it's too high then things that accidentally fall below it might take too long to load. I came up with 512kB of uncompressed shell HTML. See the comment in code for the rationale for this number. TL;DR: Data and theory indicates that having this much content inside `rel="expect"` doesn't meaningfully change metrics. Research of above-the-fold content on various websites indicate that this can comfortable fit all of them which should be enough for any intentional initial paint.
…ebook#33456) Follow up to facebook#33442. This is the bundled version. To keep type check passes from exploding and the maintainance of the annoying `paths: []` list small, this doesn't add this to flow type checks. We might miss some config but every combination should already be covered by other one passes. I also don't add any jest tests because to test these double export entry points we need conditional importing to cover builds and non-builds which turns out to be difficult for the Flight builds so these aren't covered by any basic build tests. This approach is what I'm going for, for the other bundlers too.
…acebook#33457) Same as facebook#33456 and facebook#33442 but for Turbopack and Parcel.
Missed these the last time.
Adding throttling or delaying on images, can obviously impact metrics. However, it's all in the name of better actual user experience overall. (Note that it's not strictly worse even for metric. Often it's actually strictly better due to less work being done overall thanks to batching.) Metrics can impact things like search ranking but I believe this is on a curve. If you're already pretty good, then a slight delay won't suddenly make you rank in a completely different category. Similarly, if you're already pretty bad then a slight delay won't make it suddenly way worse. It's still in the same realm. It's just one weight of many. I don't think this will make a meaningful practical impact and if it does, that's probably a bug in the weights that will get fixed. However, because there's a race to try to "make everything green" in terms of web vitals, if you go from green to yellow only because of some throttling or suspensey images, it can feel bad. Therefore this implements a heuristic where if the only reason we'd miss a specific target is because of throttling or suspensey images, then we shorten the timeout to hit the metric. This is a worse user experience because it can lead to extra flashing but feeling good about "green" matters too. If you then have another reveal that happens to be the largest contentful paint after that, then that's throttled again so that it doesn't become flashy after that. If you've already missed the deadline then you're not going to hit your metric target anyway. It can affect average but not median. This is mainly about LCP. It doesn't affect FCP since that doesn't have a throttle. If your LCP is the same as your FCP then it also doesn't matter. We assume that `performance.now()`'s zero point starts at the "start of the navigation" which makes this simple. Even if we used the `PerformanceNavigationTiming` API it would just tell us the same thing. This only implements for Fizz since these metrics tend to currently only by tracked for initial loads, but with soft navs tracking we could consider implementing the same for Fiber throttles.
When deeply nested Suspense boundaries inside a fallback of another boundary resolve it is possible to encounter situations where you either attempt to flush an aborted Segment or you have a boundary without any root segment. We intended for both of these conditions to be impossible to arrive at legitimately however it turns out in this situation you can. The fix is two-fold 1. allow flushing aborted segments by simply skipping them. This does remove some protection against future misconfiguraiton of React because it is no longer an invariant that you hsould never attempt to flush an aborted segment but there are legitimate cases where this can come up and simply omitting the segment is fine b/c we know that the user will never observe this. A semantically better solution would be to avoid flushing boudaries inside an unneeded fallback but to do this we would need to track all boundaries inside a fallback or create back pointers which add to memory overhead and possibly make GC harder to do efficiently. By flushing extra we're maintaining status quo and only suffer in performance not with broken semantics. 2. when queuing completed segments allow for queueing aborted segments and if we are eliding the enqueued segment allow for child segments that are errored to be enqueued too. This will mean that we can maintain the invariant that a boundary must have a root segment the first time we flush it, it just might be aborted (see point 1 above). This change has two seemingly similar test cases to exercise this fix. The reason we need both is that when you have empty segments you hit different code paths within Fizz and so each one (without this fix) triggers a different error pathway. This change also includes a fix to our tests where we were not appropriately setting CSPnonce back to null at the start of each test so in some contexts scripts would not run for some tests
Reverts facebook#33457, facebook#33456 and facebook#33442. There are too many issues with wrappers, lazy init, stateful modules, duplicate instantiation of async_hooks and duplication of code. Instead, we'll just do a wrapper polyfill that uses Node Streams internally. I kept the client indirection files that I added for consistency with the server though.
…k#33473) This effectively lets us consume Web Streams in a Node build. In fact the Node entry point is now just adding Node stream APIs. For the client, this is simple because the configs are not actually stream type specific. The server is a little trickier.
New take on facebook#33441. This uses a wrapper instead of a separate bundle.
…k#33474) This needs some tweaks to the implementation and a conversion but simple enough. --------- Co-authored-by: Hendrik Liebau <mail@hendrik-liebau.de>
kushxg
force-pushed
the
upstream-pr-33438
branch
2 times, most recently
from
4ce29ea to
78f7790
Compare
…facebook#33478) I noticed that the ThirdPartyComponent in the fixture was showing the wrong stack and the `"use third-party"` is in the wrong location. <img width="628" alt="Screenshot 2025-06-06 at 11 22 11 PM" src="https://github.com/user-attachments/assets/f0013380-d79e-4765-b371-87fd61b3056b" /> When creating the initial JSX inside the third party server, we should make sure that it has no owner. In a real cross-server environment you get this by default by just executing in different context. But since the fixture example is inside the same AsyncLocalStorage as the parent it already has an owner which gets transferred. So we should make sure that were we create the JSX has no owner to simulate this. When we then parse a null owner on the receiving side, we replace its owner/stack with the owner/stack of the call to `createFrom...` to connect them. This worked fine with only two environments. The bug was that when we did this and then transferred the result to a third environment we took the original parsed stack trace. We should instead parse a new one from the replaced stack in the current environment. The second bug was that the `"use third-party"` badge ends up in the wrong place when we do this kind of thing. Because the stack of the thing entering the new environment is the call to `createFrom...` which is in the old environment even though the component itself executes in the new environment. So to see if there's a change we should be comparing the current environment of the task to the owner's environment instead of the next environment after the task. After: <img width="494" alt="Screenshot 2025-06-07 at 1 13 28 AM" src="https://github.com/user-attachments/assets/e2e870ba-f125-4526-a853-bd29f164cf09" />
We don't really need the ID here. It's the same as the task id. We pass the task instead. It's really just used as a flag. Update comments Update comments
…mponent We also ignore any await entries that finished before the request started since they're irrelevant to the loading sequence.
…aranteed to be set yet
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#33438