[pull] main from facebook:main #177
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
![pull[bot]](https://avatars.githubusercontent.com/in/12910?s=60&v=4){kind=small}
When I added the `ready_for_review` event in #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.
## 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 (#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.
) Follow up to #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.
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 #33457, #33456 and #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.
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 #33441. This uses a wrapper instead of a separate bundle.
This needs some tweaks to the implementation and a conversion but simple enough. --------- Co-authored-by: Hendrik Liebau <[email protected]>
…#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" />
Technically the async call graph spans basically all the way back to the start of the app potentially, but we don't want to include everything. Similarly we don't want to include everything from previous components in every child component. So we need some heuristics for filtering out data. We roughly want to be able to inspect is what might contribute to a Suspense loading sequence even if it didn't this time e.g. due to a race condition. One flaw with the previous approach was that awaiting a cached promise in a sibling that happened to finish after another sibling would be excluded. However, in a different race condition that might end up being used so I wanted to include an empty "await" in that scenario to have some association from that component. However, for data that resolved fully before the request even started, it's a little different. This can be things that are part of the start up sequence of the app or externally cached data. We decided that this should be excluded because it doesn't contribute to the loading sequence in the expected scenario. I.e. if it's cached. Things that end up being cache misses would still be included. If you want to test externally cached data misses, then it's up to you or the framework to simulate those. E.g. by dropping the cache. This also helps free up some noise since static / cached data can be excluded in visualizations. I also apply this principle to forwarding debug info. If you reuse a cached RSC payload, then the Server Component render time and its awaits gets clamped to the caller as if it has zero render/await time. The I/O entry is still back dated but if it was fully resolved before we started then it's completely excluded.
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.
See Commits and Changes for more details.
Created by
pull[bot] (v2.0.0-alpha.1)
Can you help keep this open source service alive? 💖 Please sponsor : )