Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Flight] Encode React Elements in Replies as Temporary References #28564

Merged
merged 3 commits into from
Mar 19, 2024

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Mar 15, 2024

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.

@sebmarkbage sebmarkbage requested a review from gnoff March 15, 2024 17:45
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Mar 15, 2024
}
// We can serialize class instances as temporary references.
return serializeTemporaryReferenceID(
writeTemporaryReference(temporaryReferences, value),
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Anything we can't serialize, we can still encode as a temporary reference.

Unfortunately, for some things like the stuff below, we only warn in DEV that we can't serialize it so for any of those, we don't put it as a temporary reference.

So any of those are really invalid in both cases.

@unstubbable
Copy link
Collaborator

Lovely PR description! ❤️

// Temporary Reference
const id = parseInt(value.slice(2), 16);
const temporaryReferences = response._tempRefs;
if (temporaryReferences == null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Saw that the type is void | TemporaryReferenceSet and in the other place we're checking with === undefined. Is the == null expected?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I just do it since it's a public API and a user might still pass null incorrectly but really we should probably be more strongly opinionated about not doing that.

pendingParts--;
if (pendingParts === 0) {
resolve(data);
try {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

was this supposed to always be wrapped in try? I don't quite follow why this change required the try now but it didn't before

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I think it was an oversight because the .then reject wouldn't catch the case when a promise's body wasn't able to be serialized and threw.

const resolvedModel = init(payload);
// We always outline this as a separate part even though we could inline it
// because it ensures a more deterministic encoding.
pendingParts++;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

why isn't this lifted above the try and then not re-incremented in the catch? I assume nothing can throw between here and the decrement but if it did we'd end up with a double increment in the catch if it was a thenable.

mostly I'm asking b/c I think you did this on purpose and I want to know why, not that I think it's wrong per se

Copy link
Collaborator Author

@sebmarkbage sebmarkbage Mar 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to keep it incremented when serializing the inner tree since otherwise the inner one can complete if it finishes. There are branches below that doesn't increment so it would require them to remember to decrement.

However I think it's a bug because resolveToJSON can throw in this mode (unlike FlightServer). It's only if it throws promise that it matters though because otherwise we reject the whole thing - and there's nothing that really should support throwing a promise but maybe a Proxy.

Which is a bit strange by itself because it means that we reject creating a payload in this case where as other Flight throws on the receiving side.

This means that if we later shifted this to be streaming (we don't right now mainly because Safari doesn't support it in fetch but also for simplicity), we'd need to change the error handling semantics.

const lazyId = nextPartId++;
const thenable: Thenable<any> = (x: any);
thenable.then(
partValue => {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Writing a test I found another bug. The resolved value of the thrown promise is not the same as the value if you try it again.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since we have to recursively call serialization it doesn't make sense to try to outline these. Which means serializing lazy won't be deterministic unfortunately.

@sebmarkbage sebmarkbage merged commit 83409a1 into facebook:main Mar 19, 2024
37 of 38 checks passed
github-actions bot pushed a commit that referenced this pull request Mar 19, 2024
…8564)

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.

DiffTrain build for [83409a1](83409a1)
sebmarkbage added a commit that referenced this pull request Mar 21, 2024
…eration (#28611)

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.
github-actions bot pushed a commit that referenced this pull request Mar 21, 2024
…eration (#28611)

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.

DiffTrain build for [fee786a](fee786a)
gnoff added a commit to gnoff/next.js that referenced this pull request Mar 25, 2024
- facebook/react#28596
- facebook/react#28625
- facebook/react#28616
- facebook/react#28491
- facebook/react#28583
- facebook/react#28427
- facebook/react#28613
- facebook/react#28599
- facebook/react#28611
- facebook/react#28610
- facebook/react#28606
- facebook/react#28598
- facebook/react#28549
- facebook/react#28557
- facebook/react#28467
- facebook/react#28591
- facebook/react#28459
- facebook/react#28590
- facebook/react#28564
- facebook/react#28582
- facebook/react#28579
- facebook/react#28578
- facebook/react#28521
- facebook/react#28550
- facebook/react#28576
- facebook/react#28577
- facebook/react#28571
- facebook/react#28572
- facebook/react#28560
- facebook/react#28569
- facebook/react#28573
- facebook/react#28546
- facebook/react#28568
- facebook/react#28562
- facebook/react#28566
- facebook/react#28565
- facebook/react#28559
- facebook/react#28508
- facebook/react#20432
- facebook/react#28555
- facebook/react#24730
- facebook/react#28472
- facebook/react#27991
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
gnoff added a commit to gnoff/next.js that referenced this pull request Mar 25, 2024
- facebook/react#28596
- facebook/react#28625
- facebook/react#28616
- facebook/react#28491
- facebook/react#28583
- facebook/react#28427
- facebook/react#28613
- facebook/react#28599
- facebook/react#28611
- facebook/react#28610
- facebook/react#28606
- facebook/react#28598
- facebook/react#28549
- facebook/react#28557
- facebook/react#28467
- facebook/react#28591
- facebook/react#28459
- facebook/react#28590
- facebook/react#28564
- facebook/react#28582
- facebook/react#28579
- facebook/react#28578
- facebook/react#28521
- facebook/react#28550
- facebook/react#28576
- facebook/react#28577
- facebook/react#28571
- facebook/react#28572
- facebook/react#28560
- facebook/react#28569
- facebook/react#28573
- facebook/react#28546
- facebook/react#28568
- facebook/react#28562
- facebook/react#28566
- facebook/react#28565
- facebook/react#28559
- facebook/react#28508
- facebook/react#20432
- facebook/react#28555
- facebook/react#24730
- facebook/react#28472
- facebook/react#27991
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…cebook#28564)

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.
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…eration (facebook#28611)

This a follow up to facebook#28564. It's alternative to facebook#28609 which takes
facebook#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 facebook#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 facebook#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.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
…8564)

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.

DiffTrain build for commit 83409a1.
eps1lon added a commit to vercel/next.js that referenced this pull request Apr 19, 2024
### React upstream changes

- facebook/react#28643
- facebook/react#28628
- facebook/react#28361
- facebook/react#28513
- facebook/react#28299
- facebook/react#28617
- facebook/react#28618
- facebook/react#28621
- facebook/react#28614
- facebook/react#28596
- facebook/react#28625
- facebook/react#28616
- facebook/react#28491
- facebook/react#28583
- facebook/react#28427
- facebook/react#28613
- facebook/react#28599
- facebook/react#28611
- facebook/react#28610
- facebook/react#28606
- facebook/react#28598
- facebook/react#28549
- facebook/react#28557
- facebook/react#28467
- facebook/react#28591
- facebook/react#28459
- facebook/react#28590
- facebook/react#28564
- facebook/react#28582
- facebook/react#28579
- facebook/react#28578
- facebook/react#28521
- facebook/react#28550
- facebook/react#28576
- facebook/react#28577
- facebook/react#28571
- facebook/react#28572
- facebook/react#28560
- facebook/react#28569
- facebook/react#28573
- facebook/react#28546
- facebook/react#28568
- facebook/react#28562
- facebook/react#28566
- facebook/react#28565
- facebook/react#28559
- facebook/react#28508
- facebook/react#20432
- facebook/react#28555
- facebook/react#24730
- facebook/react#28472
- facebook/react#27991
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
- facebook/react#28514
- facebook/react#28548
- facebook/react#28526
- facebook/react#28515
- facebook/react#28533
- facebook/react#28532
- facebook/react#28531
- facebook/react#28407
- facebook/react#28522
- facebook/react#28538
- facebook/react#28509
- facebook/react#28534
- facebook/react#28527
- facebook/react#28528
- facebook/react#28519
- facebook/react#28411
- facebook/react#28520
- facebook/react#28518
- facebook/react#28493
- facebook/react#28504
- facebook/react#28499
- facebook/react#28501
- facebook/react#28496
- facebook/react#28471
- facebook/react#28351
- facebook/react#28486
- facebook/react#28490
- facebook/react#28488
- facebook/react#28468
- facebook/react#28321
- facebook/react#28477
- facebook/react#28479
- facebook/react#28480
- facebook/react#28478
- facebook/react#28464
- facebook/react#28475
- facebook/react#28456
- facebook/react#28319
- facebook/react#28345
- facebook/react#28337
- facebook/react#28335
- facebook/react#28466
- facebook/react#28462
- facebook/react#28322
- facebook/react#28444
- facebook/react#28448
- facebook/react#28449
- facebook/react#28446
- facebook/react#28447
- facebook/react#24580
unstubbable added a commit to unstubbable/react that referenced this pull request Jul 31, 2024
This playwright test is using the Flight Fixture to demonstrate the
Flight Reply equivalent of the scenario that was fixed in facebook#30528 for the
Flight Client.

It's basically an advanced case of what was outlined in facebook#28564,
returning a client component from a server action that is used in
`useActionState`. In addition, the client component uses another element
twice, which leads to the element's props being deduped. Resolving those
references needs to be handled specifically, both in the Flight Client
(done in facebook#30528), as well as in the temporary references of the Flight
Reply Client (and possibly Flight Reply Server?).

The test should probably be converted into a unit test, e.g. in
`packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMBrowser-test.js`
or
`packages/react-server-dom-webpack/src/__tests__/ReactFlightDOMReply-test.js`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed React Core Team Opened by a member of the React Core Team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants