When enableRefAsProp is on, we should always use the props as the source
of truth for refs. Not a field on the fiber.
In the case of string refs, this presents a problem, because string refs
are not passed around internally as strings; they are converted to
callback refs. The ref used by the reconciler is not the same as the one
the user provided.
But since this is a deprecated feature anyway, what we can do is clone
the props object and replace it with the internal callback ref. Then we
can continue to use the props object as the source of truth.
This means the internal callback ref will leak into userspace. The
receiving component will receive a callback ref even though the parent
passed a string. Which is weird, but again, this is a deprecated
feature, and we're only leaving it around behind a flag so that Meta can
keep using string refs temporarily while they finish migrating their
codebase.
This removes the remaining `propTypes` validation calls, making
declaring `propTypes` a no-op. In other words, React itself will no
longer validate the `propTypes` that you declare on your components.
In general, our recommendation is to use static type checking (e.g.
TypeScript). If you'd like to still run propTypes checks, you can do so
manually, same as you'd do outside React:
```js
import checkPropTypes from 'prop-types/checkPropTypes';
function Button(props) {
checkPropTypes(Button.propTypes, prop, 'prop', Button.name)
// ...
}
```
This could be automated as a Babel plugin if you want to keep these
checks implicit. (We will not be providing such a plugin, but someone in
community might be interested in building or maintaining one.)
## Summary
Cleaning up internal usage of ReactTestRenderer
## How did you test this change?
`yarn test packages/react/src/__tests__/ReactCreateRef-test.js`
## Summary
Cleaning up internal usage of ReactTestRenderer
## How did you test this change?
`yarn test
packages/use-subscription/src/__tests__/useSubscription-test.js`
## Summary
Cleaning up internal usage of ReactTestRenderer
## How did you test this change?
`yarn test
packages/react-reconciler/src/__tests__/ReactSuspense-test.internal.js`
Since this is more about specifically the streaming protocol and I'll
add other dimensions that don't map 1:1.
E.g. some configs need to be the same across all servers.
Depends on:
- #28317
- #28320
---
Changes the behavior of the JSX runtime to pass through `ref` as a
normal prop, rather than plucking it from the props object and storing
on the element.
This is a breaking change since it changes the type of the receiving
component. However, most code is unaffected since it's unlikely that a
component would have attempted to access a `ref` prop, since it was not
possible to get a reference to one.
`forwardRef` _will_ still pluck `ref` from the props object, though,
since it's extremely common for users to spread the props object onto
the inner component and pass `ref` as a differently named prop. This is
for maximum compatibility with existing code — the real impact of this
change is that `forwardRef` is no longer required.
Currently, refs are resolved during child reconciliation and stored on
the fiber. As a result of this change, we can move ref resolution to
happen only much later, and only for components that actually use them.
Then we can remove the `ref` field from the Fiber type. I have not yet
done that in this step, though.
Depends on:
- #28317
---
There's a ton of overlap between the createElement implementation and
the JSX implementation, so I combined them into a single module.
In the actual build output, the shared code between JSX and
createElement will get duplicated anyway, because react/jsx-runtime and
react (where createElement lives) are separate, flat build artifacts.
So this is more about code organization — with a few key exceptions, the
implementations of createElement and jsx are highly coupled.
There are too many layers to the JSX runtime implementation. I think
basically everything should be implemented in a single file, so that's
what I'm going to do.
As a first step, this deletes ReactJSXElementValidator and moves all the
code into ReactJSXElement. I can already see how this will help us
remove more indirections in the future.
Next I'm going to do start moving the `createElement` runtime into this
module as well, since there's a lot of duplicated code.
Certain fiber types may have a ref attached to them. The main ones are
HostComponent and ClassComponent. During the render phase, we check if a
ref was passed to it, and if so, we schedule a Ref effect: `markRef`.
Currently, we're not consistent about whether we call `markRef` in the
begin phase or the complete phase. For some fiber types, I found that
`markRef` was called in both phases, causing redundant work.
After some investigation, I don't believe it's necessary to call
`markRef` in both the begin phase and the complete phase, as long as you
don't bail out before calling `markRef`.
I though that maybe it had to do with the
`attemptEarlyBailoutIfNoScheduledUpdates` branch, which is a fast path
that skips the regular begin phase if no new props, state, or context
were passed. But if the props haven't changed (referentially — the
`memo` and `shouldComponentUpdate` checks happen later), then it follows
that the ref couldn't have changed either. This is true even in the old
`createElement` runtime where `ref` is stored on the element instead of
as a prop, because there's no way to pass a new ref to an element
without also passing new props. You might argue this is a leaky
assumption, but since we're shifting ref to be just a regular prop
anyway, I think it's the correct way to think about it going forward.
I think the pattern of calling `markRef` in the complete phase may have
been left over from an earlier iteration of the implementation before
the bailout logic was structured like it is today.
So, I removed all the `markRef` calls from the complete phase. In the
case of ScopeComponent, which had no corresponding call in the begin
phase, I added one.
We already had a test that asserted that a ref is reattached even if the
component bails out, but I added some new ones to be extra safe.
The reason I'm changing this this is because I'm working on a different
change to move the ref handling logic in `coerceRef` to happen in render
phase of the component that accepts the ref, instead of during the
parent's reconciliation.
This won't ever be serialized and is likely just a mistake.
This should be covered by the "use server" compiler since it ensures
that something that accepts a "this" won't be allowed to compile and if
it doesn't accept it, TypeScript should ideally forbid it to be passed.
So maybe this is unnecessary.
If an error happens before the shell, we need to handle it. In this case
we choose the strategy of rendering a blank document and client
rendering the app. Which will intentionally have a hydration mismatch.
It's possible for the same function instance to appear more than once in
the same graph or even the same file.
Currently this errors on trying to reconfigure the property but it
really doesn't matter which one wins. First or last.
Regardless there will be an entry point generated that can get them.
Alternative to #28354.
If a client reference is one of the props being describes as part of
another error, we call toString on it, which errors.
We should error explicitly when a Symbol prop is extracted.
However, pragmatically I added the toString symbol tag even though we
don't know what the real tostring will be but we also lie about the
typeof.
We can however in addition to this give it a different description
because describing this property as an object isn't quite right.
We probably could extract the export name but that's kind of renderer
specific and I just added this shared module to Fizz which doesn't have
that which is unfortunate an consequence.
For default exports we don't have a good name of what the alias was in
the receiver. Could maybe call it "default" but for now I just call it
"client".
Also warn for symbols.
It's weird because for objects we throw a hard error but functions we do
a dev only check. Mainly because we have an object branch anyway.
In the object branch we have some built-ins that have bad errors like
forwardRef and memo but since they're going to become functions later, I
didn't bother updating those. Once they're functions those names will be
part of this.
We have an unresolved conflict where the Flight client wants to execute
inside Fizz to emit side-effects like preloads (which can be early) into
that stream. However, the FormState API requires the state to be passed
at the root, so if you're getting that through the RSC payload it's a
Catch 22.
#27314 used a hack to mutate the form state array to fill it in later,
but that doesn't actually work because it's not always an array. It's
sometimes null like if there wasn't a POST. This lead to a bunch of
hydration errors - which doesn't have the best error message for this
case neither. It probably should error with something that specifies
that it's form state.
This fixes it by teeing the stream into two streams and consuming it
with two Flight clients. One to read the form state and one to emit
side-effects and read the root.
ErrorBoundaries are currently not fully composable. The reason is if you
decide your boundary cannot handle a particular error and rethrow it to
higher boundary the React runtime does not understand that this throw is
a forward and it recreates the component stack from the Boundary
position. This loses fidelity and is especially bad if the boundary is
limited it what it handles and high up in the component tree.
This implementation uses a WeakMap to store component stacks for values
that are objects. If an error is rethrown from an ErrorBoundary the
stack will be pulled from the map if it exists. This doesn't work for
thrown primitives but this is uncommon and stashing the stack on the
primitive also wouldn't work
The newer version triggers an error due to require() not being compiled.
This was upgraded in #27328. I'm not sure why that was needed. Maybe it
was just because it was allowed by the caret and something else upgraded
but I haven't been able to make it work with the newer version. So I'll
just pin it.
Removes all `propTypes` validation called from outside the JSX
factories. Haven't touched JSX.
Tests that verify related behavior are stripped down to the
non-`propTypes` logic.
The `reference` that is passed into `registerServerReference` can be a
plain function. It does not need to have the three additonal properties
of a `ServerRefeference`. In fact, adding these properties (plus `bind`)
is precisely what `registerServerReference` does.
Same as #28327 but for Fizz.
One thing that's weird about this recoverable error is that we don't
send the regular stack for it, just the component stack it seems. This
is missing some potential information and if we move toward integrated
since stacks it would be one thing.
Also deals with symbols. Alternative to #28312.
We currently always normalize rejections or thrown values into `Error`
objects. Partly because in prod it'll be an error object and you
shouldn't fork behavior on knowing the value outside a digest. We might
want to even make the message always opaque to avoid being tempted and
then discover in prod that it doesn't work.
However, we do include the message in DEV.
If this is a non-Error object we don't know what the properties mean.
Ofc, we don't want to include too much information in the rendered
string, so we use the general `describeObjectForErrorMessage` helper.
Unfortunately it's pretty conservative about emitting values so it's
likely to exclude any embedded string atm. Could potentially expand it a
bit.
We could in theory try to serialize as much as possible and re-throw the
actual object to allow for inspection to be expanded inside devtools
which is what I plan on for consoles, but since we're normalizing to an
Error this is in conflict with that approach.
In preparation for https://github.com/facebook/react/pull/28207.
These tests aren't actually testing propTypes, they just use them to
verify we can display a meaningful component name. We've mostly moved
away from warnings that display component names directly in favor of
component stacks. So let's just replace these with tests asserting the
component names show up in stacks.
Part of https://github.com/facebook/react/pull/28207, this is easy to
land in isolation.
The approach I'm taking is slightly different — instead of leaving
validation on for legacy context, I disable the validation (it's
DEV-only) and leave just the parts that drive the runtime logic. I.e.
`contexTypes` and `childContextTypes` *values* are now ignored, the keys
are used just like before.
This option was added defensively but it's not needed. There's no cost
to including it always.
I suspect this optional was added mainly to avoid needing to update
tests. That's not a reason to have an unnecessary public API though.
We have a praxis for dealing with source location in tests to avoid them
failing tests. I also ported them to inline snapshots so that additions
to the protocol isn't such a pain.
All our sources are considered third party and should be hidden in stack
traces unless expanded. Our internals aren't actionable anyway.
This doesn't really do much without tooling that actually forwards this
to new generated source maps, in which case they probably just add them
to ignorelist anyway.
Andrew Clark
bubucuo
dan
Sebastian Markbåge
Jack Pope
Sebastian Silbermann
Masato Nakamura
Josh Story
Hendrik Liebau
Ricky