Summary:
Change detection is desgined to determine whether rules of react have been violated, and to do so better we need to loosen Forgets assumptions about what kinds of values don't need to be memoized. For example, the compiler typically doesn't think of o.x as something that needs to be memoized, because it does not allocate. However, we want to compare o.x in the current render with o.x in a previous one, so we now insert a "memoization" (comparison, really) block around this value.
The structure of this work is that we now add a reactive scope for identifiers if they originate from any instruction that could read from state that could have mutated between renders. This means that `LoadProperty` is always going to get a reactive scope; `LoadGlobal` will get a scope if we're not reading from something mutable, and `PrefixUpdate` won't (because the variable being incremented would have already been loaded and checked).
Supersedes #30180.
[ghstack-poisoned]
Summary:
Change detection is desgined to determine whether rules of react have been violated, and to do so better we need to loosen Forgets assumptions about what kinds of values don't need to be memoized. For example, the compiler typically doesn't think of o.x as something that needs to be memoized, because it does not allocate. However, we want to compare o.x in the current render with o.x in a previous one, so we now insert a "memoization" (comparison, really) block around this value.
The structure of this work is that we now add a reactive scope for identifiers if they originate from any instruction that could read from state that could have mutated between renders. This means that `LoadProperty` is always going to get a reactive scope; `LoadGlobal` will get a scope if we're not reading from something mutable, and `PrefixUpdate` won't (because the variable being incremented would have already been loaded and checked).
Supersedes #30180.
[ghstack-poisoned]
Updates this script to download from GH actions instead, to prepare for
moving the cron jobs over to GH actions.
ghstack-source-id: 9bba9f2721e42b508d7fac6604be72906dedd836
Pull Request resolved: https://github.com/facebook/react/pull/30485
Extracts out the code to download builds from GH into its own module so
that it can be reused later.
ghstack-source-id: 26687db971d06339d099d1d5075825efb82cf6b8
Pull Request resolved: https://github.com/facebook/react/pull/30484
**This API is not intended to ship. This is a temporary unstable hook
for internal performance profiling.**
This PR exposes `unstable_useContextWithBailout`, which takes a compare
function in addition to Context. The comparison function is run to
determine if Context propagation and render should bail out earlier.
`unstable_useContextWithBailout` returns the full Context value, same as
`useContext`.
We can profile this API against `useContext` to better measure the cost
of Context value updates and gather more data around propagation and
render performance.
The bailout logic and test cases are based on
https://github.com/facebook/react/pull/20646
Additionally, this implementation allows multiple values to be compared
in one hook by returning a tuple to avoid requiring additional Context
consumer hooks.
Now that we've fully migrated the PR CI workflow (build_and_test) to GH,
we can fully delete the workflow altogether. This will break
https://react-builds.vercel.app/ unfortunately but I'll wait for
acdlite to come back from vacation and work with him to fix it.
The remaining jobs in circleci are for publishing prereleases of React
to npm. I'll work on migrating those next.
ghstack-source-id: 05f346829f
Pull Request resolved: https://github.com/facebook/react/pull/30480
Promotes v2 to the primary workflow file so that we don't double write
to the protected branches.
Note: this may break DiffTrain temporarily, I will fix forward if so
ghstack-source-id: f6505a72f2
Pull Request resolved: https://github.com/facebook/react/pull/30477
Adds back the missing steps with a few tweaks to where previously some
`github` context value was referenced, I changed it to read from the
triggering workflow_run (ie the build on `main`) instead.
ghstack-source-id: 4b0fa135f0
Pull Request resolved: https://github.com/facebook/react/pull/30476
In https://github.com/facebook/react/pull/23316 we fixed a bug where
onload events were missed if they happened too early. This update adds
support for srcset to retrigger the load event. Firefox unfortunately
does not trigger a load even when you assign srcset so this won't work
in every browser when you use srcset without src however it does close a
gap in chrome at least
Unfortunately creating a workflow that depends on another worfklow run
requires it to first be merged into main, so I can't really test porting
this without landing it first.
To do this safely, I've left the original job intact for DiffTrain and
added a forked file. This fork only currently downloads the artifact
from the HEAD commit in GH actions; I've removed the steps that push to
the protected branches for now while I test to see if this works as
expected.
This workflow needs to depend on the runtime_build_and_test workflow
being complete because otherwise it will fail since the artifacts
haven't been built yet.
ghstack-source-id: 0f9cebc525
Pull Request resolved: https://github.com/facebook/react/pull/30472
More or less a straight copy from the circleci config. I spotted some
inefficiencies but will fix those later to make reviewing this easier.
ghstack-source-id: cb3456c602
Pull Request resolved: https://github.com/facebook/react/pull/30406
This was only used for build_and_test which has since been migrated to
gh actions and is now therefore unused. Looks like I missed this during
the previous cleanup.
ghstack-source-id: 278443951e
Pull Request resolved: https://github.com/facebook/react/pull/30467
To handle more cases, always append the synthetic outlined function as a
new child of the module rather than make assumptions about the original
function. This should handle whatever case where the original function
expression may be a child of a variety of parents
ghstack-source-id: 8581edb8be
Pull Request resolved: https://github.com/facebook/react/pull/30466
Addresses follow up feedback from #30446. Since the outlined function is
guaranteed to have a module-scoped unique identifier name, we can
simplify the insertion logic for the outlined function to always emit a
function declaration rather than switch based on the original function
type. This is fine because the outlined function is synthetic anyway.
ghstack-source-id: 0a4d1f7b0a
Pull Request resolved: https://github.com/facebook/react/pull/30464
If a function expression that mutates a global is passed as a prop,
we don't throw an error as we assume it's not called in render.
But if this function expression is captured in an object and passed down
as prop, we throw an error.
ghstack-source-id: 74cacee09f565550007b2e01fa8877ad64ccfbe9
Pull Request resolved: https://github.com/facebook/react/pull/30456
This is a major nit but this avoids an extra stack frame when we're
replaying logs.
Normally the `printToConsole` frame doesn't show up because it'd be
ignore listed.
<img width="421" alt="Screenshot 2024-07-25 at 11 49 39 AM"
src="https://github.com/user-attachments/assets/81334c2f-e19e-476a-871e-c4db9dee294e">
When you expand to show ignore listed frames a ton of other frames show
up.
<img width="516" alt="Screenshot 2024-07-25 at 11 49 47 AM"
src="https://github.com/user-attachments/assets/2ab8bdfb-464c-408d-9176-ee2fabc114b6">
The annoying thing about this frame is that it's at the top of the stack
where as typically framework stuff ends up at the bottom and something
you can ignore. The user space stack comes first.
With this fix there's no longer any `printToConsole` frame.
<img width="590" alt="Screenshot 2024-07-25 at 12 09 09 PM"
src="https://github.com/user-attachments/assets/b8365d53-31f3-43df-abce-172d608d3c9c">
Am I wiling to eat the added complexity and slightly slower performance
for this nit? Definitely.
Following https://github.com/facebook/react/pull/30436
Concurrent by default strategy has been unshipped. Here we clean up the
`allowConcurrentByDefault` path and related logic/tests.
For now, this keeps the `concurrentUpdatesByDefaultOverride` argument in
`createContainer` and `createHydrationContainer` and ignores the value
to prevent more breaking changes to `react-reconciler` in the RC stage.
This enables configuring the name of the requested environment.
When we currently use createTask, we start with a `"use server"`
annotation. This option basically configures that string.
I now also deal with the case when switching environments along the
owner path. If you go from `"Third Party"` to `"Server"` to `"Client"`,
it'll have a task named `"use third party"` at the root, then `"use
server"` and then finally `"use client"`.
We don't really have the concept of a Server Component making a request
during render to then create another Server Component. Really the inner
one should conceptually have the first one as its owner in that case. So
currently the inner one will always have a null owner. We could somehow
connect them in this server-to-server case.
We don't currently have a way to configure the `"use client"` option but
I figured maybe that could be inferred by the server environment that
the Flight Client is executed within.
Note: We did talk before about annotating each stack frame with the
environment. You can effectively do that manually when parsing
`rsc://React/{environment}/` from `captureOwnerStack`. However, we can't
do that natively. At least not without deeper integration. Because it's
the source map that's responsible for the actual function name of each
stack frame - not what we give it at runtime. So for the native stacks,
the task showing the change in environment is more practical.
This way you can use the environment to know where to look for the
source map in case you have multiple server environments.
This becomes part of the public protocol since it's part of what you'll
parse out of the `rsc://React/` prefixed URLs inside of
`captureOwnerStack`.
This lets you customize the filter, for example allowing node_modules or
filter out additional functions that you don't want to include when
sending the stack to the client.
Notably this doesn't filter out Server Components out of the parent
stack. Those are just like a view of the tree by name. Not virtual stack
frames.
https://github.com/facebook/react/pull/30422 broke existing build
shortcuts.
Revert the usage of `names` (`_`) and `type` args.
`yarn build-for-devtools` / `yarn build-for-devtools-dev` / `yarn
build-for-devtools-prod` should all work again.
Moved the bundleType documentation into description so they can be fuzzy
matched. But a build like `yarn build --type FB_WWW_PROD` still works
when matched exactly.
There's probably a better way to document the positional `names` arg in
the `--help` command, but didn't see it when browsing the yargs docs so
let's just fix the existing builds for now.
Now:
```
% yarn build --help
yarn run v1.22.19
$ node ./scripts/rollup/build-all-release-channels.js --help
Options:
--help Show help [boolean]
--version Show version number [boolean]
--releaseChannel, -r Build the given release channel. [string] [choices: "experimental", "stable"]
--index, -i Worker id. [number]
--total, -t Total number of workers. [number]
--ci Run tests in CI [choices: "circleci", "github"]
--type Build the given bundle type. (NODE_ES2015,ESM_DEV,ESM_PROD,NODE_DEV,NODE_PROD,NODE_PROFILING,BUN_DEV,BUN_PROD,FB_WWW_DEV,FB_WWW_PROD,FB_WWW_PROFILING,RN_OSS_DE
V,RN_OSS_PROD,RN_OSS_PROFILING,RN_FB_DEV,RN_FB_PROD,RN_FB_PROFILING,BROWSER_SCRIPT) [string]
--pretty Force pretty output. [boolean]
--sync-fbsource Include to sync build to fbsource. [string]
--sync-www Include to sync build to www. [string]
--unsafe-partial Do not clean ./build first.
```
When a model references a deduped object of a blocked element that has
subsequently been turned into a lazy element, we need to wait for the
lazy element's chunk to resolve before resolving the reference.
Without the fix, the new test failed with the following runtime error:
```
TypeError: Cannot read properties of undefined (reading 'children')
1003 | let value = chunk.value;
1004 | for (let i = 1; i < path.length; i++) {
> 1005 | value = value[path[i]];
| ^
1006 | }
1007 | const chunkValue = map(response, value);
1008 | if (__DEV__ && chunk._debugInfo) {
at getOutlinedModel (packages/react-client/src/ReactFlightClient.js:1005:26)
```
The bug was uncovered after updating React in Next.js in
https://github.com/vercel/next.js/pull/66711.
Previously we would insert new (Arrow)FunctionExpressions as a sibling
of the original function. However this would break in the outlining case
as it would cause the original function expression's parent to become a
SequenceExpression, breaking a bunch of assumptions in the babel plugin.
To get around this, we synthesize a new VariableDeclaration to contain
the newly inserted function expression and therefore insert it as a true
sibling to the original function.
Yeah, it's kinda gross
ghstack-source-id: df13e3b439962b95af4bbd82ef4302624668faf7
Pull Request resolved: https://github.com/facebook/react/pull/30446
I discovered this compiler crash while trying to do an internal sync of
the compiler. Any kind of outlining appears to crash the babel plugin
when the component is a function expression.
ghstack-source-id: 4f717674af91d4d4b730e64cbd7a144b9faab13e
Pull Request resolved: https://github.com/facebook/react/pull/30443
We still filter them before passing from server to client in Flight
Server but when presenting a native stack, we don't need to filter them.
That's left to ignore listing in the presentation.
The stacks are pretty clean regardless thanks to the bottom stack
frames.
We can also unify the owner stack formatters into one shared module
since Fizz/Flight/Fiber all do the same thing. DevTools currently does
the same thing but is forked so it can support multiple versions.
Concurrent by default has been unshipped! Let's clean it up.
Here we remove `forceConcurrentByDefaultForTesting`, which allows us to
run tests against both concurrent strategies. In the next PR, we'll
remove the actual concurrent by default code path.
Addresses discussion at https://github.com/facebook/react/pull/30399#discussion_r1684693021. Once we've constructed scopes it's invalid to use identifier mutable ranges. The only places we can do this which i can find are ValidateMemoizedEffectDeps (which is already flawed and disabled by default) and ValidatePreservedManualMemoization. I added a todo to the former, and fixed up the latter.
The idea of the fix is that for StartMemo dependencies, if they needed to be memoized (identifier.scope != null) then that scope should exist and should have already completed. If they didn't need a scope or can't have one created (eg their range spans a hook), then their scope would be pruned. So if the scope is set, not pruned, and not completed, then it's an error.
For declarations (FinishMemo) the existing logic applies unchanged.
ghstack-source-id: af5bfd88553de3e30621695f9d139c4dc5efb997
Pull Request resolved: https://github.com/facebook/react/pull/30428
Later passes may rely on HIR invariants such as blocks being in RPO or instructions having unique, ascending InstructionIds. However, BuildReactiveScopeTerminalsHIR doesn't currently gurantee this.
This PR updates that pass to first restore RPO, fixup predecessors (the previous logic tried to do this but failed on unreachable blocks, where `markPredecessors()` handles that case), and renumber instructions. Then it walks instructions and scopes to update identifier and scope ranges given the new instruction ids.
ghstack-source-id: 2a99df02ac9d125b202cae369e2dc4dccefb0625
Pull Request resolved: https://github.com/facebook/react/pull/30399
Once we create scopes, we should prefer to use the block structure to identify active scope ranges rather than the scope range. They _should_ always be in sync, but ultimately the block structure determine the active range (ie the id of the 'scope' terminal and the terminal's fallthrough block).
ghstack-source-id: 730b6d1cfaf0eb689d71057c78a48045ac4fb11c
Pull Request resolved: https://github.com/facebook/react/pull/30398
Doing some debugging I noticed that a few of the newer terminals kinds weren't printing the instruction id.
ghstack-source-id: e0e4c96aeefdfe09d3be1527fd7103b4e506eb8e
Pull Request resolved: https://github.com/facebook/react/pull/30397
Stacked on #30427.
Most hooks and such are called inside renders which already have these
on the stack but life-cycles that call out on them are useful to cut off
too.
Typically we don't create JSX in here so they wouldn't be part of owner
stacks anyway but they can be apart of plain stacks such as the ones
prefixes to console logs or printed by error dialogs.
This lets us cut off any React internals below. This should really be
possible using just ignore listing too ideally.
At this point we should maybe just build a Babel plugin that lets us
annotate a function to need to have this name.
The current stack is available in the native UI but that's hidden by
default so you don't see the actual current component on the stack.
This is unlike the native async stacks UI where they're all together.
So we prefix the stack with the current stack first.
<img width="279" alt="Screenshot 2024-07-22 at 10 05 13 PM"
src="https://github.com/user-attachments/assets/8f568fda-6493-416d-a0be-661caf44d808">
---------
Co-authored-by: Ruslan Lesiutin <rdlesyutin@gmail.com>
babel-plugin-idx enforces that its 2nd argument is an arrow function, so
outlining needs to skip over lambdas that are args to idx. This PR adds
a small repro highlighting the issue.
ghstack-source-id: b4627ec552056f33090e2f7bc0536a6006d79d18
Pull Request resolved: https://github.com/facebook/react/pull/30435
To surface any potential conflicts with this plugin, let's install it
into snap so we can surface any runtime errors after compilation
ghstack-source-id: 545eee6fb7f6401e919422581cf64070da581d50
Pull Request resolved: https://github.com/facebook/react/pull/30434
This ensures that we can keep overriding what runtime to use by
resetting modules while still using the automatic JSX plugin. This is
like the "inline requires" transform but just for JSX.
I got sick of trying to figure out workarounds to hide the extra stack
frame that appears due to the wrappers.
