<!--
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 debug-test --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
Browsers restore state like forms and scroll position right after the
popstate event. To make sure the page work as expected on back or
forward button, we need to flush transitions scheduled in a popstate
synchronously, and only yields if it suspends.
This PR adds a new HostConfig method to check if `window.event ===
'popstate'`, and `scheduleMicrotask` if a transition is scheduled in a
`PopStateEvent`.
## How did you test this change?
yarn test
Many of our Suspense-related tests were written before the `act` API was
introduced, and use the lower level `waitFor` helpers instead. So they
are less resilient to changes in implementation details than they could
be.
This converts some of our test suite to use `act` in more places. I
found these while working on a PR to expand our fallback throttling
mechanism to include all renders that result from a promise resolving,
even if there are no more fallbacks in the tree.
I think this covers all the remaining tests that are affected.
This traces back to https://github.com/facebook/react/pull/6449 and then
another before that.
I think that back then we favored the property over the attribute, and
setting the property wouldn't be enough. However, the default path for
these are now using attributes if we don't special case it. So we don't
need it.
The only difference is that we currently have a divergence for
symbol/function behavior between controlled values that use the
getToStringValue helpers which treat them as empty string, where as
everywhere else they're treated as null/missing.
Since this comes with a warning and is a weird error case, it's probably
fine to change.
part of https://github.com/facebook/react/pull/26571
merging separately to improve tracking of files renames in git
Rename HostConfig files to FiberConfig to clarify they are configs for
Fiber and not Fizz/Flight. This better conforms to the naming used in
Flight and now Fizz of `ReactFlightServerConfig` and `ReactFizzConfig`
In #26573 I changed it so that textareas get their defaultValue reset if
you don't specify one.
However, the way that was implemented, it always set it for any update
even if it hasn't changed.
We have a test for that, but that test only works if no properties
update at all so that no update was scheduled. This fixes the test so
that it updates some unrelated prop.
I also found a test for `checked` that needed a similar fix.
Interestingly, we don't do this deduping for `defaultValue` or
`defaultChecked` on inputs and there's no test for that.
This is mainly renaming some stuff. The behavior change is
hasOwnProperty to nullish check.
I had a bigger refactor that was a dead-end but might as well land this
part and see if I can pick it up later.
There are four places we have special cases based off the DOMProperty
config:
1) DEV-only: ReactDOMUnknownPropertyHook warns for passing booleans to
non-boolean attributes. We just need a simple list of all properties
that are affected by that. We could probably move this in under setProp
instead and have it covered by that list.
2) DEV-only: Hydration. This just needs to read the value from an
attribute and compare it to what we'd expect to see if it was rendered
on the client. This could use some simplification/unification of the
code but I decided to just keep it simple and duplicated since code size
isn't an issue.
3) DOMServerFormatConfig pushAttribute: This just maps the special case
to how to emit it as a HTML attribute.
4) ReactDOMComponent setProp: This just maps the special case to how to
emit it as setAttribute or removeAttribute.
Basically we just have to remember to keep pushAttribute and setProp
aligned. There's only one long switch in prod per environment.
This just turns it all to a giant simple switch statement with string
cases. This is in theory the most optimizable since syntactically all
the information for a hash table is there. However, unfortunately we
know that most VMs don't optimize this very well and instead just turn
them into a bunch of ifs. JSC is best. We can minimize the cost by just
moving common attribute to the beginning of the list.
If we shipped this, maybe VMs will get it together to start optimizing
this case but there's a chicken and egg problem here and the game theory
reality is that we probably don't want to regress. Therefore, I intend
to do a follow up after landing this which reintroduces an object
indirection for simple property aliases. That should be enough to make
the remaining cases palatable. I'll also extract the most common
attributes to the beginning or separate ifs.
Ran attribute-behavior fixture and the table is the same.
This reverts commit b2ae9ddb3b.
While the feature flag is fully rolled out, these tests are also testing
behavior set with an unstable flag on root, which for now we want to
preserve.
Not sure if there's a better way then adding a dynamic feature flag to
the www build?
We almost never want to show content before its styles have loaded. But
eventually we will give up and allow unstyled content. So this extends
the timeout to a full minute. This somewhat arbitrary — big enough that
you'd only reach it under extreme circumstances.
Note that, like regular Suspense, the app is still interactive while
we're waiting for content to load. Only the unstyled content is blocked
from appearing, not updates in general. A new update will interrupt it.
We should figure out what the browser engines do during initial page
load and consider aligning our behavior with that. It's supposed to be
render blocking by default but there may be some cases where they, too,
give up and FOUC.
I originally made it so that a Suspensey commit — i.e. a commit that's
waiting for a stylesheet, image, or font to load before proceeding —
could not be interrupted by transitions. My reasoning was that Suspensey
commits always time out after a short interval, anyway, so if the
incoming update isn't urgent, it's better to wait to commit the current
frame instead of throwing it away.
I don't think this rationale was correct, for a few reasons. There are
some cases where we'll suspend for a longer duration, like stylesheets —
it's nearly always a bad idea to show content before its styles have
loaded, so we're going to be extend this timeout to be really long.
But even in the case where the timeout is shorter, like fonts, if you
get a new update, it's possible (even likely) that update will allow us
to avoid showing a fallback, like by navigating to a different page. So
we might as well try.
The behavior now matches our behavior for interrupting a suspended
render phase (i.e. `use`), which makes sense because they're not that
conceptually different.
This flag is already enabled everywhere except for www, which is blocked
by a few tests that assert on the old behavior. Once www is ready, I'll
land this.
This PR has a bunch of surrounding refactoring. See individual commits.
The main change is that we no longer special case `typeof is ===
'string'` as a special case according to the
`enableCustomElementPropertySupport` flag.
Effectively this means that you can't use custom properties/events,
other than the ones React knows about on `<input is="my-input">`
extensions.
This is unfortunate but there's too many paths that are forked in
inconsistent ways since we fork based on tag name. I think __the
solution is to let all React elements set unknown properties/events in
the same way as this flag__ but that's a bigger change than this flag
implies.
Since `is` is not universally supported yet anyway, this doesn't seem
like a huge loss. Attributes still work.
We still support passing the `is` prop and turn that into the
appropriate createElement call.
@josepharhar
This is a step towards getting rid of the meta programming in
DOMProperty and CSSProperty.
This moves isAttributeNameSafe and isUnitlessNumber to a separate shared
modules.
isUnitlessNumber is now a single switch instead of meta-programming.
There is a slight behavior change here in that I hard code a specific
set of vendor-prefixed attributes instead of prefixing all the unitless
properties. I based this list on what getComputedStyle returns in
current browsers. I removed Opera prefixes because they were [removed in
Opera](https://dev.opera.com/blog/css-vendor-prefixes-in-opera-12-50-snapshots/)
itself. I included the ms ones mentioned [in the original
PR](https://github.com/facebook/react/commit/5abcce534382d85887f3d33475e8e54e3b5d8457).
These shouldn't really be used anymore anyway so should be pretty safe.
Worst case, they'll fallback to the other property if you specify both.
Finally I inline the mustUseProperty special cases - which are also the
only thing that uses propertyName. These are really all controlled
components and all booleans.
I'm making a small breaking change here by treating `checked` and
`selected` specially only on the `input` and `option` tags instead of
all tags. That's because those are the only DOM nodes that actually have
those properties but we used to set them as expandos instead of
attributes before. That's why one of the tests is updated to now use
`input` instead of testing an expando on a `div` which isn't a real use
case. Interestingly this also uncovered that we update checked twice for
some reason but keeping that logic for now.
Ideally `multiple` and `muted` should move into `select` and
`audio`/`video` respectively for the same reason.
No change to the attribute-behavior fixture.
This is not really part of the bindings, it's more part of the package
entry points. /shared/ is not really right neither because it's more
like an isomorphic entry point and not some utility.
We currently throw an error when disableJavaScriptURLs is on and trigger
an error boundary. I kind of thought that's what would happen with CSP
or Trusted Types anyway. However, that's not what happens. Instead, in
those environments what happens is that the error is triggered when you
try to actually visit those links. So if you `preventDefault()` or
something it'll never show up and since the error just logs to the
console or to a violation logger, it's effectively a noop to users.
We can simulate the same without CSP by simply generating a different
`javascript:` url that throws instead of executing the potential attack
vector.
This still allows these to be used - at least as long as you
preventDefault before using them in practice. This might be legit for
forms. We still don't recommend using them for links-as-buttons since
it'll be possible to "Open in a New Tab" and other weird artifacts. For
links we still recommend the technique of assigning a button role etc.
It also is a little nicer when an attack actually happens because at
least it doesn't allow an attacker to trigger error boundaries and
effectively deny access to a page.
I use a shared helper when setting properties into a helper whether it's
initial or update.
I moved the special cases per tag to commit phase so we can check it
only once. This also effectively inlines getHostProps which can be done
in a single check per prop key.
The diffProperties operation is simplified to mostly just generating a
plain diff of all properties, generating an update payload. This might
generate a few more entries that are now ignored in the commit phase.
that previously would've been ignored earlier. We could skip this and
just do the whole diff in the commit phase by always scheduling a commit
phase update.
I tested the attribute table (one change documented below) and a few
select DOM fixtures.
I rewrote some of our tests that deal with microtasks with the aim of
making them less coupled to implementation details. This is related to
an upcoming change to move update processing into a microtask.
Added an explicit type to all $FlowFixMe suppressions to reduce
over-suppressions of new errors that might be caused on the same lines.
Also removes suppressions that aren't used (e.g. in a `@noflow` file as
they're purely misleading)
Test Plan:
yarn flow-ci
Prerendering a tree (i.e. with Offscreen) should not suspend the commit
phase, because the content is not yet visible. However, when revealing a
prerendered tree, we should suspend the commit phase if resources in the
prerendered tree haven't finished loading yet.
To do this properly, we need to visit all the visible nodes in the tree
that might possibly suspend. This includes nodes in the current tree,
because even though they were already "mounted", the resources might not
have loaded yet, because we didn't suspend when it was prerendered.
We will need to add this capability to the Offscreen component's
"manual" mode, too. Something like a `ready()` method that returns a
promise that resolves when the tree has fully loaded.
Also includes some fixes to #26450. See PR for details.
Before a commit is finished if any new stylesheet resources are going to
mount and we are capable of delaying the commit we will do the following
1. Wait for all preloads for newly created stylesheet resources to load
2. Once all preloads are finished we insert the stylesheet instances for
these resources and wait for them all to load
3. Once all stylesheets have loaded we complete the commit
In this PR I also removed the synchronous loadingstate tracking in the
fizz runtime. It was not necessary to support the implementation on not
used by the fizz runtime itself. It makes the inline script slightly
smaller
In this PR I also integrated ReactDOMFloatClient with
ReactDOMHostConfig. It leads to better code factoring, something I
already did on the server a while back. To make the diff a little easier
to follow i make these changes in a single commit so you can look at the
change after that commit if helpful
There is a 500ms timeout which will finish the commit even if all
suspended host instances have not finished loading yet
At the moment error and load events are treated the same and we're
really tracking whether the host instance is finished attempting to
load.
## Overview
This PR unfortunately removes the warning emitted when using layout
effects on the server:
> useLayoutEffect does nothing on the server, because its effect cannot
be encoded into the server renderer's output format. This will lead to a
mismatch between the initial, non-hydrated UI and the intended UI. To
avoid this, useLayoutEffect should only be used in components that
render exclusively on the client. See
https://reactjs.org/link/uselayouteffect-ssr for common fixes.
## Why this warning exists
The new docs explain this really well. Adding a screenshot because as
part of this change, we'll be removing these docs.
<img width="1562" alt="Screenshot 2023-03-15 at 10 56 17 AM"
src="https://user-images.githubusercontent.com/2440089/225349148-f0e57c3f-95f5-4f2e-9178-d9b9b221c28d.png">
## Why are we changing it
In practice, users are not just ignoring this warning, but creating
hooks to bypass this warning by switching the useLayoutEffect hook on
the server instead of fixing it. This battle seems to be lost, so let's
remove the warning so at least users don't need to use the indirection
hook any more. In practice, if it's an issue, you should see the
problems like flashing the wrong content on first load in development.
With this flag off, we don't throw and therefore don't patch up the tree
when suppression is off.
Haven't tested.
---------
Co-authored-by: Rick Hanlon <rickhanlonii@fb.com>
Today if something suspends, React will continue rendering the siblings
of that component.
Our original rationale for prerendering the siblings of a suspended
component was to initiate any lazy fetches that they might contain. This
was when we were more bullish about lazy fetching being a good idea some
of the time (when combined with prefetching), as opposed to our latest
thinking, which is that it's almost always a bad idea.
Another rationale for the original behavior was that the render was I/O
bound, anyway, so we might as do some extra work in the meantime. But
this was before we had the concept of instant loading states: when
navigating to a new screen, it's better to show a loading state as soon
as you can (often a skeleton UI), rather than delay the transition.
(There are still cases where we block the render, when a suitable
loading state is not available; it's just not _all_ cases where
something suspends.) So the biggest issue with our existing
implementation is that the prerendering of the siblings happens within
the same render pass as the one that suspended — _before_ the loading
state appears.
What we should do instead is immediately unwind the stack as soon as
something suspends, to unblock the loading state.
If we want to preserve the ability to prerender the siblings, what we
could do is schedule special render pass immediately after the fallback
is displayed. This is likely what we'll do in the future. However, in
the new implementation of `use`, there's another reason we don't
prerender siblings: so we can preserve the state of the stack when
something suspends, and resume where we left of when the promise
resolves without replaying the parents. The only way to do this
currently is to suspend the entire work loop. Fiber does not currently
support rendering multiple siblings in "parallel". Once you move onto
the next sibling, the stack of the previous sibling is discarded and
cannot be restored. We do plan to implement this feature, but it will
require a not-insignificant refactor.
Given that lazy data fetching is already bad for performance, the best
trade off for now seems to be to disable prerendering of siblings. This
gives us the best performance characteristics when you're following best
practices (i.e. hoist data fetches to Server Components or route
loaders), at the expense of making an already bad pattern a bit worse.
Later, when we implement resumable context stacks, we can reenable
sibling prerendering. Though even then the use case will mostly be to
prerender the CPU-bound work, not lazy fetches.
(This was reviewed and approved as part of #26380; I'm extracting it
into its own PR so that it can bisected later if it causes an issue.)
I noticed while working on a PR that when an error happens during
hydration, and we revert to client rendering, React actually does _two_
additional render passes instead of just one. We didn't notice it
earlier because none of our tests happened to assert on how many renders
it took to recover, only on the final output.
It's possible this extra render pass had other consequences that I'm not
aware of, like messing with some assumption in the recoverable errors
logic.
This adds a test to demonstrate the issue. (One problem is that we don't
have much test coverage of this scenario in the first place, which
likely would have caught this earlier.)
This is in line with the refactor I already did on Fizz earlier and
brings Fiber up to a similar structure.
We end up with a lot of extra checks due the extra abstractions we use
to check the various properties. This uses a flatter and more inline
model which makes it easier to see what each property does. The tradeoff
is that a change might need changes in more places.
The general structure is that there's a switch for tag first, then a
switch for each attribute special case, then a switch for the value. So
it's easy to follow where each scenario will end up and there shouldn't
be any unnecessary code executed along the way.
My goal is to eventually get rid of the meta-programming in DOMProperty
and CSSProperty but I'm leaving that in for now - in line with Fizz.
My next step is moving around things a bit in the diff/commit phases.
This is the first step to more refactors for perf and size, but also
because I'm adding more special cases so I need to have a flatter
structure that I can reason about for those special cases.
In concurrent mode we error if child nodes mismatches which triggers a
recreation of the whole hydration boundary. This ensures that we don't
replay the wrong thing, transform state or other security issues.
For text content, we respect `suppressedHydrationWarning` to allow for
things like `<div suppressedHydrationWarning>{timestamp}</div>` to
ignore the timestamp. This mode actually still patches up the text
content to be the client rendered content.
In principle we shouldn't have to do that because either value should be
ok, and arguably it's better not to trigger layout thrash after the
fact.
We do have a lot of code still to deal with patching up the tree because
that's what legacy mode does which is still in the code base. When we
delete legacy mode we would still be stuck with a lot of it just to deal
with this case.
Therefore I propose that we change the semantics to not patch up
hydration errors for text nodes. We already don't for attributes.
Just noticed the test isn't testing what it is meant to test properly.
The error `Warning: ReactDOM.render is no longer supported in React 18.
Use createRoot instead. Until you switch to the new API, your app will
behave as if it's running React 17. Learn more:
https://reactjs.org/link/switch-to-createroot` is thrown, the inner
`expect(error).toContain('Warning: Maximum update depth exceeded.');`
failed and threw jest error, and the outer `.toThrow('Maximum update
depth exceeded.')` happens to catch it and makes the test pass.
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.
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.
## 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.
Jan Kassens
Tianyu Yao
Andrew Clark
Sebastian Markbåge
Josh Story
Ricky
