It seems worthwhile to me to run a test to experiment with different
expiration times. This moves the expiration times for scheduler and
reconciler into FeatureFlags for the facebook build. Non-facebook should
not be affected by these changes.
Postponing in a promise that is being serialized to the client from the
server should be possible however prior to this change Flight treated
this case like an error rather than a postpone. This fix adds support
for postponing in this position and adds a test asserting you can
successfully prerender the root if you unwrap this promise inside a
suspense boundary.
Add a regression test for the [minimal
repro](https://codesandbox.io/s/react-18-suspense-state-never-resolving-bug-hmlny5?file=/src/App.js)
from @kassens
And includes the fix from @acdlite:
> This is another place we special-case Retry lanes to opt them out of
expiration. The reason is we rely on time slicing to unwrap uncached
promises (i.e. async functions during render). Since that ability is
still experimental, and enableRetryLaneExpiration is Meta-only, we can
remove the special case when enableRetryLaneExpiration is on, for now.
---------
Co-authored-by: Andrew Clark <git@andrewclark.io>
Small test similar to few tests added in #27740 , the `reactDom-input`
error message was just modified to match the error message, and the
`reactDomTextarea-test.js` has tests added to ensure more coverage.
## Summary
Concurrent rendering has been the default since React 18 release.
ReactTestRenderer requires passing `{unstable_isConcurrent: true}` to
match this behavior, which means by default tests written with RTR use a
different rendering method than the code they test.
Eventually, RTR should only use ConcurrentRoot. As a first step, let's
add a version of the concurrent option that isn't marked unstable. Next
we will follow up with removing the unstable option when it is safe to
merge.
## How did you test this change?
`yarn test
packages/react-test-renderer/src/__tests__/ReactTestRendererAsync-test.js`
As part of the process of removing the deprecated `react-dom/test-utils`
package references to `act` from this module are replaced with
references to `unstable_act` in `react`. It is likely that the unstable
act implementation will be made stable. The test utils act is just a
reexport of the unstable_act implementation in react itself.
Adds `isChildPublicInstance` method to both renderers (Fabric and
Paper), which will receive 2 public instances and return if first
argument is an ancestor of the second, based on fibers.
This will be used as a fallback when DOM node APIs are not available:
for Paper renderer or for Fabric without DOM node APIs.
How it is going to be used: to determine which `AppContainer` component
in RN is responsible for highlighting an inspected element on the
screen.
We should probably treat `React.use()` the same as `use()` to allow it
within loops and conditionals.
Ideally this would implement a test that `React` is imported or required
from `'react'`, but we don't otherwise implement such a test.
## Summary
This PR:
- Adds the `"sideEffects": false` flag to the `react-is` package, to
enable proper tree-shaking
## How did you test this change?
React-Redux v9 beta switches its artifacts from separate JS files to
pre-bundled artifacts. While testing builds locally, I noticed that our
use of `react-is` was no longer getting tree-shaken from Vite builds,
despite `react-is` only being used by our `connect` API and `connect`
itself getting shaken out of the final bundle.
Hand-adding `"sideEffects": false` to the `react-is` package locally
convinced Vite (+Rollup) to properly tree-shake `react-is` out of the
bundle when it wasn't being used.
I'd love to see this published as v18.2.1 as soon as this PR is merged -
we're hoping to release React-Redux v9 in the next few weeks!
## Summary
Follow up from #27717 based on feedback to rename the fork module itself
## How did you test this change?
- `yarn build`
- `yarn test
packages/scheduler/src/__tests__/SchedulerUMDBundle-test.internal.js`
Co-authored-by: Jack Pope <jackpope@meta.com>
This PR adds a new FB-specific configuration of Flight. We also need to
bundle a version of ReactSharedSubset that will be used for running
Flight on the server.
This initial implementation does not support server actions yet.
The FB-Flight still uses the text protocol on the server (the flag
`enableBinaryFlight` is set to false). It looks like we need some
changes in Hermes to properly support this binary format.
Adds `Forget` badge to all relevant components.
Changes:
- If component is compiled with Forget and using a built-in
`useMemoCache` hook, it will have a `Forget` badge next to its display
name in:
- components tree
- inspected element view
- owners list
- Such badges are indexable, so Forget components can be searched using
search bar.
Fixes:
- Displaying the badges for owners list inside the inspected component
view
Implementation:
- React DevTools backend is responsible for identifying if component is
compiled with Forget, based on `fiber.updateQueue.memoCache`. It will
wrap component's display name with `Forget(...)` prefix before passing
operations to the frontend. On the frontend side, we will parse the
display name and strip Forget prefix, marking the corresponding element
by setting `compiledWithForget` field. Almost the same logic is
currently used for HOC display names.
We are currently just pass the first element, which diverges from the
implementation for web. This is especially bad if you are inspecting
something like a list, where host fiber can represent multiple elements.
This part runs on the backend of React DevTools, so it should not affect
cases for React Native when frontend version can be more up-to-date than
backend's. I will double-check it before merging.
Once version of `react-devtools-core` is updated in React Native, this
should be supported, I will work on that later.
## Summary
I had to change the commands to be windows specific so that it doesn't
cause any crashes
## How did you test this change?
I successfully built the different types of devtools extenstions on my
personal computer. In future may need to add a github action with
windows config to test these errors
#27193
`onHeaders` can throw however for now we can assume that headers are
optimistic values since the only things we produce for them are preload
links. This is a pragmatic decision because React could concievably have
headers in the future which were not optimistic and thus non-optional
however it is hard to imagine what these headers might be in practice.
If we need to change this behavior to be fatal in the future it would be
a breaking change.
This commit adds error logging when `onHeaders` throws and ensures the
request can continue to render successfully.
In https://github.com/facebook/react/pull/27472 I've removed broken
`useMemoCache` implementation and replaced it with a stub. It actually
produces errors when trying to inspect components, which are compiled
with Forget.
The main difference from the implementation in
https://github.com/facebook/react/pull/26696 is that we are using
corresponding `Fiber` here, which has patched `updateQueue` with
`memoCache`. Previously we would check it on a hook object, which
doesn't have `updateQueue`.
Tested on pages, which are using Forget and by inspecting elements,
which are transpiled with Forget.
An attempt to see if we can bring back expiration of retry lanes to
avoid cases resolving Suspense can be starved by frequent updates.
In the past, this caused increase browser crashes, but a lot of time has
passed since then. Just trying if we can re-enable this.
Old PR that reverted adding the timeout:
https://github.com/facebook/react/pull/21300
## Summary
Forget compiled code currently cannot be tested with ReactTestRender as
`useMemoCache` is not being set on the dispatcher. This PR ensures that
projects can execute unit tests with Forget compilation in the test
build pipeline.
```js
// source code
function Component(props) {
// ...
}
// transformed code, which also should be evaluated in unit tests
function Component(props) {
const $ = useMemoCache(...);
// ...
}
```
This PR enables the `enableUseMemoCacheHook` feature flag for all bundle
variations of ReactTestRenderer. Forget *should* be the only caller of
useMemoCache, so this should be a reversible change (in the event we
need to change the implementation or api of the hook).
## How did you test this change?
* Check that generated ReactTestRenderer bundles contain `useMemoCache`.
* Synced to Meta and checked that unit tests that use Forget +
@testing-library/react pass.
I did not add new tests to check that useMemoCache can be called when
using the test renderer as `useMemoCache` is not yet stable. Happy to
add a test case here if that would be helpful to reviewers though (I'm
guessing that would go in
`packages/react-test-renderer/src/__tests__/ReactTestRenderer-test.js` )
These are all functionally equivalent changes.
- remove double negation and more explicit naming of
`hadNoMutationsEffects`
- use docblock syntax that's consumed by Flow
- remove useless cast
When we postpone during a render we inject a new segment synchronously
which we postpone. That gets assigned an ID so we can refer to it
immediately in the postponed state.
When we do that, the parent segment may complete later even though it's
also synchronous. If that ends up not having any content in it, it'll
inline into the child and that will override the child's segment id
which is not correct since it was already assigned one.
To fix this, we simply opt-out of the optimization in that case which is
unfortunate because we'll generate many more unnecessary empty segments.
So we should come up with a new strategy for segment id assignment but
this fixes the bug.
Co-authored-by: Josh Story <story@hey.com>
I introduced a bug in a recent change to how bootstrap scripts are
handled. Rather than clearing out the bootstrap script state from
ResumableState on completion of the prerender I did it during the
flushing phase which comes later after the postponed state has likely
been serialized. We should freeze these objects in dev so this is not
possible to do easily in test (nor in actual code in real systems).
This fixes the bug by eliminating the bootstrap config during
getPostponedState which is before the state can be serialized.
Previously it was possible to postpone in the shell during a prerender
and then during a resume the bootstrap scripts would not be emitted
leading to no hydration on the client. This change moves the bootstrap
configuration to `ResumableState` where it can be serialized after
postponing if it wasn't flushed as part of the static shell.
## Summary
There's a bug with the existing stack comparison algorithm in
`describeNativeComponentFrame` — specifically how it attempts to find a
common root frame between the control and sample stacks. This PR
attempts to fix that bug by injecting a frame that can have a guaranteed
string in it for us to search for in both stacks to find a common root.
## Brief Background/How it works now
Right now `describeNativeComponentFrame` does the following to leverage
native browser/VM stack frames to get details (e.g. script path, row and
col #s) for a single component:
1. Throwing and catching a control error in the function
2. Calling the component which should eventually throw an error (most of
the time), that we'll catch as our sample error.
3. Diffing the stacks in the control and sample errors to find the line
which should represent our component call.
## What's broken
To account for potential stack trace truncation, the stack diffing
algorithm first attempts to find a common "root" frame by inspecting the
earliest frame of the sample stack and searching for an identical frame
in the control stack starting from the bottom. However, there are a
couple of scenarios which I've hit that cause the above approach to not
work correctly.
First, it's possible that for render passes of extremely large component
trees to have a lot of repeating internal react function calls, which
can result in an incorrect common or "root" frame found. Here's a small
example from a stack trace using React Fizz for SSR.
Our control frame can look like this:
```
Error:
at Fake (...)
at construct (native)
at describeNativeComponentFrame (...)
at describeClassComponentFrame (...)
at getStackByComponentStackNode (...)
at getCurrentStackInDEV (...)
at renderNodeDestructive (...)
at renderElement (...)
at renderNodeDestructiveImpl (...) // <-- Actual common root frame with the sample stack
at renderNodeDestructive (...)
at renderElement (...)
at renderNodeDestructiveImpl (...) // <-- Incorrectly chosen common root frame
at renderNodeDestructive (...)
```
And our sample stack can look like this:
```
Error:
at set (...)
at PureComponent (...)
at call (native)
at apply (native)
at ErrorBoundary (...)
at construct (native)
at describeNativeComponentFrame (...)
at describeClassComponentFrame (...)
at getStackByComponentStackNode (...)
at getCurrentStackInDEV (...)
at renderNodeDestructive (...)
at renderElement (...)
at renderNodeDestructiveImpl (...) // <-- Root frame that's common in the control stack
```
Here you can see that the earliest trace in the sample stack, the
`renderNodeDestructiveImpl` call, can exactly match with multiple
`renderNodeDestructiveImpl` calls in the control stack (including file
path and line + col #s). Currently the algorithm will chose the
earliest/last frame with the `renderNodeDestructiveImpl` call (which is
the second last frame in our control stack), which is incorrect. The
actual matching frame in the control stack is the latest or first frame
(when traversing from the top) with the `renderNodeDestructiveImpl`
call. This leads to the rest of the stack diffing associating an
incorrect frame (`at getStackByComponentStackNode (...)`) for the
component.
Another issue with this approach is that it assumes all VMs will
truncate stack traces at the *bottom*, [which isn't the case for the
Hermes
VM](https://github.com/facebook/hermes/blob/df07cf713a84a4434c83c08cede38ba438dc6aca/lib/VM/JSError.cpp#L688-L699)
which **truncates stack traces in the middle**, placing a
```
at renderNodeDestructiveImpl (...)
... skipping {n} frames
at renderNodeDestructive (...)
```
line in the middle of the stack trace for all stacks that contain more
than 100 traces. This causes stack traces for React Native apps using
the Hermes VM to potentially break for large component trees. Although
for this specific case with Hermes, it's possible to account for this by
either manually grepping and removing the `... skipping` line and
everything below it (see draft PR: #26999), or by implementing the
non-standard `prepareStackTrace` API which Hermes also supports to
manually generate a stack trace that truncates from the bottom ([example
implementation](https://github.com/facebook/react/compare/main...KarimP:react:component-stack-hermes-fix)).
## The Fix
I found different ways to go about fixing this. The first was to search
for a common stack frame starting from the top/latest frame. It's a
relatively small change ([see
implementation](https://github.com/facebook/react/compare/main...KarimP:react:component-stack-fix-2)),
although it is less performant by being n^2 (albeit with `n`
realistically being <= 5 here). It's also a bit more buggy for class
components given that different VMs insert a different amount of
additional lines for new/construct calls...
Another fix would be to actually implement a [longest common
substring](https://en.wikipedia.org/wiki/Longest_common_substring)
algorithm, which can also be roughly n^2 time (assuming the longest
common substring between control and sample will be most of the sample
frame).
The fix I ended up going with was have the lines that throw the control
error and the lines that call/instantiate the component be inside a
distinct method under an object property
(`"DescribeNativeComponentFrameRoot"`). All major VMs (Safari's
JavaScriptCore, Firefox's SpiderMonkey, V8, Hermes, and Bun) should
display the object property name their stack trace. I've also set the
`name` and `displayName` properties for method as well to account for
minification, any advanced optimizations (e.g. key crushing), and VM
inconsistencies (both Bun and Safari seem to exclusively use the value
under `displayName` and not `name` in traces for methods defined under
an object's own property...).
We can then find this "common" frame by simply finding the line that has
our special method name (`"DescribeNativeComponentFrameRoot"`), and the
rest of the code to determine the actual component line works as
expected. If by any chance we don't find a frame with our special method
name in either control or sample stack traces, we then revert back to
the existing approach mentioned above by searching for the last line of
the sample frame in the control frame.
## How did you test this change?
1. There are bunch of existing tests that ensure a properly formatted
component trace is logged for certain scenarios, so I ensured the
existing full test suite passed
2. I threw an error in a component that's deep in the component
hierarchy of a large React app (facebook) to ensure there's stack trace
truncation, and ensured the correct component stack trace was logged for
Chrome, Safari, and Firefox, and with and without minification.
3. Ran a large React app (facebook) on the Hermes VM, threw an error in
a component that's deep in the component hierarchy, and ensured that
component frames are generated despite stack traces being truncated in
the middle.
<!--
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 flag has been tested internally on WWW, should be good to set to
true for OSS. Added a dynamic flag for fb RN.
## 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.
-->
yarn test
Adds a new option to `react-dom/server` entrypoints.
`onHeaders: (headers: Headers) => void` (non node envs)
`onHeaders: (headers: { Link?: string }) => void` (node envs)
When any `renderTo...` or `prerender...` function is called and this
option is provided the supplied function will be called sometime on or
before completion of the render with some preload link headers.
When provided during a `renderTo...` the callback will usually be called
after the first pass at work. The idea here is we want to get a set of
headers to start the browser loading well before the shell is ready. We
don't wait for the shell because if we did we may as well send the
preloads as tags in the HTML.
When provided during a `prerender...` the callback will be called after
the entire prerender is complete. The idea here is we are not responding
to a live request and it is preferable to capture as much as possible
for preloading as Headers in case the prerender was unable to finish the
shell.
Currently the following resources are always preloaded as headers when
the option is provided
1. prefetchDNS and preconnects
2. font preloads
3. high priority image preloads
Additionally if we are providing headers when the shell is incomplete
(regardless of whether it is render or prerender) we will also include
any stylesheet Resources (ones with a precedence prop)
There is a second option `maxHeadersLength?: number` which allows you to
specify the maximum length of the header content in unicode code units.
This is what you get when you read the length property of a string in
javascript. It's improtant to note that this is not the same as the
utf-8 byte length when these headers are serialized in a Response. The
utf8 representation may be the same size, or larger but it will never be
smaller.
If you do not supply a `maxHeadersLength` we defaul to `2000`. This was
chosen as half the value of the max headers length supported by commonly
known web servers and CDNs. many browser and web server can support
significantly more headers than this so you can use this option to
increase the headers limit. You can also of course use it to be even
more conservative. Again it is important to keep in mind there is no
direct translation between the max length and the bytelength and so if
you want to stay under a certain byte length you need to be potentially
more aggressive in the maxHeadersLength you choose.
Conceptually `onHeaders` could be called more than once as new headers
are discovered however if we haven't started flushing yet but since most
APIs for the server including the web standard Response only allow you
to set headers once the current implementation will only call it one
time
Had these stashed for some time, it includes:
- Some refactoring to remove unnecessary `FlowFixMe`s and type castings
via `any`.
- Optimized version of parsing component names. We encode string names
to utf8 and then pass it serialized from backend to frontend in a single
array of numbers. Previously we would call `slice` to get the
corresponding encoded string as a subarray and then parse each
character. New implementation skips `slice` step and just receives
`left` and `right` ranges for the string to parse.
- Early `break` instead of `continue` when Store receives unexpected
operation, like removing an element from the Store, which is not
registered yet.
`Activity` is the current candidate name. This PR starts the rename work
by renaming the exported unstable component name.
NOTE: downstream consumers need to rename the import when updating to
this commit.
## Summary
It's not clear to me why we currently create a new TaskController in
`runTask` – ultimately, we use the same signal and priority from the
original created in `unstable_scheduleCallback`
## How did you test this change?
```
yarn test SchedulerPostTask
```
Support Flow `as` expressions in ESLint rules, e.g. `<expr> as <type>`.
This is the same syntax as TypeScript as expressions. I just looked for
any place referencing `TSAsExpression` (the TS node) or
`TypeCastExpression` (the previous Flow syntax) and added a case for
`AsExpression` as well.
## Summary
There is a bug in the `react-hooks/exhaustive-deps` rule that forbids
the dependencies argument from being `undefined`. It triggers the error
that the dependency list is not an array literal. This makes sense in
pre ES5 strict-mode environments as undefined could be redefined, but
should not be a concern in today's JS environments.
**Justification:**
* The deps argument being undefined (for `useEffect` calls etc.) is a
valid use case for hooks that should re-run on every render.
* The deps argument being omitted is considered a valid use case by the
`exhaustive-deps` rule already.
* The TypeScript type definitions support passing `undefined` because
hooks are typed as `useEffect(effect: EffectCallback, deps?:
DependencyList): void;`.
* Since omitting an argument and passing `undefined` are considered
equivalent, this eslint rule should consider them as equivalent too.
Further, I accidentally forgot passing a dependency array to `useEffect`
in code that I shared on Twitter, and people started abusing me about
it. I'd like to create an eslint rule for my projects that requires me
to provide a dep argument in all cases (`undefined`, `[]` or the list of
dependencies) so that I can avoid such problems in the future. This
would also force me to always think about the dependencies instead of
accidentally forgetting them and my hook running on each render. In an
audit of my own codebase I had about 3% of hooks that I want to run on
each render, and adding an explicit `undefined` seems reasonable in
those situations.
It could be argued this could be an option or part of the
`exhaustive-deps` rule, but it's probably better to merge this PR, make
a release and see if my custom eslint rule gains traction in the future.
## How did you test this change?
* Added a test.
* `yarn test ESLintRuleExhaustiveDeps-test`
* Careful code inspection.
Updates useFormState to allow a sync function to be passed as an action.
A form action is almost always async, because it needs to talk to the
server. But since we support client-side actions, too, there's no reason
we can't allow sync actions, too.
I originally chose not to allow them to keep the implementation simpler
but it's not really that much more complicated because we already
support this for actions passed to startTransition. So now it's
consistent: anywhere an action is accepted, a sync client function is a
valid input.
creating the root after closing the response can lead to a promise that
never rejects. This is not intended use of the decodeReply API but if
pathalogical cases where you pass a raw FormData into this fucntion with
no zero chunk it can hang forever. This reordering causes a connection
error instead
---------
Co-authored-by: Zack Tanner <zacktanner@gmail.com>
<!--
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
Updated the typo in test description
<!--
Explain the **motivation** for making this change. What existing problem
does the pull request solve?
-->
## How did you test this change?
NA, correct test description improves readability of the code and
confusion for anyone who is new to the codebase.
<!--
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.
-->
This was missed that we track the child index on the task. The
equivalent in retryRenderTask already has this.
The effect is that a lazy node that suspends gets its child index reset
to -1 even though it should resume in the index it left off.
The loading state tracking for suspensey CSS is too complicated. Prior
to this change it had a state it could enter into where a stylesheet was
already in the DOM but the loading state did not know it was inserted
causing a later transition to try to insert it again.
This fix is to add proper tracking of insertions on the codepaths that
were missing it. It also modifies the logic of when to suspend based on
whether the stylesheet has already been inserted or not.
This is not 100% correct semantics however because a prior commit could
have inserted a stylesheet and a later transition should ideally be able
to wait on that load before committing. I haven't attempted to fix this
yet however because the loading state tracking is too complicated as it
is and requires a more thorough refactor. Additionally it's not
particularly valuable to delay a transition on a loading stylesheet when
a previous commit also relied on that stylesheet but didn't wait for it
b/c it was sync. I will follow up with an improvement PR later
fixes: https://github.com/facebook/react/issues/27585
I neglected to update the "last" pointer of the action queue. Since the
queue is circular, rather than dropping the update, the effect was to
add the update to the front of the queue instead of the back. I didn't
notice earlier because in my demos/tests, the actions would either
resolve really quickly or the actions weren't order dependent (like
incrementing a counter).
Jan Kassens
Josh Story
Tianyu YaoandAndrew Clark
BIKI DAS
Jack Pope
Ruslan Lesiutin
Mark Erikson
Andrey Lunyov
PrathamLalwani
Sebastian Markbåge
Rubén Norte
mofeiZ
Karim Piyar Ali
Noah Lemen
George Zahariev
Christoph Nakazawa
Sourabh singhandSophie Alpert
AZM