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

[Fizz] Don't pop the replay stack if we've already rendered past an element #27513

Merged
merged 1 commit into from
Oct 13, 2023

Conversation

sebmarkbage
Copy link
Collaborator

@sebmarkbage sebmarkbage commented Oct 13, 2023

This is the same problem as we had with keyPath before where if the element itself suspends, we have to restore the replay node to what it was before, however, if something below the element suspends we shouldn't pop it because that will pop it back up the stack.

Instead of passing replay as an argument to every renderElement function, I use a hack to compare if the node is still the same as the one we tried to render, then that means we haven't stepped down into the child yet. Maybe this is not quite correct because in theory you could have a recursive node that just renders itself over and over until some context bails out.

This solves an issue where if you suspended in an element it would retry trying to replay from that element but using the postponed state from the root.

@sebmarkbage sebmarkbage requested review from gnoff and acdlite October 13, 2023 04:47
@facebook-github-bot facebook-github-bot added CLA Signed React Core Team Opened by a member of the React Core Team labels Oct 13, 2023
@react-sizebot
Copy link

Comparing: dddfe68...bb172e3

Critical size changes

Includes critical production bundles, as well as any change greater than 2%:

Name +/- Base Current +/- gzip Base gzip Current gzip
oss-stable/react-dom/cjs/react-dom.production.min.js = 174.46 kB 174.46 kB = 54.27 kB 54.27 kB
oss-experimental/react-dom/cjs/react-dom.production.min.js = 176.31 kB 176.31 kB = 54.88 kB 54.88 kB
facebook-www/ReactDOM-prod.classic.js = 564.48 kB 564.48 kB = 99.37 kB 99.37 kB
facebook-www/ReactDOM-prod.modern.js = 548.21 kB 548.21 kB = 96.44 kB 96.44 kB

Significant size changes

Includes any change greater than 0.2%:

(No significant changes)

Generated by 🚫 dangerJS against bb172e3

@sebmarkbage sebmarkbage merged commit 09fbee8 into facebook:main Oct 13, 2023
2 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 13, 2023
…lement (#27513)

This is the same problem as we had with keyPath before where if the
element itself suspends, we have to restore the replay node to what it
was before, however, if something below the element suspends we
shouldn't pop it because that will pop it back up the stack.

Instead of passing replay as an argument to every renderElement
function, I use a hack to compare if the node is still the same as the
one we tried to render, then that means we haven't stepped down into the
child yet. Maybe this is not quite correct because in theory you could
have a recursive node that just renders itself over and over until some
context bails out.

This solves an issue where if you suspended in an element it would retry
trying to replay from that element but using the postponed state from
the root.

DiffTrain build for [09fbee8](09fbee8)
ztanner added a commit to vercel/next.js that referenced this pull request Oct 16, 2023
…experimental prefix for server action APIs (#56809)

The latest React canary builds have a few changes that need to be
adopted for compatability.

1. the `useFormState` and `useFormStatus` hooks in `react-dom` and the
`formData` opiont in `react-dom/server` are no longer prefixed with
`experimental_`
2. server content (an undocumented React feature) has been removed. Next
only had trivial intenral use of this API and did not expose a coherent
feature to Next users (no ability to seed context on refetches). It is
still possible that some users used the React server context APIs which
is why this should go into Next 14.

### React upstream changes

- facebook/react#27513
- facebook/react#27514
- facebook/react#27511
- facebook/react#27508
- facebook/react#27502
- facebook/react#27474
- facebook/react#26789
- facebook/react#27500
- facebook/react#27488
- facebook/react#27458
- facebook/react#27471
- facebook/react#27470
- facebook/react#27464
- facebook/react#27456
- facebook/react#27462
- facebook/react#27461
- facebook/react#27460
- facebook/react#27459
- facebook/react#27454
- facebook/react#27457
- facebook/react#27453
- facebook/react#27401
- facebook/react#27443
- facebook/react#27445
- facebook/react#27364
- facebook/react#27440
- facebook/react#27436

---------

Co-authored-by: Zack Tanner <[email protected]>
Co-authored-by: kodiakhq[bot] <49736102+kodiakhq[bot]@users.noreply.github.com>
Co-authored-by: Jiachi Liu <[email protected]>
EdisonVan pushed a commit to EdisonVan/react that referenced this pull request Apr 15, 2024
…lement (facebook#27513)

This is the same problem as we had with keyPath before where if the
element itself suspends, we have to restore the replay node to what it
was before, however, if something below the element suspends we
shouldn't pop it because that will pop it back up the stack.

Instead of passing replay as an argument to every renderElement
function, I use a hack to compare if the node is still the same as the
one we tried to render, then that means we haven't stepped down into the
child yet. Maybe this is not quite correct because in theory you could
have a recursive node that just renders itself over and over until some
context bails out.

This solves an issue where if you suspended in an element it would retry
trying to replay from that element but using the postponed state from
the root.
bigfootjon pushed a commit that referenced this pull request Apr 18, 2024
…lement (#27513)

This is the same problem as we had with keyPath before where if the
element itself suspends, we have to restore the replay node to what it
was before, however, if something below the element suspends we
shouldn't pop it because that will pop it back up the stack.

Instead of passing replay as an argument to every renderElement
function, I use a hack to compare if the node is still the same as the
one we tried to render, then that means we haven't stepped down into the
child yet. Maybe this is not quite correct because in theory you could
have a recursive node that just renders itself over and over until some
context bails out.

This solves an issue where if you suspended in an element it would retry
trying to replay from that element but using the postponed state from
the root.

DiffTrain build for commit 09fbee8.
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.

4 participants