[Flight] model halting as never delivered chunks #30740
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
facebook-github-bot
added
CLA Signed
React Core Team
gnoff
force-pushed
the
halt-calls-onerror
branch
from
19e025f to
73954db
Compare
gnoff
force-pushed
the
halt-calls-onerror
branch
from
73954db to
5a1e378
Compare
| const abortableTasks = request.abortableTasks; | ||
| // We have tasks to abort. We'll emit one error row and then emit a reference | ||
| // to that row from every row that's still remaining. | ||
| if (abortableTasks.size > 0) { | ||
| request.status = ABORTING; | ||
| request.pendingChunks++; | ||
| const errorId = request.nextChunkId++; | ||
| request.fatalError = errorId; |
There was a problem hiding this comment.
Everything from here down is the same between abort/halt. So it seems like you could just have one big utility function that does the rest instead of splitting it into various smaller helpers that might be early abstraction.
There was a problem hiding this comment.
But really if we did encode Halt like we do Postpone as a separate row then they would be even more similar.
gnoff
force-pushed
the
halt-calls-onerror
branch
from
5a1e378 to
615833a
Compare
gnoff
force-pushed
the
halt-calls-onerror
branch
2 times, most recently
from
8e9c567 to
bb2eb26
Compare
| onAllReady: () => void, | ||
| onFatalError: mixed => void, | ||
| onAllReady: void | (() => void), | ||
| onFatalError: void | (mixed => void), |
There was a problem hiding this comment.
This doesn't follow the pattern of the others and should definitely not use void as the empty value since that can cause different hidden classes. We always use null.
But I think in this case we should probably just stick to the pattern and assign a noop so we don't have to check later.
There was a problem hiding this comment.
Notably there's opportunity when the passed in options get compiled that it can just compile it out to a known value. Either if it's always passed or if it is never passed.
There was a problem hiding this comment.
Which means that we probably need to add another field then.
gnoff
force-pushed
the
halt-calls-onerror
branch
from
bb2eb26 to
3e9c0cf
Compare
gnoff
changed the title
gnoff
force-pushed
the
halt-calls-onerror
branch
6 times, most recently
from
6a6ec1b to
6340525
Compare
| // When prerendering with halt semantics we omit the referred to error. | ||
| request.pendingChunks++; | ||
| emitErrorChunk(request, errorId, digest, error); | ||
| } | ||
| } | ||
| abortableTasks.forEach(task => abortTask(task, request, errorId)); |
There was a problem hiding this comment.
When halting, we should not replace the existing ids to point to a new id but it should just leave the original pending id intact and never emit anything for it. Because it is resumable from that id. In other words, we should never generate the errorId in this branch.
gnoff
force-pushed
the
halt-calls-onerror
branch
from
6340525 to
3fd8dcb
Compare
We've refined the model of halting a prerender. Now when you abort during a prerender we simply omit the rows that would complete the flight render. This is analagous to prerendering in Fizz where you must resume the prerender to actually result in errors propagating in the postponed holes. We don't have a resume yet for flight and it's not entirely clear how that will work however the key insight here is that deciding whether the never resolving rows are an error or not should really be done on the consuming side rather than in the producer. This PR also reintroduces the logs for the abort error/postpone when prerendering which will give you some indication that something wasn't finished when the prerender was aborted.
gnoff
force-pushed
the
halt-calls-onerror
branch
from
3fd8dcb to
cf8d1d1
Compare
**breaking change for canary users: Bumps peer dependency of React from `19.0.0-rc-1eaccd82-20240816` to `19.0.0-rc-eb3ad065-20240822`** No changes required in Next.js it seems. [diff facebook/react@1eaccd82...eb3ad065](facebook/react@1eaccd8...eb3ad06) <details> <summary>React upstream changes</summary> - facebook/react#30761 - facebook/react#30779 - facebook/react#30775 - facebook/react#30770 - facebook/react#30756 - facebook/react#30755 - facebook/react#30768 - facebook/react#30760 - facebook/react#30732 - facebook/react#30757 - facebook/react#30750 - facebook/react#30751 - facebook/react#30753 - facebook/react#30740 - facebook/react#30748 - facebook/react#30746 - facebook/react#30747 - facebook/react#30731 - facebook/react#30725 - facebook/react#30741 - facebook/react#30730 - facebook/react#30726 - facebook/react#30717 - facebook/react#30729 - facebook/react#30721 - facebook/react#30720 - facebook/react#30705 </details>
stacked on: #30731
We've refined the model of halting a prerender. Now when you abort during a prerender we simply omit the rows that would complete the flight render. This is analagous to prerendering in Fizz where you must resume the prerender to actually result in errors propagating in the postponed holes. We don't have a resume yet for flight and it's not entirely clear how that will work however the key insight here is that deciding whether the never resolving rows are an error or not should really be done on the consuming side rather than in the producer.
This PR also reintroduces the logs for the abort error/postpone when prerendering which will give you some indication that something wasn't finished when the prerender was aborted.