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
Currently, any commit to React causes an internal sync since the Git
commit hash is part of the build. This creates a lot more sync commits
and noise than necessary, see:
https://github.com/facebook/react/commits/builds/facebook-www
This PR changes the version string to be a hash of the target build
files instead. This way we get a new version with any change that
actually impacts the generated files and still have a matching version
across the files.
Some build artifacts contain multiple version strings. It seems like an
oversight to me that this `.replace` call just replaces the one that
happens to be first.
When the work loop is suspended, we shouldn't schedule a new render task
until the promise has resolved. When I originally implemented this, I
wasn't sure where to put this logic — `ensureRootIsScheduled` is the
more natural place for it, but that's also a really hot path, so I chose
to do it elsewhere, and left a TODO to reconsider later.
Now it's later. I'm working on a refactor to move the
`ensureRootIsScheduled` call to always happen in a microtask, so that if
there are multiple updates/pings in a single event, they get batched
into a single operation. Which means I can put the logic in that
function where it belongs.
(This only affects our own internal repo; it's not a public API.)
I think most of us agree this is a less confusing name. It's possible
someone will confuse it with `console.log`. If that becomes a problem we
can warn in dev or something.
## Summary
The following methods have exactly the same implementation on Fabric and
the legacy renderer:
* `findHostInstance_DEPRECATED`
* `findNodeHandle`
* `dispatchCommand`
* `sendAccessibilityEvent`
This just extracts those functions to a common module so they're easier
to change (no need to sync changes in 2 files).
## How did you test this change?
Existing tests (this is a refactor).
Based on a bug report from @bvaughn.
`act` should not consult `shouldYield` when it's performing work,
because in a unit testing environment, I/O (such as `setTimeout`) is
likely mocked. So the result of `shouldYield` can't be trusted.
In the regression test, I simulate the bug by mocking `shouldYield` to
always return `true`. This causes an infinite loop in `act`, because it
will keep trying to render and React will keep yielding.
We support any super type of anything that we can serialize. Meaning
that as long as the Type that's passed through is less precise, it means
that we can encoded it as any subtype and therefore the incoming type
doesn't have to be the subtype in that case. Basically, as long as
you're only passing through an `Iterable<T>` in TypeScript, then you can
pass any `Iterable<T>` and we'll treat it as an array.
For example we support Promises *and* Thenables but both are encoded as
Promises.
We support Arrays and since Arrays are also Iterables, we can support
Iterables.
For @wongmjane
Previously when a called server reference function was rejected, the
emitted error chunk was not flushed, and the request was not properly
closed.
Co-authored-by: Sebastian Markbage <sebastian@calyptus.eu>
We always look up these references in a map so it doesn't matter what
their value is. It could be a hash for example.
The loaders now encode a single $$id instead of filepath + name.
This changes the react-client-manifest to have a single level. The value
inside the map is still split into module id + export name because
that's what gets looked up in webpack.
The react-ssr-manifest is still two levels because that's a reverse
lookup.
This converts some of our test suite to use the `waitFor` test pattern,
instead of the `expect(Scheduler).toFlushAndYield` pattern. Most of
these changes are automated with jscodeshift, with some slight manual
cleanup in certain cases.
See #26285 for full context.
This converts some of our test suite to use the `waitFor` test pattern,
instead of the `expect(Scheduler).toFlushAndYield` pattern. Most of
these changes are automated with jscodeshift, with some slight manual
cleanup in certain cases.
See #26285 for full context.
This converts some of our test suite to use the `waitFor` test pattern,
instead of the `expect(Scheduler).toFlushAndYield` pattern. Most of
these changes are automated with jscodeshift, with some slight manual
cleanup in certain cases.
See #26285 for full context.
This converts some of our test suite to use the `waitFor` test pattern,
instead of the `expect(Scheduler).toFlushAndYield` pattern. Most of
these changes are automated with jscodeshift, with some slight manual
cleanup in certain cases.
See #26285 for full context.
I'm in the process of codemodding our test suite to the waitFor pattern.
See #26285 for full context.
This module required a lot of manual changes so I'm doing it as its own
PR. The reason is that most of the tests involved simulating an async
import by wrapping them in `Promise.resolve()`, which means they would
immediately resolve the next time the microtask queue was flushed. I
rewrote the tests to resolve the simulated import explicitly.
While converting these tests, I also realized that the `waitFor` helpers
weren't properly waiting for the entire microtask queue to recursively
finish — if a microtask schedules another microtask, the subsequent one
wouldn't fire until after `waitFor` had resolved. To fix this, I used
the same strategy as `act` — wait for a real task to finish before
proceeding, such as a message event.
## Summary
Fix bug in how the Fizz external runtime processes existing template
elements.
Bug:
- getElementsByTagName returns a HTMLCollection, which is live.
- while iterating over an HTMLCollection, we call handleNode which
removes nodes
Fix:
- Call Array.from to copy children of `document.body` before processing.
- We could use `querySelectorAll` instead, but that is likely slower due
to reading more nodes.
## How did you test this change?
Did ad-hoc testing on Facebook home page by commenting out the mutation
observer and adding the following.
```javascript
window.addEventListener('DOMContentLoaded', function () {
handleExistingNodes(document.body);
});
```
This converts some of our test suite to use the `waitFor` test pattern,
instead of the `expect(Scheduler).toFlushAndYield` pattern. Most of
these changes are automated with jscodeshift, with some slight manual
cleanup in certain cases.
See #26285 for full context.
Andrew Clark
