Adds a way to configure how we insert deps for experimental purposes.
```
[
{
module: 'react',
imported: 'useEffect',
numRequiredArgs: 1,
},
{
module: 'MyExperimentalEffectHooks',
imported: 'useExperimentalEffect',
numRequiredArgs: 2,
},
]
```
would insert dependencies for calls of `useEffect` imported from `react`
if they have 1 argument and calls of useExperimentalEffect` from
`MyExperimentalEffectHooks` if they have 2 arguments. The pushed dep
array is appended to the arg list.
We didn't originally support holes within array patterns, so DCE was
only able to prune unused items from the end of an array pattern. Now
that we support holes we can replace any unused item with a hole, and
then just prune the items to the last identifier/spread entry.
Note: this was motivated by finding useState where either the state or
setState go unused — both are strong indications that you're violating
the rules in some way. By DCE-ing the unused portions of the useState
destructuring we can easily check if you're ignoring either value.
closes#31603
This is a redo of that PR not using ghstack
A long standing issue for React has been that if you reorder stateful
nodes, they may lose their state and reload. The thing moving loses its
state. There's no way to solve this in general where two stateful nodes
swap.
The [`moveBefore()`
proposal](https://chromestatus.com/feature/5135990159835136?gate=5177450351558656)
has now moved to
[intent-to-ship](https://groups.google.com/a/chromium.org/g/blink-dev/c/YE_xLH6MkRs/m/_7CD0NYMAAAJ).
This function is kind of like `insertBefore` but preserves state.
There's [a demo here](https://state-preserving-atomic-move.glitch.me/).
Ideally we'd port this demo to a fixture so we can try it.
Currently this flag is always off - even in experimental. That's because
this is still behind a Chrome flag so it's a little early to turn it on
even in experimental. So you need a custom build. It's on in RN but only
because it doesn't apply there which makes it easier to tell that it's
safe to ship once it's on everywhere else.
The other reason it's still off is because there's currently a semantic
breaking change. `moveBefore()` errors if both nodes are disconnected.
That happens if we're inside a completely disconnected React root.
That's not usually how you should use React because it means effects
can't read layout etc. However, it is currently supported. To handle
this we'd have to try/catch the `moveBefore` to handle this case but we
hope this semantic will change before it ships. Before we turn this on
in experimental we either have to wait for the implementation to not
error in the disconnected-disconnected case in Chrome or we'd have to
add try/catch.
This is a hack that ensures that all four lanes as visible whether you
have any tracks in them or not, and that they're in the priority order
within the Scheduler track group. We do want to show all even if they're
not used because it shows what options you're missing out on.
<img width="1035" alt="Screenshot 2024-11-22 at 12 38 30 PM"
src="https://github.com/user-attachments/assets/f30ab0b9-af5e-48ed-b042-138444352575">
In Chrome, the order of tracks within a group are determined by the
earliest start time. We add fake markers at start time zero in that
order eagerly. Ideally we could do this only once but because calls that
aren't recorded aren't considered for ordering purposes, we need to keep
adding these over and over again in case recording has just started. We
can't tell when recording starts.
Currently performance.mark() are in first insertion order but
performance.measure() are in the reverse order. I'm not sure that's
intentional. We can always add the 0 time slot even if it's in the past.
That's still considered for ordering purposes as long as the measurement
is recorded at the time we call it.
This is for researching/prototyping, not a feature we are releasing
imminently.
Putting up an early version of inferring effect dependencies to get
feedback on the approach. We do not plan to ship this as-is, and may not
start by going after direct `useEffect` calls. Until we make that
decision, the heuristic I use to detect when to insert effect deps will
suffice for testing.
The approach is simple: when we see a useEffect call with no dep array
we insert the deps inferred for the lambda passed in. If the first
argument is not a lambda then we do not do anything.
This diff is the easy part. I think the harder part will be ensuring
that we can infer the deps even when we have to bail out of memoization.
We have no other features that *must* run regardless of rules of react
violations. Does anyone foresee any issues using the compiler passes to
infer reactive deps when there may be violations?
I have a few questions:
1. Will there ever be more than one instruction in a block containing a
useEffect? if no, I can get rid of the`addedInstrs` variable that I use
to make sure I insert the effect deps array temp creation at the right
spot.
2. Are there any cases for resolving the first argument beyond just
looking at the lvalue's identifier id that I'll need to take into
account? e.g., do I need to recursively resolve certain bindings?
---------
Co-authored-by: Mofei Zhang <feifei0@meta.com>
This ensures that we mark the time from ping until we render as
"Blocked".
We intentionally don't want to show the event time even if it's
something like "load" because it draws attention away from interactions
etc.
<img width="577" alt="Screenshot 2024-11-21 at 7 22 39 PM"
src="https://github.com/user-attachments/assets/70cca2e8-bd5e-489f-98f0-b4dfee5940af">
This avoid re-emitting the yellow "Event" log when we ping inside the
original event. Instead of treating events as repeated when we get
repeated updates, we treat them as repeated if we've ever logged out
this event before.
Additionally, in the case the prerender sibling flag is on we need to
ensure that if a render gets interrupted when it has been suspended we
treat that as "Prewarm" instead of "Interrupted Render".
Before:
<img width="539" alt="Screenshot 2024-11-19 at 2 39 44 PM"
src="https://github.com/user-attachments/assets/190ca50c-5168-40d8-a6fd-6b9a583af1f0">
After:
<img width="1004" alt="Screenshot 2024-11-21 at 4 53 16 PM"
src="https://github.com/user-attachments/assets/0c441ada-1ed1-412c-8935-aaf040c25dfe">
Fixes a bug with the experimental `useResourceEffect` hook where we
would compare the wrong deps when there happened to be another kind of
effect preceding the ResourceEffect. To do this correctly we need to add
a pointer to the ResourceEffect's identity on the update.
I also unified the previously separate push effect impls for resource
effects since they are always pushed together as a unit.
Stacked on #31552. Must be tested with `enableSiblingPrerendering` off
since the `use()` optimization is not on there yet.
This adds a span to the Components track when we yield in the middle of
the event loop. In this scenario, the "Render" span continues through
out the Scheduler track. So you can see that the Component itself might
not take a long time but yielding inside of it might.
This lets you see if something was blocking the React render loop while
yielding. If we're blocked 1ms or longer we log that as "Blocked".
If we're yielding due to suspending in the middle of the work loop we
log this as "Suspended".
<img width="837" alt="Screenshot 2024-11-16 at 1 15 14 PM"
src="https://github.com/user-attachments/assets/45a858ea-17e6-416c-af1a-78c126e033f3">
If the render doesn't commit because it restarts due to some other
prewarming or because some non-`use()` suspends, it doesn't have from
context components.
<img width="971" alt="Screenshot 2024-11-16 at 1 13 55 PM"
src="https://github.com/user-attachments/assets/a67724f8-702e-4e7d-9499-9ffc09541a61">
The `useActionState` path doesn't work yet because the `use()`
optimization doesn't work there for some reason. But the idea is that it
should mark the time that the component is blocked as Action instead of
Suspended.
When we suspend the render with delay, we won't do any more work until
we get some kind of another update/ping. It's because conceptually
something is suspended and then will update later. We need to highlight
this period to show why it's not doing any work. We fill the empty space
with "Suspended". This stops whenever the same lane group starts
rendering again. Clamped by the preceeding start time/event time/update
time.
<img width="902" alt="Screenshot 2024-11-15 at 1 01 29 PM"
src="https://github.com/user-attachments/assets/acf9dc9a-8fc3-4367-a8b0-d19f9c9eac73">
Ideally we would instead start the next render and suspend the work loop
at all places we suspend. In that mode this will instead show up as a
very long "Render" with a "Suspended" period instead highlighted in the
Components track as one component is suspended. We'll soon have that for
`use()` but not all updates so this covers the rest.
One issue with `useActionState` is that it is implemented as suspending
at the point of the `useActionState` which means that the period of the
Action shows up as a suspended render instead of as an Action which
happens for raw actions. This is not really how you conceptually think
about it so we need some special case for `useActionState`. In the
screenshot above, the first "Suspended" is actually awaiting an Action
and the second "Suspended" is awaiting the data from it.
```
=> Found "hermes-parser@0.25.1"
info Reasons this module exists
- "_project_#prettier-plugin-hermes-parser" depends on it
- Hoisted from "_project_#prettier-plugin-hermes-parser#hermes-parser"
- Hoisted from "_project_#eslint-plugin-react-compiler#hermes-parser"
- Hoisted from "_project_#snap#hermes-parser"
- Hoisted from "_project_#snap#babel-plugin-syntax-hermes-parser#hermes-parser"
- Hoisted from "_project_#eslint-plugin-react-compiler#hermes-eslint#hermes-parser"
info Disk size without dependencies: "1.49MB"
info Disk size with unique dependencies: "1.82MB"
info Disk size with transitive dependencies: "1.82MB"
info Number of shared dependencies: 1
✨ Done in 0.81s.
```
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31586).
* __->__ #31586
* #31585
```
=> Found "react@0.0.0-experimental-4beb1fd8-20241118"
info Reasons this module exists
- "_project_#babel-plugin-react-compiler" depends on it
- Hoisted from "_project_#babel-plugin-react-compiler#react"
- Hoisted from "_project_#snap#react"
info Disk size without dependencies: "252KB"
info Disk size with unique dependencies: "252KB"
info Disk size with transitive dependencies: "252KB"
info Number of shared dependencies: 0
✨ Done in 0.60s.
```
```
=> Found "react-dom@0.0.0-experimental-4beb1fd8-20241118"
info Reasons this module exists
- "_project_#babel-plugin-react-compiler" depends on it
- Hoisted from "_project_#babel-plugin-react-compiler#react-dom"
- Hoisted from "_project_#snap#react-dom"
info Disk size without dependencies: "8.04MB"
info Disk size with unique dependencies: "8.17MB"
info Disk size with transitive dependencies: "8.17MB"
info Number of shared dependencies: 1
✨ Done in 0.56s.
```
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31585).
* #31586
* __->__ #31585
Our e2e setup with monaco is kinda brittle since it relies on the dom.
It seems like longish text gets truncated so let's just simpify all
these test cases.
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31573).
* __->__ #31573
* #31572
Since `enableRefAsProp` shipped everywhere, the ReactElement
implementation on prod puts refs on both `element.ref` and
`element.props.ref`. Here we let the `ref` case fall through so its now
available on props, matching the JSX runtime.
This PR introduces a new experimental hook `useResourceEffect`, which is
something that we're doing some very early initial tests on.
This may likely not pan out and will be removed or modified if so.
Please do not rely on it as it will break.
This lets us track separately if something was suspended on an Action
using useActionState rather than suspended on Data.
This approach feels quite bloated and it seems like we'd eventually
might want to read more information about the Promise that suspended and
the context it suspended in. As a more general reason for suspending.
The way useActionState works in combination with the prewarming is quite
unfortunate because 1) it renders blocking to update the isPending flag
whether you use it or not 2) it prewarms and suspends the useActionState
3) then it does another third render to get back into the useActionState
position again.
Now that we rely on function context exclusively, let's clean up
`HIRFunction.context` after DCE. This PR is in preparation of #31204,
which would otherwise have unnecessary declarations (of context values
that become entirely DCE'd)
'
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31202).
* __->__ #31202
* #31203
* #31201
* #31200
* #31521
`JSXMemberExpression` is currently the only instruction (that I know of)
that directly references identifier lvalues without a corresponding
`LoadLocal`.
This has some side effects:
- deadcode elimination and constant propagation now reach
JSXMemberExpressions
- we can delete `LoweredFunction.dependencies` without dangling
references (previously, the only reference to JSXMemberExpression
objects in HIR was in function dependencies)
- JSXMemberExpression now is consistent with all other instructions
(e.g. has a rvalue-producing LoadLocal)
'
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31201).
* #31202
* #31203
* __->__ #31201
* #31200
* #31521
Recursively visit inner function instructions to extract dependencies
instead of using `LoweredFunction.dependencies` directly.
This is currently gated by enableFunctionDependencyRewrite, which needs
to be removed before we delete `LoweredFunction.dependencies` altogether
(#31204).
Some nice side effects
- optional-chaining deps for inner functions
- full DCE and outlining for inner functions (see #31202)
- fewer extraneous instructions (see #31204)
-
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31200).
* #31202
* #31203
* #31201
* __->__ #31200
* #31521
We were previously filtering out `ref.current` dependencies in
propagateScopeDependencies:checkValidDependency`. This is incorrect.
Instead, we now always take a dependency on ref values (the outer box)
as they may be reactive. Pruning is done in
pruneNonReactiveDependencies.
This PR includes a small patch to `collectReactiveIdentifier`. Prior to
this, we conservatively assumed that pruned scopes always produced
reactive declarations. This assumption fixed a bug with non-reactivity,
but some of these declarations are `useRef` calls. Now we have special
handling for this case
```js
// This often produces a pruned scope
React.useRef(1);
```
---
[//]: # (BEGIN SAPLING FOOTER)
Stack created with [Sapling](https://sapling-scm.com). Best reviewed
with [ReviewStack](https://reviewstack.dev/facebook/react/pull/31521).
* #31202
* #31203
* #31201
* #31200
* __->__ #31521
This includes:
- `Interrupted Render`: Interrupted Renders (setState or ping at higher
priority)
- `Prewarm`: Suspended Renders outside a Suspense boundary
(RootSuspendedWithDelay/RootSuspendedAtTheShell)
- `Errored Render`: Render that errored somewhere in the tree (Fatal or
Not) (which may or may not be retried and then complete)
- `Teared Render`: Due to useSyncExternalStore not matching (which will
do another sync attempt)
Suspended Commit:
<img width="893" alt="Screenshot 2024-11-14 at 11 47 40 PM"
src="https://github.com/user-attachments/assets/b25a6a8b-a5e9-4d66-b325-57aef4bf9dad">
Errored with a second recovery attempt that also errors:
<img width="976" alt="Screenshot 2024-11-15 at 12 09 06 AM"
src="https://github.com/user-attachments/assets/9ce52cbb-b587-4f1e-8b67-e51d9073ae5b">
## Summary
This fixes a typo in the error that gets reported when Float errors
while hoisting a style tag that does not contain both `precedence` and
`href`. There was a typo in _conflict_ and the last part of the sentence
doesn't make sense. I assume it wasn't needed since the message already
suggests moving the style tag to the head manually.
It's useful to quickly see where new events are kicking off new
rendering. This uses the new "warning" color (yellow) to do that. This
is to help distinguish it from the purple (secondary color) which is
used for the commit phase which is more of a follow up and it's often
that you have several rerenders within one event which makes it hard to
tell a part where it starts and event otherwise.
For the span marking between previous render within the same event and
the next setState, I use secondary-light (light purple) since it's kind
of still part of the same sequence at that point. It's usually a spawned
render (e.g. setState in useEffect or microtask) but it can also be
sequential flushSync.
I was bothered by that the event name is the only thing that's lower
case so I prefixed it with `Event: ` like the JS traces are.
<img width="1499" alt="Screenshot 2024-11-13 at 7 15 45 PM"
src="https://github.com/user-attachments/assets/0c81c810-6b5d-4fc7-9bc0-d15b53844ade">
It might be a little confusing why our track starts earlier than the JS
one below in the "Main Thread" flamegraph which looks the same. That's
because ours is the start of the event time which is when the click
happens where as the Main Thread one is when the JS event loop gets
around to processing the event.
When you schedule a microtask from render or effect and then call
setState (or ping) from there, the "event" is the event that React
scheduled (which will be a postMessage). The event time of this new
render will be before the last render finished.
We usually clamp these but in this scenario the update doesn't happen
while a render is happening. Causing overlapping events.
Before:
<img width="1229" alt="Screenshot 2024-11-12 at 11 01 30 PM"
src="https://github.com/user-attachments/assets/9652cf3b-b358-453c-b295-1239cbb15952">
Therefore when we finalize a render we need to store the end of the last
render so when we a new update comes in later with an event time earlier
than that, we know to clamp it.
There's also a special case here where when we enter the
`RootDidNotComplete` or `RootSuspendedWithDelay` case we neither leave
the root as in progress nor commit it. Those needs to finalize too.
Really this should be modeled as a suspended track that we haven't added
yet. That's the gap between "Blocked" and "message" below.
After:
<img width="1471" alt="Screenshot 2024-11-13 at 12 31 34 AM"
src="https://github.com/user-attachments/assets/b24f994e-9055-4b10-ad29-ad9b36302ffc">
I also fixed an issue where we may log the same event name multiple
times if we're rendering more than once in the same event. In this case
I just leave a blank trace between the last commit and the next update.
I also adding ignoring of the "message" event at all in these cases when
the event is from React's scheduling itself.
Fixes a bug.
We're supposed to not log "Waiting for Paint" if the passive effect
phase was forced since we weren't really waiting until the paint.
Instead we just log an empty string when we force it to still ensure
continuity.
We should always log the passive phase. This check was in the wrong
place.
In order to make use of the compiler in stable releases (eg React 19 RC,
canary), we need to export the compiler runtime in the stable channel as
well.
## Summary
`@rollup/plugin-typescript` emits a warning while building, hinting that
`outputToFilesystem` defaults to true.
Although "noEmit" is set to `true` for the tsconfig, rollup writes a
`dist/.tsbuildinfo`. That file is then also shipped inside the npm
module and doesn't offer any benefit for library consumers. Setting this
option to false results in the file not being written and thus omitted
from the npm module.
## How did you test this change?
`dist/.tsbuildinfo` is not emitted any more.
Previously we were showing Components inside each lane track but that
meant that as soon as you expanded a lane you couldn't see the other
line so you couldn't get an overview over how well things were
scheduled.
This instead moves all the Components into a single top-level track and
renames the previous one to a "Scheduler" track group.
<img width="1352" alt="Screenshot 2024-11-12 at 8 26 05 PM"
src="https://github.com/user-attachments/assets/590bc6d3-3540-4ee4-b474-5d733b8d8d8d">
That way you can get an overview over what React is working on first and
then right below see which Component is being worked on.
Ideally the "Scheduler" track would be always expanded since each Track
is always just a single row. Now you have to expand each lane to see the
labels but then you're wasting a lot of vertical real estate. There's
currently no option to create this with the Chrome performance.measure
extensions.
<img width="1277" alt="Screenshot 2024-11-12 at 8 26 16 PM"
src="https://github.com/user-attachments/assets/4fc39e35-10ec-4452-ad32-c1c2e6b5e1a8">
In preparation for the next RC, I set this feature flag to true
everywhere. I did not delete the feature flag yet, in case there are yet
more bugs to be discovered.
I also didn't remove the dynamic feature flag from the Meta builds; I'll
let the Meta folks handle that.
Our CI workflows generally cache `**/node_modules` (note the glob, it
caches all transitive node_module directories) to speed up startup for
new jobs that don't change any dependencies. However it seems like one
of our caches got into a weird state (not sure how it happened) where
the `build` directory (used in various other scripts as the directory
for compiled React packages) would contain a `node_modules` directory as
well. This made sizebot size change messages very big since it would try
to compare every single file in `build/node_modules`.
The fix is to ensure we always clean the `build` directory before doing
anything with it. We can also delete that one problematic cache but this
PR is a little more resilient to other weird behavior with that
directory.
This provides less context but skips a lot of noise.
Previously we were including parent components to provide context about
what is rendering but this turns out to be:
1) Very expensive due to the overhead of `performance.measure()` while
profiling.
2) Unactionable noise in the profile that hurt more than it added in
real apps with large trees.
This approach instead just add performance.measure calls for each
component that was marked as PerformedWork (which was used for this
purpose by React Profiler) or had any Effects.
Not everything gets marked with PerformedWork though. E.g. DOM nodes do
not but they can have significant render times since creating them takes
time. We might consider including them if a self-time threshold is met.
Because there is little to no context about the component anymore it
becomes really essential to get a feature from Chrome DevTools that can
link to something with more context like React DevTools.
This reverts commit d3bf32a958 which was
part of #30983
When you have very deep trees this trick can cause the top levels to
skew way too much from the real numbers. Creating unbalanced trees.
The bug should have been fixed in Chrome Canary now so that entries
added later are sorted to go first which should've addressed this issue.
Jordan Brown
lauren
Joseph Savona
Sebastian Markbåge
Mark Skelton
Aditya Subramanyam
mofeiZ
Jack Pope
Andrew Clark
Zack Tanner
Niklas Mollenhauer