As I understand it this isn't used at Meta and it would let us get rid
of at least legacy mode hydration code when we remove legacy mode from
OSS builds.
<!--
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?
-->
`react-dom/server` in Bun (correctly) chooses `react-dom/server.bun`,
but `react-dom/server.bun` currently can't be imported because it is not
included in package.json `"exports"` (`react-dom/server` works,
`react-dom/server.bun` doesn't). Previously, I didn't think it was
necessary to do that, but it is too easy to accidentally run the browser
build in unit tests when importing `react-dom/server`
This also aligns behavior of package.json `"exports"` of
`react-dom/server.bun` with `react-dom/server.browser`,
`react-dom/server.node`, and the rest.
## How did you test this change?
Manually edited package.json in node_modules in a separate folder and
ran tests in Bun with `react-dom/server.bun` as the import specifier
<!--
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.
-->
## Summary
I refactored this code in #26290 but forgot to add guards when the fiber
or the state node where null, and this is typed as `any` so Flow didn't
catch it.
This restores the same logic to guard against null.
## How did you test this change?
Existing tests.
There's currently a giant cycle between the event system, through
react-dom-bindings, reconciler and then react-dom. We resolve this cycle
using dependency injection. However, this all ends up in the same
bundle. It can be reordered to resolve the cycles. If we avoid
side-effects and avoid reading from module exports during
initialization, this should be resolvable in a more optimal way by the
compiler.
I'm trying to get rid of all meta programming in the module scope so
that closure can do a better job figuring out cyclic dependencies and
ability to reorder.
This is converting a lot of the patterns that assign functions
conditionally to using function declarations instead.
```
let fn;
if (__DEV__) {
fn = function() {
...
};
}
```
->
```
function fn() {
if (__DEV__) {
...
}
}
```
These used to be used by partial render.
ReactDOMDispatcher ended up not being used in this way.
Move shared DOM files to client. These are only used by client
abstractions now. They're inlined in the Fizz code so they're no longer
shared.
Now that promises are renderable nodes, we can remove the `use` call
from the root of the Flight fixture.
Uncached promises will likely be accompanied by a warning when they are
rendered outside a transition. But this promise is the result of a
Flight response, so it's cached. And it's also a rendered as part of a
transition. So it's fine. Indeed, this is the canonical way to use this
feature.
This doesn't need its own set of flags. We use things like `__PROFILE__`
in the regular feature flags file to fork for the `react-dom/profiling`
build so we can do the same here if needed but I don't think we actually
need to fork this anywhere as far as I can tell.
We disallow empty strings for `href` and `src` since they're common
mistakes that end up loading the current page as a preload, image or
link. We also disallow it for `action`. You have to pass `null` which is
the same.
However, for `formAction` passing `null` is not the same as passing
empty string. Passing empty string overrides the form's action to be the
current page even if the form's action was set to something else.
There's no easy way to express the same thing `#` show up in the user
visible URLs and `?` clears the search params.
Since this is also not a common mistake, we can just allow this.
## Summary
The current definition of `Instance` in Fabric has 2 fields:
- `node`: reference to the native node in the shadow tree.
- `canonical`: public instance provided to users via refs + some
internal fields needed by Fabric.
We're currently using `canonical` not only as the public instance, but
also to store internal properties that Fabric needs to access in
different parts of the codebase. Those properties are, in fact,
available through refs as well, which breaks encapsulation.
This PR splits that into 2 separate fields, leaving the definition of
instance as:
- `node`: reference to the native node in the shadow tree.
- `publicInstance`: public instance provided to users via refs.
- Rest of internal fields needed by Fabric at the instance level.
This also migrates all the current usages of `canonical` to use the
right property depending on the use case.
To improve encapsulation (and in preparation for the implementation of
this [proposal to bring some DOM APIs to public instances in React
Native](https://github.com/react-native-community/discussions-and-proposals/pull/607)),
this also **moves the creation of and the access to the public instance
to separate modules** (`ReactFabricPublicInstance` and
`ReactFabricPublicInstanceUtils`). In a following diff, that module will
be moved into the `react-native` repository and we'll access it through
`ReactNativePrivateInterface`.
## How did you test this change?
Existing unit tests.
Manually synced the PR in Meta infra and tested in Catalyst + the
integration with DevTools. Everything is working normally.
For various reasons some of the DevTools e2e tests uses our repo's
private internal version of `act`. It should really just be using the
public one.
This converts one of the usages, because it was causing CI to fail.
## Based on #25634
Like promises, this adds support for Context as a React node.
In this initial implementation, the context dependency is added to the
parent of child node. This allows the parent to re-reconcile its
children when the context updates, so that it can delete the old node if
the identity of the child has changed (i.e. if the key or type of an
element has changed). But it also means that the parent will replay its
entire begin phase. Ideally React would delete the old node and mount
the new node without reconciling all the children. I'll leave this for a
future optimization.
Implements Promise as a valid React node types. The idea is that any
type that can be unwrapped with `use` should also be renderable.
When the reconciler encounters a Usable in a child position, it will
transparently unwrap the value before reconciling it. The value of the
inner value will determine the identity of the child during
reconciliation, not the Usable object that wraps around it.
Unlike `use`, the reconciler will recursively unwrap the value until it
reaches a non-Usable type, e.g. `Usable<Usable<Usable<T>>>` will resolve
to T.
In this initial commit, I've added support for Promises. I will do
Context in the [next
step](https://github.com/facebook/react/pull/25641).
Being able to render a promise as a child has several interesting
implications. The Server Components response format can use this feature
in its implementation — instead of wrapping references to client
components in `React.lazy`, it can just use a promise.
This also fulfills one of the requirements for async components on the
client, because an async component always returns a promise for a React
node. However, we will likely warn and/or lint against this for the time
being because there are major caveats if you re-render an async
component in response to user input. (Note: async components already
work in a Server Components environment — the caveats only apply to
running them in the browser.)
To suspend, React uses the same algorithm as `use`: by throwing an
exception to unwind the stack, then replaying the begin phase once the
promise resolves. It's a little weird to suspend during reconciliation,
however, `lazy` already does this so if there were any obvious bugs
related to that we likely would have already found them.
Still, the structure is a bit unfortunate. Ideally, we shouldn't need to
replay the entire begin phase of the parent fiber in order to reconcile
the children again. This would require a somewhat significant refactor,
because reconciliation happens deep within the begin phase, and
depending on the type of work, not always at the end. We should consider
as a future improvement.
Adding `.internal` to a test file prevents it from being tested in build
mode. The best practice is to instead gate the test based on whether the
feature is enabled.
Ideally we'd use the `@gate` pragma in these tests, but the `itRenders`
test helpers don't support that.
The www builds include disableLegacyContext as a dynamic flag, so we
should be running the tests in that mode, too. Previously we were
overriding the flag during the test run. This strategy usually doesn't
work because the flags get compiled out in the final build, but we
happen to not test www in build mode, only source.
To get of this hacky override, I added a test gate to every test that
uses legacy context. When we eventually remove legacy context from the
codebase, this should make it slightly easier to find which tests are
affected. And removes one more hack from our hack-ridden test config.
Given that sometimes www has features enabled that aren't on in other
builds, we might want to consider testing its build artifacts in CI,
rather than just source. That would have forced this cleanup to happen
sooner. Currently we only test the public builds in CI.
`Scheduler.unstable_flushAll` in existing tests doesn't work with
microtask. This PR converts most of the remaining
`Scheduler.unstable_flushAll()` calls to using internal test utilities
to unblock refactoring `ensureRootIsScheduled` with scheduling a
microtask.
We're decompressing and then writing and recompressing in the proxy.
This causes it to stall if buffered because `.pipe()` doesn't force
flush automatically.
I updated some of the Float tests that intentionally trigger an error to
assert on the specific error message, rather than swallow any errors
that may or may not happen.
If something throws as a result of `flushSync`, and there's remaining
work left in the queue, React should keep working until all the work is
complete.
If multiple errors are thrown, React will combine them into an
AggregateError object and throw that. In environments where
AggregateError is not available, React will rethrow in an async task.
(All the evergreen runtimes support AggregateError.)
The scenario where this happens is relatively rare, because `flushSync`
will only throw if there's no error boundary to capture the error.
This adds `encodeReply` to the Flight Client and `decodeReply` to the
Flight Server.
Basically, it's a reverse Flight. It serializes values passed from the
client to the server. I call this a "Reply". The tradeoffs and
implementation details are a bit different so it requires its own
implementation but is basically a clone of the Flight Server/Client but
in reverse. Either through callServer or ServerContext.
The goal of this project is to provide the equivalent serialization as
passing props through RSC to client. Except React Elements and
Components and such. So that you can pass a value to the client and back
and it should have the same serialization constraints so when we add
features in one direction we should mostly add it in the other.
Browser support for streaming request bodies are currently very limited
in that only Chrome supports it. So this doesn't produce a
ReadableStream. Instead `encodeReply` produces either a JSON string or
FormData. It uses a JSON string if it's a simple enough payload. For
advanced features it uses FormData. This will also let the browser
stream things like File objects (even though they're not yet supported
since it follows the same rules as the other Flight).
On the server side, you can either consume this by blocking on
generating a FormData object or you can stream in the
`multipart/form-data`. Even if the client isn't streaming data, the
network does. On Node.js busboy seems to be the canonical library for
this, so I exposed a `decodeReplyFromBusboy` in the Node build. However,
if there's ever a web-standard way to stream form data, or if a library
wins in that space we can support it. We can also just build a multipart
parser that takes a ReadableStream built-in.
On the server, server references passed as arguments are loaded from
Node or Webpack just like the client or SSR does. This means that you
can create higher order functions on the client or server. This can be
tokenized when done from a server components but this is a security
implication as it might be tempting to think that these are not fungible
but you can swap one function for another on the client. So you have to
basically treat an incoming argument as insecure, even if it's a
function.
I'm not too happy with the naming parity:
Encode `server.renderToReadableStream` Decode: `client.createFromFetch`
Decode `client.encodeReply` Decode: `server.decodeReply`
This is mainly an implementation details of frameworks but it's annoying
nonetheless. This comes from that `renderToReadableStream` does do some
"rendering" by unwrapping server components etc. The `create` part comes
from the parity with Fizz/Fiber where you `render` on the server and
`create` a root on the client.
Open to bike-shedding this some more.
---------
Co-authored-by: Josh Story <josh.c.story@gmail.com>
This fixes a handful of tests that were accidentally relying on React
synchronously queuing work in the Scheduler after a setState.
Usually this is because they use a lower level SchedulerMock method
instead of either `act` or one of the `waitFor` helpers. In some cases,
the solution is to switch to those APIs. In other cases, if we're
intentionally testing some lower level behavior, we might have to be a
bit more clever.
Co-authored-by: Tianyu Yao <skyyao@fb.com>
## Summary
Adds support for returning `undefined` from Server Components.
Also fixes a bug where rendering an empty fragment would throw the same
error as returning undefined.
## How did you test this change?
- [x] test failed with same error message I got when returning undefined
from Server Components in a Next.js app
- [x] test passes after adding encoding for `undefined`
Node's MessageChannel implementation will leak if you don't explicitly
close the port. This updates the enqueueTask function we use in our
internal testing helpers to close the port once it's no longer needed.
We attempt to preload scripts that we detect during Fizz rendering
however when `noModule={true}` we should not because it will force
modern browser to fetch scripts they will never execute
Hoisted script resources already don't preload because we just emit the
resource immediately. This change currently on affects the preloads for
scripts that aren't hoistable
This is just moving some stuff around and renaming things.
This tuple is opaque to the Flight implementation and we should probably
encode it separately as a single string instead of a model object.
The term "Metadata" isn't the same as when used for ClientReferences so
it's not really the right term anyway.
I also made it optional since a bound function with no arguments bound
is technically different than a raw instance of that function (it's a
clone).
I also renamed the type ReactModel to ReactClientValue. This is the
generic serializable type for something that can pass through the
serializable boundary from server to client. There will be another one
for client to server.
I also filled in missing classes and ensure the serializable sub-types
are explicit. E.g. Array and Thenable.
Prior to #26347, our internal `act` API (not the public API) behaved
differently depending on whether the scope function returned a promise
(i.e. was an async function), for historical reasons that no longer
apply. Now that this is fixed, I've codemodded all async act scopes that
don't contain an await to be sync.
No pressing motivation other than it looks nicer and the codemod was
easy. Might help avoid confusion for new contributors who see async act
scopes with nothing async inside and infer it must be like that for a
reason.
This adds an async gap to our internal implementation of `act` (the one
used by our repo, not the public API). Rather than call the provided
scope function synchronously when `act` is called, we call it in a
separate async task. This is an extra precaution to ensure that our
tests do not accidentally rely on work being queued synchronously,
because that is an implementation detail that we should be allowed to
change. We don't do this in the public version of `act`, though we maybe
should in the future, for the same rationale. That might be tricky,
though, because it could break existing tests.
This also fixes the issue where our internal `act` requires an async
function. You can pass it a regular function, too.
We rely heavily on being able to batch rendering after multiple fetches
etc. have completed on the server. However, we only do this in the
Node.js build. Node.js `setImmediate` has the exact semantics we need.
To be after the current cycle of I/O so that we can collect after all
those I/O events already in the queue has been processed.
This doesn't exist in standard browsers, so we ended up not using it
there. We could've used `setTimeout` but that risks being throttled
which would severely negatively affect the performance so we just did it
synchronously there. We probably could just use the `scheduler` there.
Now we have a separate build for Edge where `setTimeout(..., 0)`
actually behaves like `setImmediate` which is what we want. So we can
just use that in that build.
@Jarred-Sumner not sure what you want for Bun.
To wait for the microtask queue to empty, our internal test helpers
schedule an arbitrary task using `setImmediate`. It doesn't matter what
kind of task it is, only that it's a separate task from the current one,
because by the time it fires, the microtasks for the current event will
have already been processed.
The issue with `setImmediate` is that Jest mocks it. Which can lead to
weird behavior.
I've changed it to instead use a message event, via the MessageChannel
implementation exposed by the `node:worker_threads` module.
We should consider doing this in the public implementation of `act`,
too.
Our internal `act` implementation flushes Jest's fake timer queue as a
way to force Suspense fallbacks to appear. So we should avoid using
timers in our internal tests.
This is not a public API. We only use it for our internal tests, the
ones in this repo. Let's move it to this private package. Practically
speaking this will also let us use async/await in the implementation.
- Specifies Node 16 as the minimum supported version.
- Remove no longer supported 17.x version (per
https://nodejs.dev/en/about/releases/)
- Add 19.x
Test Plan:
(using node 19) as that's what I'm adding)
- yarn build
- yarn test
**This commit only affects the internal version of `act` that we use in
this repo. The public `act` API is unaffected, for now.**
We should always await the result of an `act` call so that any work
queued in a microtask has a chance to flush. Neglecting to do this can
cause us to miss bugs when testing React behavior.
I codemodded all the existing `act` callers in previous PRs.
Similar to the rationale for `waitFor` (see #26285), we should always
await the result of an `act` call so that microtasks have a chance to
fire.
This only affects the internal `act` that we use in our repo, for now.
In the public `act` API, we don't yet require this; however, we
effectively will for any update that triggers suspense once `use` lands.
So we likely will start warning in an upcoming minor.
## Summary
In #26283, I changed definition of `NativeMethods` from an object to an
interface. This is correct but introduces a lot of errors in React
Native, so this restores the original definition and exports the fixed
type as a separate type so we can gradually migrate in React Native.
## How did you test this change?
Manually applied this change in React Native and validated the errors
are gone.
## Summary
resolves#25667
This PR also resolves several security issues in the standalone app
## How did you test this change?
Tested locally `yarn start` in react-devtools package. Everything works
normal
---
- To see the specific tasks where the Asana app for GitHub is being
used, see below:
- https://app.asana.com/0/0/1204123419819195
Similar to the rationale for `waitFor` (see #26285), we should always
await the result of an `act` call so that microtasks have a chance to
fire.
This only affects the internal `act` that we use in our repo, for now.
In the public `act` API, we don't yet require this; however, we
effectively will for any update that triggers suspense once `use` lands.
So we likely will start warning in an upcoming minor.
Similar to the rationale for `waitFor` (see #26285), we should always
await the result of an `act` call so that microtasks have a chance to
fire.
This only affects the internal `act` that we use in our repo, for now.
In the public `act` API, we don't yet require this; however, we
effectively will for any update that triggers suspense once `use` lands.
So we likely will start warning in an upcoming minor.
Similar to the rationale for `waitFor` (see
https://github.com/facebook/react/pull/26285), we should always await
the result of an `act` call so that microtasks have a chance to fire.
This only affects the internal `act` that we use in our repo, for now.
In the public `act` API, we don't yet require this; however, we
effectively will for any update that triggers suspense once `use` lands.
So we likely will start warning in an upcoming minor.
There's an old collection of test suites that test class component
behavior across ES6 (regular JavaScript classes), CoffeeScript classes,
and TypeScript classes. They work by running the same tests in all
environments and comparing the results.
Rather than use `act` or `waitFor` in these, I've changed them to use
`flushSync` instead so that they can flush synchronously. The reason is
that CoffeeScript doesn't have async/await, so we'd have to write those
tests differently than how they are written in the corresponding
modules. Since none of these tests cover any concurrent behavior, I
believe it's fine in this case to do everything synchronously; they
don't use any concurrent features, anyway, so effectively it's just
skipping a microtask.
## Summary
This is a small refactor of `ReactFabricHostComponent` to remove
unnecessary dependencies, unused methods and type definitions to
simplify a larger refactor of the class in a following PR
(https://github.com/facebook/react/pull/26321).
## How did you test this change?
Existing unit tests.
This PR is now based on #26256
The original matching function for `hydrateHoistable` some challenging
time complexity since we built up the list of matchable nodes for each
link of that type and then had to check to exclusion. This new
implementation aims to improve the complexity
For hoisted title tags we match the first title if it is valid (not in
SVG context and does not have `itemprop`, the two ways you opt out of
hoisting when rendering titles). This path is much faster than others
and we use it because valid Documents only have 1 title anyway and if we
did have a mismatch the rendered title still ends up as the
Document.title so there is no functional degradation for misses.
For hoisted link and meta tags we track all potentially hydratable
Elements of this type in a cache per Document. The cache is refreshed
once each commit if and only if there is a title or meta hoistable
hydrating. The caches are partitioned by a natural key for each type
(href for link and content for meta). Then secondary attributes are
checked to see if the potential match is matchable.
For link we check `rel`, `title`, and `crossorigin`. These should
provide enough entropy that we never have collisions except is contrived
cases and even then it should not affect functionality of the page. This
should also be tolerant of links being injected in arbitrary places in
the Document by 3rd party scripts and browser extensions
For meta we check `name`, `property`, `http-equiv`, and `charset`. These
should provide enough entropy that we don't have meaningful collisions.
It is concievable with og tags that there may be true duplciates `<meta
property="og:image:size:height" content="100" />` but even if we did
bind to the wrong instance meta tags are typically only read from SSR by
bots and rarely inserted by 3rd parties so an adverse functional outcome
is not expected.
## Do not hoist elements with `itemProp`
In HTML `itemprop` signifies a property of an `itemscope` with respect
to the Microdata spec
(https://html.spec.whatwg.org/multipage/microdata.html#microdata)
additionally `itemprop` is valid on any tag and can even make some tags
that are otherwise invalid in the `<body>` valid there (`<meta>` for
instance).
Originally I tried an approach where if you rendered something otherwise
hoistable inside an `itemscope` it would not hoist if it had an
`itemprop`. This meant that some components with `itemprop` could hoist
(if they were not scoped, which is generally invalid microdata
implementation). However the problem is things that do hoist, hoist into
the head and body and these tags can have an `itemscope`. This creates a
ton of ambiguity when trying to hydrate in these hoist scopes because we
can't know for certain whether a DOM node we find there was hoisted or
not even if it has an `itemprop` attribute. There are other scenarios
too that have abiguous semantics like rendering a hoistable with
`itemProp` outside of `<html itemScope={true>`. Is it fair to embed that
hoistable inside that itemScope even though it was defined outside?
To simplify the situation and disambiguate I dropped the `itemscope`
portion from the implementation and now any host component that could
normally be hoisted will not hoist if it has an `itemProp` prop.
In addition to the changes made for `itemProp` this PR also modifies
part of the hydration implementation to be more tolerant of tags
injected by 3rd parties. This was opportunistically done when we needed
to have context information like `inItemScope` but with the most recent
implementation that has been removed. I have however left the hydration
changes in place as it is a goal to make React handle hydrating the
entire Document even when we cannot control whether 3rd parties are
going to inject tags that React will not render but are also not
hoistables
-------
##### Original Description when we considered tracking itemScope
>One recent decision was to make elements using the `itemProp` prop not
hoistable if they were inside and itemScope. This better fits with
Microdata spec which allows for meta tags and other tag types usually
reserved for the `<head>` to be used in the `<body>` when using
itemScope.
>
>To implement this a number of small changes were necessary
>
>1. HostContext in prod needed to expand beyond just tracking the
element namespace for new element creation. It now tracks whether we are
in an itemScope. To keep this efficient it is modeled as a bitmask.
>2. To disambiguate what is and is not a potential instance in the DOM
for hoistables the hydration algo was updated to skip past non-matching
instances while attempting to claim the instance rather than ahead of
time (getNextHydratable).
>3. React will not consider an itemScope on `<html>`, `<head>`, or
`<body>` as a valid scope for the hoisting opt-out. This is important as
an invariant so we can make assumptions about certain tags in these
scopes. This should not be a functional breaking change because if any
of these tags have an `itemScope` then it can just be moved into the
first node inside the `<body>`
>
>Since we were already updating the logic for hydration to better
support `itemScope` opt-out I also changed the hydration behavior for
suspected 3rd party nodes in `<head>` and `<body>`. Now if you are
hydrating in either of those contexts hydration will skip past any
non-matching nodes until it finds a match. This allows 3rd party scripts
and extensions to inject nodes in either context that React does not
expect and still avoid a hydration mismatch.
>
>This new algorithm isn't perfect and it is possible for a mismatch to
occur. The most glaring case may be if a 3rd party script prepends a
`<div>` into `<body>` and you render a `<div>` in `<body>` in your app.
there is nothing to signal to React that this div was 3rd party so it
will claim is as the hydrated instance and hydration will almost
certainly fail immediately afterwards.
>
>The expectation is that this is rare and that if falling back to client
rendering is transparent to the user then there is not problem here. We
will continue to evaluate this and may change the hydration matching
algorithm further to match user and developer expectations
Sebastian Markbåge
Ricky
Jarred Sumner
Sophie Alpert
Jan Kassens
Rubén Norte
Andrew Clark
Tianyu Yao
Mengdi ChenandHendrik Liebau
Sebastian Silbermann
Josh Story
