This a follow up to #28564. It's alternative to #28609 which takes
#28610 into account.
It used to be possible to return JSX from an action with
`useActionState`.
```js
async function action(errors, payload) {
"use server";
try {
...
} catch (x) {
return <div>Error message</div>;
}
}
```
```js
const [errors, formAction] = useActionState(action);
return <div>{errors}</div>;
```
Returning JSX from an action is itself not anything problematic. It's
that it also has to return the previous state to the action reducer
again that's the problem. When this happens we accidentally could
serialize an Element back to the server.
I fixed this in #28564 so it's now blocked if you don't have a temporary
reference set.
However, you can't have that for the progressive enhancement case. The
reply is eagerly encoded as part of the SSR render. Typically you
wouldn't have these in the initial state so the common case is that they
show up after the first POST back that yields an error and it's only in
the no-JS case where this happens so it's hard to discover.
As noted in #28609 there's a security implication with allowing elements
to be sent across this kind of payload, so we can't just make it work.
When an error happens during SSR our general policy is to try to recover
on the client instead. After all, SSR is mainly a perf optimization in
React terms and it's not primarily intended for a no JS solution.
This PR takes the approach that if we fail to generate the progressive
enhancement payload. I.e. if the serialization of previous state /
closures throw. Then we fallback to the replaying semantics just client
actions instead which will succeed.
The effect of this is that this pattern mostly just works:
- First render in the typical case doesn't have any JSX in it so it just
renders a progressive enhanced form.
- If JS fails to hydrate or you click early we do a form POST. If that
hits an error and it tries to render it using JSX, then the new page
will render successfully - however this time with a Replaying form
instead.
- If you try to submit the form again it'll have to be using JS.
Meaning if you use JSX as the error return value of form state and you
make a first attempt that fails, then no JS won't work because either
the first or second attempt has to hydrate.
We have ideas for potentially optimizing away serializing unused
arguments like if you don't actually use previous state which would also
solve it but it wouldn't cover all cases such as if it was deeply nested
in complex state.
Another approach that I considered was to poison the prev state if you
passed an element back but let it through to the action but if you try
to render the poisoned value, it wouldn't work. The downside of this is
when to error. Because in the progressive enhancement case it wouldn't
error early but when you actually try to invoke it at which point it
would be too late to fallback to client replaying. It would probably
have to always error even on the client which is unfortunate since this
mostly just works as long as it hydrates.
---
Thanks to @josephsavona for finding this bug. This is another example of why we
really want hir-everywhere.
Forget output currently nullthrows because we believe `obj.a` is run
unconditionally in source (missing the break/returns out of this scope)
I haven't debugged to understand exactly why this pattern fails, but there are a
few instances of this internally. It's especially weird because
```javascript
// @enableAssumeHooksFollowRulesOfReact
@enableTransitivelyFreezeFunctionExpressions
function Component(props) {
const [_state, setState] = useState();
const a = () => {
return b();
};
const b = () => {
return (
<>
<div onClick={() => onClick(true)} />
<div onClick={() => onClick(false)} /> // <---- only repros if there's a second
call!
</>
);
};
const onClick = (value) => {
setState(value);
};
return <div>{a()}</div>;
}
```
Here, if `b()` only had one nested function expression that called `onClick` it
would work. Also, if we disable `@enableTransitivelyFreezeFunctionExpressions`
then it works.
But the combination of multiple calls plus that mode causes "context variables
are always mutable". I'm guessing we're freezing `onClick` twice and the second
time reports an error since it calls `setState`.
We don't have a `DestructureContext` equivalent of `StoreContext`, so variables
that are declared via destructuring and later reassigned trigger the invariant
that all mentions of a variable must be consistently local or context. The next
PR adds a todo for this case.
We inadvertently think the type annotation on the function expression param is
an identifier and create a LoadLocal for it, which fails. This happens to trip
up on the InferReferenceEffects initialization check, which we had assumed would
only fire for invalid hoisting cases (hence the specific error message).
When PruneMaybeThrows removes maybe-throw terminals, it's possible that the
block in question reassigned a value s.t. it appears as a later phi operand.
That phi has to be rewritten to reflect the updated predecessor block.
Here we track these rewrites (transitively) and rewrite phi operands
accordingly.
As mentioned in #28609 there's a potential security risk if you allow a
passed value to the server to spoof Elements because it allows a hacker
to POST cross origin. This is only an issue if your framework allows
this which it shouldn't but it seems like we should provide an extra
layer of security here.
```js
function action(errors, payload) {
try {
...
} catch (x) {
return [newError].concat(errors);
}
}
```
```js
const [errors, formAction] = useActionState(action);
return <div>{errors}</div>;
```
This would allow you to construct a payload where the previous "errors"
set includes something like `<script src="danger.js" />`.
We could block only elements from being received but it could
potentially be a risk with creating other React types like Context too.
We use symbols as a way to securely brand these.
Most JS don't use this kind of branding with symbols like we do. They're
generally properties which we don't support anyway. However in theory
someone else could be using them like we do. So in an abundance of
carefulness I just ban all symbols from being passed (except by
temporary reference) - not just ours.
This means that the format isn't fully symmetric even beyond just React
Nodes.
#28611 allows code that includes symbols/elements to continue working
but may have to bail out to replaying instead of no JS sometimes.
However, you still can't access the symbols inside the server - they're
by reference only.
Since it was first implemented renderToStaticNodeStream never correctly
set the renderer state to mark the output as static markup which means
it was functionally the same as renderToNodeStream. This change fixes
this oversight. While we are removing renderToNodeStream in a future
version we never did deprecate the static version of this API because it
has no immediate analog in the modern APIs.
- Make all test cases in ReactLazy use RTR with concurrent root
- Except, two cases with "legacy mode" specified in description. These
are moved to a separate description block where the disableLegacyMode
flag is turned off to allow RTR to use legacy root after
https://github.com/facebook/react/pull/28498
Found when running the compiler on a large swath of internal code.
PruneMaybeThrows rewrites terminals, but the logic to update subsequent phis was
incorrectly dropping phis rather than rewriting them. Fixed in the next PR.
---
Reusing optionalMemberExpression nodes recently led to a bug when compiling
Forget playground.
```js
// the two a?.b's here should be different nodes!
if (a?.b !== $[0]) {
// ...
$[0] = a?.b;
}
```
Forget playground uses `babel-plugin-react-forget` and `next/babel`. Reusing the
same node in two positions in the AST lead to invalid mutations:
- the first `a?.b` is visited and transpiled to `a === void 0 ? ...`, which (1)
inserts nodes between the original node and its parent and (2) mutates `a?.b` in
place to a non-optional call
- the second `a?.b` in source gets updated to `a.b` and does not get visited
again
```js
// Source in `EditorImpl.tsx`
compilerOutput.kind === "err" ? compilerOutput.error.details : []
// Forget transformed:
if ($[2] !== compilerOutput.kind || $[3] !== compilerOutput.error?.details) {
t4 = compilerOutput.kind === "err" ? compilerOutput.error.details : [];
$[2] = compilerOutput.kind;
// this is good!
$[3] = compilerOutput.error?.details;
$[4] = t4;
} else {
t4 = $[4];
}
// After next/babel
if ($[2] !== compilerOutput.kind || $[3] !== ((_compilerOutput$error =
compilerOutput.error) === null || _compilerOutput$error === void 0 ? void 0 :
_compilerOutput$error.details)) {
t4 = compilerOutput.kind === "err" ? compilerOutput.error.details : [];
$[2] = compilerOutput.kind;
// Oh no!!
$[3] = _compilerOutput$error.details;
$[4] = t4;
} else {
t4 = $[4];
}
```
---
This should make it easier to grep through error diagnostics to understand state
of the codebase:
- no matching dependences -> likely that source is ignoring eslint failures
- differences in ref.current access -> non-backwards compatible refs
- subpath -> should be fixable in the compiler (unless source is ignore eslint
failures)
---
Previously (in #2663), we check that inferred dependencies exactly match source
dependencies. As @validatePreserveExistingMemoizationGuarantees checks that
Forget output does not invalidate *any more* than source, we now allow more
specific inferred dependencies when neither the inferred dep or matching source
dependency reads into a ref (based on `.current` access).
See added comments for more details
If a global error event is dispatched during a test, Jest reports that
test as a failure.
Our `@gate` pragma feature should account for this — if the gate
condition is false, and the global error event is dispatched, then the
test should be reported as a success.
The solution is to install an error event handler right before invoking
the test function. Because we install our own handler, Jest will not
report the test as a failure if a global error event is dispatched; it's
conceptually as if we wrapped the whole test event in a try-catch.
I need to do more debugging to figure out exactly why the example earlier fails
— but whatever it is, it's clearly a matter of the fbt plugin relying on some
specifics of source locations.
Here we just detect multiple instances of `<fbt:enum>` within a given `<fbt>`
tag and throw a todo.
<img width="553" alt="Screenshot 2024-03-19 at 4 41 15 PM"
src="https://github.com/facebook/react-forget/assets/6425824/e87ee704-6c67-4e10-824b-71e97e7e19f5">
Slightly improves source locations for JSX elements so that the opening and
closing tag have distinct locations that match up with source. The identifier
itself within the closing tag still has the wrong location, but at least this is
an improvement.
Doesn't fix the fbt thing but it was worth a try.
Fbt enums appear to rely on source locations and something that we're doing
(maybe destructuring?) isn't preserving locations such that the fbt plugin
breaks.
Fbt violates the JSX spec by using a lowercase function as a tagname, even
though lowercase names are reserved for builtins. Here we detect cases where
there is an `<fbt>` tag where `fbt` is a local identifier and throw a todo.
Also apply content hash for experimental files
In #28582 I missed that experimental files have a copy of this build
function setting the version strings.
Currently you can accidentally pass React Element to a Server Action. It
warns but in prod it actually works because we can encode the symbol and
otherwise it's mostly a plain object. It only works if you only pass
host components and no function props etc. which makes it potentially
error later. The first thing this does it just early hard error for
elements.
I made Lazy work by unwrapping though since that will be replaced by
Promises later which works.
Our protocol is not fully symmetric in that elements flow from Server ->
Client. Only the Server can resolve Components and only the client
should really be able to receive host components. It's not intended that
a Server can actually do something with them other than passing them to
the client.
In the case of a Reply, we expect the client to be stateful. It's
waiting for a response. So anything we can't serialize we can still pass
by reference to an in memory object. So I introduce the concept of a
TemporaryReferenceSet which is an opaque object that you create before
encoding the reply. This then stashes any unserializable values in this
set and encode the slot by id. When a new response from the Action then
returns we pass the same temporary set into the parser which can then
restore the objects. This lets you pass a value by reference to the
server and back into another slot.
For example it can be used to render children inside a parent tree from
a server action:
```
export async function Component({ children }) {
"use server";
return <div>{children}</div>;
}
```
(You wouldn't normally do this due to the waterfalls but for advanced
cases.)
A common scenario where this comes up accidentally today is in
`useActionState`.
```
export function action(state, formData) {
"use server";
if (errored) {
return <div>This action <strong>errored</strong></div>;
}
return null;
}
```
```
const [errors, formAction] = useActionState(action);
return <div>{errors}<div>;
```
It feels like I'm just passing the JSX from server to client. However,
because `useActionState` also sends the previous state *back* to the
server this should not actually be valid. Before this PR this actually
worked accidentally. You get a DEV warning but it used to work in prod.
Once you do something like pass a client reference it won't work tho. We
could perhaps make client references work by stashing where we got them
from but it wouldn't work with all possible JSX.
By adding temporary references to the action implementation this will
work again - on the client. It'll also be more efficient since we don't
send back the JSX content that you shouldn't introspect on the server
anyway.
However, a flaw here is that the progressive enhancement of this case
won't work because we can't use temporary references for progressive
enhancement since there's no in memory stash. What is worse is that it
won't error if you hydrate. ~It also will error late in the example
above because the first state is "undefined" so invoking the form once
works - it errors on the second attempt when it tries to send the error
state back again.~ It actually errors on the first invocation because we
need to eagerly serialize "previous state" into the form. So at least
that's better.
I think maybe the solution to this particular pattern would be to allow
JSX to serialize if you have no temporary reference set, and remember
client references so that client references can be returned back to the
server as client references. That way anything you could send from the
server could also be returned to the server. But it would only deopt to
serializing it for progressive enhancement. The consequence of that
would be that there's a lot of JSX that might accidentally seem like it
should work but it's only if you've gotten it from the server before
that it works. This would have to have pair them somehow though since
you can't take a client reference from one implementation of Flight and
use it with another.
The example earlier in the stack had unreachable code in the output because
there was an unnecessary memoization block around an assignment. This was a
holdover from before we moved the logic to expand mutable ranges for phis from
LeaveSSA to InferMutableRanges. We were conservatively assigning a mutable range
to all variables with a phi, even those that didn't strictly need one.
Removing the range extension logic in LeaveSSA fixed the issue, but uncovered
the fact that AlignReactiveScopesToBlockScopes was missing a case to handle
optionals.
## Test Plan
Synced internally and ran a snapshot/comparison of compilation before/after
(P1197734337 for those curious). The majority of components get fewer memo slots
thanks to not needing to memoize non-allocating value block expressions like
ternaries/optionals. In a few cases, the fact that we're no longer assigning a
mutable range for value blocks (unless there is actually a mutation!) means we
get more fine-grained memoization and increase the number of memoization blocks.
So overall this appears to be correct, improve memoization, and reduce code
size.
Extracts a helper from the repro earlier in the stack into a helper in
shared-runtime. This makes it easy to verify that memoization is actually
working.
This case is specific to early return inside an inlined IIFE (which can often
occur as a result of dropping manual memoization). When we inline IIFEs, as a
reminder we wrap the body in a labeled block and convert returns to assignment
of a temporary + break out of the label.
Those reassignments themselves are getting a reactive scope assigned since the
reassigned value has a mutable range. They don't really need a mutable range or
scope, though. And then the presence of the `break` statements means that we can
sometimes exit out of the scope before reaching the end - leading to unreachable
code.
This can only occur though where _all the values are already memoized_. So the
code works just fine and even memoizes just fine - it's just that we have some
extraneous scopes and there is technically unreachable code. I'll fix in a
follow-up, adding a repro here.
With this change, the different files in RN will have *different*
hashes. This replaces the git hash and means that the file content
(including version) is only updated when the rest of the file content
actually changes. This should remove "noop" changes that need to be
synced that only update the version string.
A difference to the www implementation here is (and I'd be looking at
updating www as well if this lands well) that each file has an
individual hash instead of a combined content hash. This further reduces
the number of updated files and I couldn't find a reason we need to have
these in sync. The best I can gather is that this hash is used so folks
don't directly compare version string and make future updates harder.
## Summary
We need to unblock flipping the default for RTR to be concurrent
rendering. Update ReactART-test to use `unstable_isConcurrent` in place.
## How did you test this change?
`yarn test packages/react-art/src/__tests__/ReactART-test.js`
of dependencies from source
---
`validatePreserveExistingMemoizationGuarantees` previously checked
- manual memoization dependencies and declarations (the returned value) do not
"lose" memoization due to inferred mutations
```
function useFoo() {
const y = {};
// bail out because we infer that y cannot be a dependency of x as its
mutableRange
// extends beyond
const x = useMemo(() => maybeMutate(y), [y]);
// similarly, bail out if we find that x or y are mutated here
return x;
}
```
- manual memoization deps and decls do not get deopted due to hook calls
```
function useBar() {
const x = getArray();
useHook();
mutate(x);
return useCallback(() => [x], [x]);
}
```
This PR updates `validatePreserveExistingMemoizationGuarantees` with the
following correctness conditions:
*major change* All inferred dependencies of reactive scopes between
`StartMemoize` and `StopMemoize` instructions (e.g. scopes containing manual
memoization code) must either:
1. be produced from earlier within the same manual memoization block
2. exactly match an element of depslist from source
This assumes that the source codebase mostly follows the `exhaustive-deps` lint
rule, which ensures that deps lists are (1) simple expressions composing of
reads from named identifiers + property loads and (2) exactly match deps usages
in the useMemo/useCallback itself.
---
Validated that this does not change source by running internally on ~50k files
(no validation on `main`, no validation on this PR, and validation on this PR).
## Summary
Creates a new `alwaysThrottleDisappearingFallbacks` feature flag that
gates the changes from https://github.com/facebook/react/pull/26802
(instead of being controlled by `alwaysThrottleRetries`). The values of
this new flag mirror the current values of `alwaysThrottleRetries` such
that there is no behavior difference.
This additional feature flag allows us to incrementally validate the
change (arguably bug fix) from
https://github.com/facebook/react/pull/26802 independently from
`alwaysThrottleRetries`.
## How did you test this change?
```
$ yarn test
$ yarn flow dom-browser
$ yarn flow dom-fb
$ yarn flow fabric
```
---
Previously, we always emitted `Memoize dep` instructions after the function
expression literal and depslist instructions
```js
// source
useManualMemo(() => {...}, [arg])
// lowered
$0 = FunctionExpression(...)
$1 = LoadLocal (arg)
$2 = ArrayExpression [$1]
$3 = Memoize (arg)
$4 = Call / LoadLocal
$5 = Memoize $4
```
Now, we insert `Memoize dep` before the corresponding function expression
literal:
```js
// lowered
$0 = StartMemoize (arg) <---- this moved up!
$1 = FunctionExpression(...)
$2 = LoadLocal (arg)
$3 = ArrayExpression [$2]
$4 = Call / LoadLocal
$5 = FinishMemoize $4
```
Design considerations:
- #2663 needs to understand which lowered instructions belong to a manual
memoization block, so we need to emit `StartMemoize` instructions before the
`useMemo/useCallback` function argument, which contains relevant memoized
instructions
- we choose to insert StartMemoize instructions to (1) avoid unsafe instruction
reordering of source and (2) to ensure that Forget output does not change when
enabling validation
This PR only renames `Memoize` -> `Start/FinishMemoize` and hoists
`StartMemoize` as described. The latter may help with stricter validation for
`useCallback`s, although testing is left to the next PR.
#2663 contains all validation changes
Mofei Zhang
Jan Kassens
Sebastian Markbåge
Joe Savona
Josh Story
Jack Pope
Sebastian SilbermannandAndrew Clark
dependabot[bot]
Sathya Gunasekaran
Timothy Yung