Per title, implements an HIR-based version of FlattenScopesWithHooksOrUse as part of our push to use HIR everywhere. This is the last pass to migrate before PropagateScopeDeps, which is blocking the fix for `bug.invalid-hoisting-functionexpr`, ie where we can infer incorrect dependencies for function expressions if the dependencies are accessed conditionally.
ghstack-source-id: 05c6e26b3b
Pull Request resolved: https://github.com/facebook/react/pull/29840
Pre the title, this implements an HIR-based version of FlattenReactiveLoops. Another step on the way to HIR-everywhere.
ghstack-source-id: e1d166352d
Pull Request resolved: https://github.com/facebook/react/pull/29838
The ci step for the playground already installs playwright browsers so
this step was unnecessary. It also doesn't work internally for our sync
scripts
ghstack-source-id: d6e7615637
Pull Request resolved: https://github.com/facebook/react/pull/29841
<!--
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?
-->
The parsePluginOptions seemed to be duplicated within
[BabelPlugin.ts](https://github.com/facebook/react/blob/f5af92d2c47d1e1f455faf912b1d3221d1038c37/compiler/packages/babel-plugin-react-compiler/src/Babel/BabelPlugin.ts#L32)
and
[Program.ts](https://github.com/facebook/react/blob/f5af92d2c47d1e1f455faf912b1d3221d1038c37/compiler/packages/babel-plugin-react-compiler/src/Entrypoint/Program.ts#L220).
Since the options already parsed in BabelPlugin.ts should have been
passed to compileProgram, in this PR we deleted parsePluginOptions in
compileProgram and used the options passed as arguments as they are.
I've done that.
## 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.
-->
<img width="516" alt="image"
src="https://github.com/facebook/react/assets/87469023/2a70c6ea-0330-42a2-adff-48ae3e905790">
Basically make `console.error` and `console.warn` behave like normal -
when a component stack isn't appended. I need this because I need to be
able to print rich logs with the component stack option and to be able
to disable instrumentation completely in `console.createTask`
environments that don't need it.
Currently we can't print logs with richer objects because they're
toString:ed first. In practice, pretty much all arguments we log are
already toString:ed so it's not necessary anyway. Some might be like a
number. So it would only be a problem if some environment can't handle
proper consoles but then it's up to that environment to toString it
before logging.
The `Warning: ` prefix is historic and is both noisy and confusing. It's
mostly unnecessary since the UI surrounding `console.error` and
`console.warn` tend to have visual treatment around it anyway. However,
it's actively misleading when `console.error` gets prefixed with a
Warning that we consider an error level. There's an argument to be made
that some of our `console.error` don't make the bar for an error but
then the argument is to downgrade each of those to `console.warn` - not
to brand all our actual error logging with `Warning: `.
Apparently something needs to change in React Native before landing this
because it depends on the prefix somehow which probably doesn't make
sense already.
## Overview
Updates `eslint-plugin-jest` and enables the recommended rules with some
turned off that are unhelpful.
The main motivations is:
a) we have a few duplicated tests, which this found an I deleted
b) making sure we don't accidentally commit skipped tests
Adds a shape type for component props, which has one defined property: "ref". This means that if the ref property exists, we can type usage of `props.ref` (or via destructuring) the same as the result of `useRef()` and infer downstream usage similarly.
ghstack-source-id: 76cd07c5df
Pull Request resolved: https://github.com/facebook/react/pull/29834
## Overview
The clever trick in https://github.com/facebook/react/pull/29799 turns
out to not work because signedsource includes the generated hash in the
header. Reverts back to checking git diff, filtering out the REVISION
file and `@generated` headers.
There are two cases where it's legit/intended to remove scopes, and we can inline the scope rather than reify a "pruned" scope:
* Scopes that contain a single instruction with a hook call. The fact that we create a scope in this case at all is just an artifact of it being simpler to do this and remove the scope later rather than try to avoid creating it in the first place. So for these scopes, we can just inline them.
* Scopes that are provably non-escaping. Removing the scope is an optimization, not a case of us having to prune away something that should be there. So again, its fine to inline in this case.
I found this from syncing the stack internally and looking at differences in compiled output. The latter case was most common but the first case is just an obvious improvement.
ghstack-source-id: 80610ddafa
Pull Request resolved: https://github.com/facebook/react/pull/29820
Adds additional information to the CompileSuccess LoggerEvent:
* `prunedMemoBlocks` is the number of reactive scopes that were pruned for some reason.
* `prunedMemoValues` is the number of unique _values_ produced by those scopes.
Both numbers exclude blocks that are just a hook call - ie although we create and prune a scope for eg `useState()`, that's just an artifact of the sequencing of our pipeline. So what this metric is counting is cases of _other_ values that go unmemoized. See the new fixture, which takes advantage of improvements in the snap runner to optionally emit the logger events in the .expect.md file if you include the "logger" pragma in a fixture.
ghstack-source-id: c2015bb556
Pull Request resolved: https://github.com/facebook/react/pull/29810
Mostly addresses the issue with non-reactive pruned scopes. Before, values from pruned scopes would not be memoized, but could still be depended upon by downstream scopes. However, those downstream scopes would assume the value could never change. This could allow the developer to observe two different versions of a value - the freshly created one (if observed outside a scope) or a cached one (if observed inside, or through) a scope which used the value but didn't depend on it.
The fix here is to consider the outputs of pruned reactive scopes as reactive. Note that this is a partial fix because of things like control variables — the full solution would be to mark these values as reactive, and then re-run InferReactivePlaces. We can do this once we've fully converted our pipeline to use HIR everywhere. For now, this should fix most issues in practice because PruneNonReactiveDependencies already does basic alias tracking (see new fixture).
ghstack-source-id: 364430bbec
Pull Request resolved: https://github.com/facebook/react/pull/29790
There's a category of bug currently where pruned reactive scopes whose outputs are non-reactive can have their code end up inlining into another scope, moving the location of the instruction. Any value that had a scope assigned has to have its order of evaluation preserved, despite the fact that it got pruned, so naively we could just force every pruned scope to have its declarations promoted to named variables.
However, that ends up assigning names to _tons_ of scope declarations that don't really need to be promoted. For example, a scope with just a hook call ends up with:
```
const x = useFoo();
=>
scope {
$t0 = Call read useFoo$ (...);
}
$t1 = StoreLocal 'x' = read $t0;
```
Where t0 doesn't need to be promoted since it's used immediately to assign to another value which is a non-temporary.
So the idea of this PR is that we can track outputs of pruned scopes which are directly referenced from inside a later scope. This fixes one of the two cases of the above pattern. We'll also likely have to consider values from pruned scopes as always reactive, i'll do that in the next PR.
ghstack-source-id: b37fb9a7cb
Pull Request resolved: https://github.com/facebook/react/pull/29789
There are a few places where we want to check whether a value actually got memoized, and we currently have to infer this based on values that "should" have a scope and whether a corresponding scope actually exists. This PR adds a new ReactiveStatement variant to model a reactive scope block that was pruned for some reason, and updates all the passes that prune scopes to instead produce this new variant.
ghstack-source-id: aea6dab469
Pull Request resolved: https://github.com/facebook/react/pull/29781
## Overview
Reverts https://github.com/facebook/react/pull/26616 and implements the
suggested way instead.
This change in #26616 broken the internal sync command, which now
results in duplicated `@generated` headers. It also makes it harder to
detect changes during the diff train sync. Instead, we will check for
changes, and if there are changes sign the files and commit them to the
sync branch.
## Strategy
The new sync strategy accounts for the generated headers during the
sync:
- **Revert Version**: Revert the version strings
- **Revert @generated**: Re-sign the files (will be the same hash as
before if unchanged)
- **Check**: Check if there are changes **if not, skip**
- **Re-apply Version**: Now add back the new version string
- **Re-sign @generated**: And re-generate the headers
Then commit to branch. This ensures that if there are no changes, we'll
skip.
---------
Co-authored-by: Timothy Yung <yungsters@gmail.com>
## Summary
See #29737
## How did you test this change?
As the feature requires module support and the test runner does
currently not support running tests as modules, I could only test it via
playground.
The goal is to improve speed of the development by inlining and DCE
unused branches.
We have the ability to preserve some variable names and pretty print in
the production version so might as well do the same with DEV.
When we made stylesheets suspend even during high priority updates we
exposed a bug in the loading tracking of stylesheets that are loaded as
part of the preamble. This allowed these stylesheets to put suspense
boundaries into fallback mode more often than expected because cases
where a stylesheet was server rendered could now cause a fallback to
trigger which was never intended to happen.
This fix updates resource construction to evaluate whether the instance
exists in the DOM prior to construction and if so marks the resource as
loaded and inserted.
One ambiguity that needed to be solved still is how to tell whether a
stylesheet rendered as part of a late Suspense boundary reveal is
already loaded. I updated the instruction to clear out the loading
promise after successfully loading. This is useful because later if we
encounter this same resource again we can avoid the microtask if it is
already loaded. It also means that we can concretely understand that if
a stylesheet is in the DOM without this marker then it must have loaded
(or errored) already.
This PR makes it so we always emit a const VariableDeclaration for
compiled functions in gating mode. If the original declaration's parent
was an ExportDefaultDeclaration we'll also append a new
ExportDefaultDeclaration pointing to the new identifier. This allows
code that adds optional properties to the function declaration to still
work in gating mode
ghstack-source-id: 5705479135
Pull Request resolved: https://github.com/facebook/react/pull/29806
When gating is enabled, any function declaration properties that were
previously set (typically `Function.displayName`) would cause a crash
after compilation as the original identifier is no longer present.
ghstack-source-id: beb7e25856
Pull Request resolved: https://github.com/facebook/react/pull/29802
This refactors key warning to happen inline after we've matched a Fiber.
I didn't want to do that originally because it was riskier. But it turns
out to be straightforward enough.
This lets us use that Fiber as the source of the warning which matters
to DevTools because then DevTools can associate it with the right
component after it mounts.
We can also associate the duplicate key warning with this Fiber. That
way we'll get the callsite with the duplicate key on the stack and can
associate this warning with the child that had the duplicate.
I kept the forked DevTools tests because the warning now is counted on
the Child instead of the Parent (18 behavior).
However, this won't be released in 19.0.0 so I only test this in
whatever the next version is.
Doesn't seem worth it to have a test for just the 19.0.0 behavior.
Fixes false positives where we currently disallow mutations of refs from callbacks passed to JSX, if the ref is also passed to jsx. We consider these to be mutations of "frozen" values, but refs are explicitly allowed to have interior mutability. The fix is to always allow (at leat within InferReferenceEffects) for refs to be mutated. This means we completely rely on ValidateNoRefAccessInRender to validate ref access and stop reporting false positives.
ghstack-source-id: 1a30609f5f
Pull Request resolved: https://github.com/facebook/react/pull/29733
Stacked on #29491
Previously if you aborted during a render the currently rendering task
would itself be aborted which will cause the entire model to be replaced
by the aborted error rather than just the slot currently being rendered.
This change updates the abort logic to mark currently rendering tasks as
aborted but allowing the current render to emit a partially serialized
model with an error reference in place of the current model.
The intent is to support aborting from rendering synchronously, in
microtasks (after an await or in a .then) and in lazy initializers. We
don't specifically support aborting from things like proxies that might
be triggered during serialization of props
## Summary
The test started to fail after
https://github.com/facebook/react/pull/29088.
Fork the test and the expected store state for:
- React 18.x, to represent the previous behavior
- React >= 19, to represent the current RDT behavior, where error can't
be connected to the fiber, because it was not yet mounted and shared
with DevTools.
Ideally, DevTools should start keeping track of such fibers, but also
distinguish them from some that haven't mounted due to Suspense or error
boundaries.
Stacked on #29551
Flight pings much more often than Fizz because async function components
will always take at least a microtask to resolve . Rather than
scheduling this work as a new macrotask Flight now schedules pings in a
microtask. This allows more microtasks to ping before actually doing a
work flush but doesn't force the vm to spin up a new task which is quite
common give n the nature of Server Components
While most builds of Flight and Fizz schedule work in new tasks some do
execute work synchronously. While this is necessary for legacy APIs like
renderToString for modern APIs there really isn't a great reason to do
this synchronously.
We could schedule works as microtasks but we actually want to yield so
the runtime can run events and other things that will unblock additional
work before starting the next work loop.
This change updates all non-legacy uses to be async using the best
availalble macrotask scheduler.
Browser now uses postMessage
Bun uses setTimeout because while it also supports setImmediate the
scheduling is not as eager as the same API in node
the FB build also uses setTimeout
This change required a number of changes to tests which were utilizing
the sync nature of work in the Browser builds to avoid having to manage
timers and tasks. I added a patch to install MessageChannel which is
required by the browser builds and made this patched version integrate
with the Scheduler mock. This way we can effectively use `act` to flush
flight and fizz work similar to how we do this on the client.
Summary
The dispatch function from useReducer is stable, so it is also non-reactive.
the related PR: #29665
the related comment: #29674 (comment)
I am not sure if the location of the new test file is appropriate😅.
How did you test this change?
Added the specific test compiler/packages/babel-plugin-react-compiler/src/__tests__/fixtures/compiler/useReducer-returned-dispatcher-is-non-reactive.expect.md.
## Summary
There was an attempt to upgrade `ip` to 2.0.1 to mitigate CVE in
https://github.com/facebook/react/pull/29725#issuecomment-2150389616,
but there actually another one CVE in version `2.0.1`. Instead, migrate
to `internal-ip`, which similarly small package that we can use
Note: not upgrading to version 7+, because they are pure ESM.
## How did you test this change?
Validated that standalone version of RDT works and connects to the app.
## Summary
We currently do deep diffing for object props, and also use custom
differs, if they are defined, for props with custom attribute config.
The idea is to simply do a `===` comparison instead of all that work. We
will do less computation on the JS side, but send more data to native.
The hypothesis is that this change should be neutral in terms of
performance. If that's the case, we'll be able to get rid of custom
differs, and be one step closer to deleting view configs.
This PR adds the `enableShallowPropDiffing` feature flag to support this
experiment.
## How did you test this change?
With `enableShallowPropDiffing` hardcoded to `true`:
```
yarn test packages/react-native-renderer
```
This fails on the following test cases:
- should use the diff attribute
- should do deep diffs of Objects by default
- should skip deeply-nested changed functions
Which makes sense with this change. These test cases should be deleted
if the experiment is shipped.
This lets us ensure that we use the original V8 format and it lets us
skip source mapping. Source mapping every call can be expensive since we
do it eagerly for server components even if an error doesn't happen.
In the case of an error being thrown we don't actually always do this in
practice because if a try/catch before us touches it or if something in
onError touches it (which the default console.error does), it has
already been initialized. So we have to be resilient to thrown errors
having other formats.
These are not as perf sensitive since something actually threw but if
you want better perf in these cases, you can simply do something like
`onError(error) { console.error(error.message) }` instead.
The server has to be aware whether it's looking up original or compiled
output. I currently use the file:// check to determine if it's referring
to a source mapped file or compiled file in the fixture. A bundled app
can more easily check if it's a bundle or not.
Normally we take the renderClientElement path but this is an internal
fast path.
No tests because we don't run tests with console.createTask (which is
not easy since we test component stacks).
Ideally this would be covered by types but since the types don't
consider flags and DEV it doesn't really help.
## Overview
We didn't have any tests that ran in persistent mode with the xplat
feature flags (for either variant).
As a result, invalid test gating like in
https://github.com/facebook/react/pull/29664 were not caught.
This PR adds test flavors for `ReactFeatureFlag-native-fb.js` in both
variants.
<!--
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
Remove `startTransition` and `useActionState` from `react-server`
condition of react, as they should only stay in client bundle.
This will reduce the server bundle of react itself.
Found this while tracing where the `process.emit` was called.
<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->
## 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.
-->
Following the instructions in the compiler/docs/DEVELOPMENT_GUIDE.md, we are stuck on the command `yarn snap --watch` because it calls readTestFilter even though the filter option is not enabled.
Joe Savona
Vitali Zaidman
Lauren Tan
Yuto Yoshino
Sebastian Markbåge
Ricky
Rick Hanlon
Niklas Mollenhauer
Josh Story
Jan Kassens
Ruslan Lesiutin
XiaoPi
Dmytro Rykun
Batuhan Tomo
Jiachi Liu