Mirror of upstream PR #34718 #485
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
…acebook#34192) This fixes an edge case where you abort the render while rendering a component that ends up Suspending. It technically only applied if you were deep enough to be inside `renderNode` and was not susceptible to hanging if the abort + suspending component was being tried inside retryRenderTask/retryReplaytask. The fix is to preempt the thenable checks in renderNode and check if the request is aborting and if so just bubble up to the task handler. The reason this hung before is a new task would get scheduled after we had aborted every other task (minus the currently rendering one). This led to a situation where the task count would not hit zero.
This did an unnecessary bind allocation even if there's cache hit.
The skeletons right now are too jarring because they're visually heavier than the content that comes in later. This makes them draw attention to themselves as flashing things. A good skeleton and loading indicator should ideally start as invisible as possible and then gradually become more visible the longer time passes so that if it loads quickly then it was never much visible at all. Even at its max it should never be heavier weight than the final content so that it visually reverts into lesser. Another rule of thumb is that it should be as close as possible to the final content in size but if it's unknown it should always be smaller than the final content so that the content grows into its slot rather than the slot contracting. This makes the skeleton fade from invisible into the dimmest color just as a subtle hint that something is still loading. I also added a missing skeleton since the stack traces in rendered by can now suspend while source mapping. The other tweak I did is use disabled buttons in all the cases where we load the ability to enable a button. This is more subtle and if you hover over you can see why it's still disabled. Rather than flashing the button each time you change element.
…4181) Same as facebook#34166 but for Suspensey images. The trick here is to check the `SuspenseyImagesMode` since not all versions of React and not all subtrees will have Suspensey images enabled yet. The other trick is to read back from `currentSrc` to get the image url we actually resolved to in this case. Similar to how for Suspensey CSS we check if the media query would've matched. <img width="591" height="205" alt="Screenshot 2025-08-11 at 9 32 56 PM" src="https://github.com/user-attachments/assets/ac98785c-d3e0-407c-84e0-c27f86c0ecac" />
…facebook#34069) Found a couple of issues while integrating FragmentInstance#compareDocumentPosition into Fabric. 1. Basic checks of nested host instances were inaccurate. For example, checking the first child of the first child of the Fragment would not return CONTAINED_BY. 2. Then fixing that logic exposed issues with Portals. The DOM positioning relied on the assumption that the first and last top-level children were in the same order as the Fiber tree. I added additional checks against the parent's position in the DOM, and special cased a portaled Fragment by getting its DOM parent from the child instance, rather than taking the instance from the Fiber return. This should be accurate in more cases. Though its still a guess and I'm not sure yet I've covered every variation of this. Portals are hard to deal with and we may end up having to push more results towards IMPLEMENTATION_SPECIFIC if accuracy is an issue.
…pense boundary (facebook#34201) This computes a min and max range for the whole suspense boundary even when selecting a single component so that each component in a boundary has a consistent range. The start of this range is the earliest start of I/O in that boundary or the end of the previous suspense boundary, whatever is earlier. If the end of the previous boundary would make the range large, then we cap it since it's likely that the other boundary was just an independent render. The end of the range is the latest end of I/O in that boundary. If this is smaller than the end of the previous boundary plus the 300ms throttle, then we extend the end. This visualizes what throttling could potentially do if the previous boundary committed right at its end. Ofc, it might not have committed exactly at that time in this render. So this is just showing a potential throttle that could happen. To see actual throttle, you look in the Performance Track. <img width="661" height="353" alt="Screenshot 2025-08-14 at 12 41 43 AM" src="https://github.com/user-attachments/assets/b0155e5e-a83f-400c-a6b9-5c38a9d8a34f" /> We could come up with some annotation to highlight that this is eligible to be throttled in this case. If the lines don't extend to the edge, then it's likely it was throttled.
Stacked on facebook#34069 Same basic semantics as the react-dom for determining document position of a Fragment compared to a given node. It's simpler here because we don't have to deal with inserted nodes or portals. So we can skip a bunch of the validation logic. The logic for handling empty fragments is the same so I've split out `compareDocumentPositionForEmptyFragment` into a shared module. There doesn't seem to be a great place to put shared DOM logic between Fabric and DOM configs at the moment. There may be more of this coming as we add more and more DOM APIs to RN. For testing I've written Fantom tests internally which pass the basic cases on this build. The renderer we have configured for Fabric tests in the repo doesn't support the Element APIs we need like `compareDocumentPosition`.
If you have a ref that the compiler doesn't know is a ref (say, a value returned from a custom hook) and try to assign its `.current = ...`, we currently fail with a generic error that hook return values are not mutable. However, an assignment to `.current` specifically is a very strong hint that the value is likely to be a ref. So in this PR, we track the reason for the mutation and if it ends up being an error, we use it to show an additional hint to the user. See the fixture for an example of the message. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34125). * facebook#34126 * __->__ facebook#34125 * facebook#34124
Hints are meant as additional information to present to the developer about an error. The first use-case here is for the suggestion to name refs with "-Ref" if we encounter a mutation that looks like it might be a ref. The original error printing used a second error detail which printed the source code twice, a hint with just extra text is less noisy.
…#34028) --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34028). * facebook#34029 * __->__ facebook#34028
The new mutation/aliasing model significantly expands on the idea of FunctionEffect. The type (and its usage in HIRFunction.effects) was only necessary for the now-deleted old inference model so we can clean up this code now.
…34200) We currently only track the reason something might suspend in development mode through debug info but this excludes some cases. As a result we can end up with boundary that suspends but has no cause. This tries to detect that and show a notice for why that might be. I'm also trying to make it work with old React versions to cover everything. In production we don't track any of this meta data like `_debugInfo`, `_debugThenable` etc. so after resolution there's no information to take from. Except suspensey images / css which we can track in prod too. We could track lazy component types already. We'd have to add something that tracks after the fact if something used a lazy child, child as a promise, hooks, etc. which doesn't exist today. So that's not backwards compatible and might add some perf/memory cost. However, another strategy is also to try to replay the components after the fact which could be backwards compatible. That's tricky for child position since there's so many rules for how to do that which would have to be replicated. If you're in development you get a different error. Given that we've added instrumentation very recently. If you're on an older development version of React, then you get a different error. Unfortunately I think my feature test is not quite perfect because it's tricky to test for the instrumentation I just added. facebook#34146 So I think for some prereleases that has `_debugOwner` but doesn't have that you'll get a misleading error. Finally, if you're in a modern development environment, the only reason we should have any gaps is because of throw-a-Promise. This will highlight it as missing. We can detect that something threw if a Suspense boundary commits with a RetryCache but since it's a WeakSet we can't look into it to see anything about what it might have been. I don't plan on doing anything to improve this since it would only apply to new versions of React anyway and it's just inherently flawed. So just deprecate it facebook#34032. Note that nothing in here can detect that we suspended Transition. So throwing at the root or in an update won't show that anywhere.
…ook#34220) The theory here is that when we reveal a boundary coming from the server we want to paint that before hydrating it. Hydration gets scheduled in a macrotask with the scheduler but it's in theory possible that it runs before the paint. If that's the case, then the JS that runs before yielding during hydration might slightly delay the paint and we might miss a window to skip the previous paint.
…ltip (facebook#34221) This is intended to be used by various client side resources where the transfer size is interesting to know how it'll perform in various network conditions. Not intended to be added by the server. For now it's only added internally by DevTools itself on img/css but I'll add it from Flight Client too in a follow up. This now shows this as the "transfer size" which is the encoded body size + headers/overhead. Where as the "fileSize" that I add to images is the decoded body size, like what you'd see on disk. This is what Chrome shows so it's less confusing if you compare Network tab and this view.
…onments without file fetching (facebook#34224)
…ddition to useEffect (facebook#34076) ## Summary This is a fix for facebook#34074 ## How did you test this change? I added tests in the eslint package, and ran `yarn jest`. After adding the new tests, I have this: On main | On this branch -|- <img width="356" height="88" alt="image" src="https://github.com/user-attachments/assets/4ae099a1-0156-4032-b2ca-635ebadcaa3f" /> | <img width="435" height="120" alt="image" src="https://github.com/user-attachments/assets/b06c04b8-6cec-43de-befa-a8b4dd20500e" /> ## Changes - Add tests to check that we are checking both `CallExpression` (`useEffect(`), and `MemberExpression` (`React.useEffect(`). To do that, I copied the `getNodeWithoutReactNamespace(` fn from `ExhaustiveDeps.ts` to `RulesOfHooks.ts`
This adds a "suspended by" row for each chunk that is referenced from a client reference. So when you select a client component, you can see what bundles will block that client component when loading on the client. This is only done in the browser build since if we added it on the server, it would show up as a blocking resource and while it's possible we expect that a typical server request won't block on loading JS. <img width="664" height="486" alt="Screenshot 2025-08-17 at 3 45 14 PM" src="https://github.com/user-attachments/assets/b1f83445-2a4e-4470-9a20-7cd215ab0482" /> <img width="745" height="678" alt="Screenshot 2025-08-17 at 3 46 58 PM" src="https://github.com/user-attachments/assets/3558eae1-cf34-4e11-9d0e-02ec076356a4" /> Currently this is only included if it ends up wrapped in a lazy like in the typical type position of a Client Component, but there's a general issue that maybe hard references need to transfer their debug info to the parent which can transfer it to the Fiber.
…ook#34228) While we still use this package internally, we now ask users to install eslint-plugin-react-hooks instead, so this package can now be deprecated on npm.
…t paint (facebook#34230) Before the first rAF, we don't know if there has been other paints before this and if so when. (We could get from performance observer.) We can assume that it's not earlier than 0 so we used delay up until the throttle time starting from zero but if the first paint is about to happen that can be very soon after. Instead, this reveals it during the next paint which should let us be able to get into the first paint. If we can trust `rel="expect"` to have done its thing we should schedule our raf before first paint but ofc browsers can cheat and paint earlier if they want to. If we're wrong, this is at least more batched than doing it synchronously. However it will mean that things might get more flashy than it should be if it would've been throttled. An alternative would be to always throttle first reveal.
…e timeline (facebook#34642) When you double click it will hide or show by jumping to the selected index or one step before the selected. Let's you go from a suspense boundary into the timeline to find its position. I also highlight the step in the timeline when you hover the rect. This only works if it's in the selected root but all of those should be merged into one single timeline. One thing that's weird about the SuspenseNodes now is that they sometimes gets deleted but not always when they're resupended. Nested ones maybe? This means that if you double click to hide it, you can't double click again to show it. This seems like an unrelated bug that we should fix. We could potentially repurpose the existing "Suspend" button in the toolbar to do this too, or maybe add another icon there.
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory. Before submitting a pull request, please make sure the following is done: 1. Fork [the repository](https://github.com/facebook/react) and create your branch from `main`. 2. Run `yarn` in the repository root. 3. If you've fixed a bug or added code that should be tested, add tests! 4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch TestName` is helpful in development. 5. Run `yarn test --prod` to test in the production environment. It supports the same options as `yarn test`. 6. If you need a debugger, run `yarn test --debug --watch TestName`, open `chrome://inspect`, and press "Inspect". 7. Format your code with [prettier](https://github.com/prettier/prettier) (`yarn prettier`). 8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only check changed files. 9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`). 10. If you haven't already, complete the CLA. Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html --> ## Summary <!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? --> Introduced `<ViewTransition>` to the React Compiler Playground. Added an initial animation on the config panel opening/closing to allow for a smoother visual experience. Previously, the panel would flash in and out of the screen upon open/close. ## How did you test this change? <!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes the user interface. How exactly did you verify that your PR solves the issue you wanted to solve? If you leave this empty, your PR will very likely be closed. --> https://github.com/user-attachments/assets/9dc77a6b-d4a5-4a7a-9d81-007ebb55e8d2
<!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory. Before submitting a pull request, please make sure the following is done: 1. Fork [the repository](https://github.com/facebook/react) and create your branch from `main`. 2. Run `yarn` in the repository root. 3. If you've fixed a bug or added code that should be tested, add tests! 4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch TestName` is helpful in development. 5. Run `yarn test --prod` to test in the production environment. It supports the same options as `yarn test`. 6. If you need a debugger, run `yarn test --debug --watch TestName`, open `chrome://inspect`, and press "Inspect". 7. Format your code with [prettier](https://github.com/prettier/prettier) (`yarn prettier`). 8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only check changed files. 9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`). 10. If you haven't already, complete the CLA. Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html --> ## Summary <!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? --> Utilized `<ViewTransition>` to introduce a sliding animation upon switching between the Output and SourceMap tabs in the default playground view. ## How did you test this change? <!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes the user interface. How exactly did you verify that your PR solves the issue you wanted to solve? If you leave this empty, your PR will very likely be closed. --> https://github.com/user-attachments/assets/1ac93482-8104-4f9a-887e-6adca3537dca
This was merged into the 19.1.1 patch release branch in facebook#33972 but we never upstreamed it to main. This should merge to main to make it easier to sync versions to RN after future releases. --------- Co-authored-by: Riccardo Cipolleschi <[email protected]>
## Summary Experimentation has completed for this at Meta and we've observed positive impact on key React Native surfaces. ## How did you test this change? yarn flow fabric
…4653) This brings the Suspense boundary that's switching into view so that when you play the loading sequence you can see how it plays out. Otherwise it's really hard to find where things are changing. This assumes we'll also scroll synchronize the suspense tab which will bring it into view there too.
…he documentElement (facebook#34651) If I can scroll the document due to it overflowing, I should be able to scroll the suspense tab as much. The real rect for the root when it's the document is really the full scroll height. This doesn't fully eliminate the need to do recursive bounding boxes for the root since it's still possible to have the rects overflow. E.g. if they're currently resuspended or inside nested scrolls. ~However, maybe we should have the actual paintable root rect just be this rectangle instead of including the recursive ones.~ Actually never mind. The root really represents the Transition so it doesn't make sense to give it any specific rectangle. It's rather the whole background.
…ted (facebook#34652) We selected the root. This means that we're currently viewing the Transition that rendered the whole screen. In laymans terms this is really "Initial Paint". Once we add subtree selection, then the equivalent should be called "Transition" since in that case it's really about a Transition within the page. So if you've selected an Activity tree this should be called "Transition". Once we add the environment support to the timeline. The first entry on the timeline should also be called "Initial Paint" when you haven't selected an Activity and "Transition" when you have. Technically they're both meant to be "Transition" but nobody thinks of initial load as a "Transition" from the previous MPA page. <img width="1214" height="419" alt="Screenshot 2025-09-29 at 5 18 58 PM" src="https://github.com/user-attachments/assets/cae263e3-133c-4fa9-9587-a7b2344199f4" />
…Offscreen (facebook#34658) Otherwise, when a context is propagated into an Activity (or Suspense) this will leave work behind on the Offscreen component itself. Which will cause an extra unnecessary render and commit pass just to figure out that we're still defering it to idle. This is because lazy context propagation, when calling to schedule some work walks back up the tree all the way to the root. This is usually fine for other nodes since they'll recompute their remaining child lanes on the way up. However, for the Offscreen component we'll have already computed it. We need to set it after propagation to ensure it gets reset.
…ok#34648) We've observed some scenarios, where cascading update happens in an effect that was shorter than 0.05ms. In this case, this effect won't be displayed on a timeline, because of the threshold that we are using, but it would be shown in entry properties or in a stack trace. To avoid confusion, we should always log such effects. Validated via manually changing the threshold to 100ms+ and observing that only effects that triggered an update are visible on a timeline.
…book#34597) <!-- Thanks for submitting a pull request! We appreciate you spending the time to work on these changes. Please provide enough information so that others can review your pull request. The three fields below are mandatory. Before submitting a pull request, please make sure the following is done: 1. Fork [the repository](https://github.com/facebook/react) and create your branch from `main`. 2. Run `yarn` in the repository root. 3. If you've fixed a bug or added code that should be tested, add tests! 4. Ensure the test suite passes (`yarn test`). Tip: `yarn test --watch TestName` is helpful in development. 5. Run `yarn test --prod` to test in the production environment. It supports the same options as `yarn test`. 6. If you need a debugger, run `yarn test --debug --watch TestName`, open `chrome://inspect`, and press "Inspect". 7. Format your code with [prettier](https://github.com/prettier/prettier) (`yarn prettier`). 8. Make sure your code lints (`yarn lint`). Tip: `yarn linc` to only check changed files. 9. Run the [Flow](https://flowtype.org/) type checks (`yarn flow`). 10. If you haven't already, complete the CLA. Learn more about contributing: https://reactjs.org/docs/how-to-contribute.html --> ## Summary <!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? --> Added `<ViewTransition>` for when the "Show Internals" button is toggled for a basic fade transition. Additionally added a transition for when tabs are expanded in the advanced view of the Compiler Playground to display a smoother show/hide animation. ## How did you test this change? <!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes the user interface. How exactly did you verify that your PR solves the issue you wanted to solve? If you leave this empty, your PR will very likely be closed. --> https://github.com/user-attachments/assets/c706b337-289e-488d-8cd7-45ff1d27788d
…ules (facebook#34497) We need to be able to specify additional effect hooks for the RulesOfHooks lint rule in order to allow useEffectEvent to be called by custom effects. ExhaustiveDeps does this with a regex suppplied to the rule, but that regex is not accessible from other rules. This diff introduces a `react-hooks` entry you can put in the eslint settings that allows you to specify custom effect hooks and share them across all rules. This works like: ``` { settings: { 'react-hooks': { additionalEffectHooks: string, }, }, } ``` The next diff allows useEffect to read from the same configuration. ---- --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34497). * facebook#34637 * __->__ facebook#34497
…#34637) Like in the diff below, we can read from the shared configuration to check exhaustive deps. I allow the classic additionalHooks configuration to override it so that this change is backwards compatible. -- --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/facebook/react/pull/34637). * __->__ facebook#34637 * facebook#34497
Stacked on facebook#34637 `useEffectEvent` is now in canary so we need to remove this `__EXPERIMENTAL__` gating on the rules and tests
Temporarily disables the compiler rules in eslint-plugin-react-hooks. Will revert this later.
Reset EventTime when clearing timers. We need to track repeat updates separately. Typically we always reset all timers when we've logged an update. The same update shouldn't be logged again. I was trying to be clever and not reset the XEventTime because we also need the timestamp to know if it's a repeat event. However, because of this it looked like we had an event schedule an update even after we had reset them. This always resets the XEventTime to -1.1 and then stashes the old time on EventRepeatTime which is our indication whether the next update was a repeat of the old event. --------- Co-authored-by: Ruslan Lesiutin <[email protected]> Co-authored-by: Ricky <[email protected]>
Co-authored-by: Ruslan Lesiutin <[email protected]>
…ofiling mode (facebook#34667) We need this to be able to log the renders that happened inside. This is the same thing we do here but for the offscreen special cases: https://github.com/facebook/react/blob/main/packages/react-reconciler/src/ReactFiberCommitWork.js#L3452-L3457
Called Before: > `logEvent` is a function created with React Hook "useEffectEvent", and can only be called from the same component. Called After: > `logEvent` is a function created with React Hook "useEffectEvent", and can only be called from Effects and Effect Events in the same component. Referenced Before: > `logEvent` is a function created with React Hook "useEffectEvent", and can only be called from the same component. They cannot be assigned to variables or passed down. Referenced After: > `logEvent` is a function created with React Hook "useEffectEvent", and can only be called from Effects and Effect Events in the same component. It cannot be assigned to a variable or passed down.
Co-authored-by: Jack Pope <[email protected]> Co-authored-by: Rick Hanlon <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
24 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Mirrored from facebook/react PR facebook#34718