[Fizz] implement onHeaders and headersLengthHint options
#27641
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
facebook-github-bot
added
CLA Signed
React Core Team
gnoff
force-pushed
the
fizz-on-headers
branch
from
c8dcce2
to
18c3ec1
Compare
sebmarkbage
reviewed
Nov 7, 2023
| @@ -60,6 +63,8 @@ type Options = { | |||
| unstable_externalRuntimeSrc?: string | BootstrapScriptDescriptor, | |||
| importMap?: ImportMap, | |||
| formState?: ReactFormState<any, any> | null, | |||
| onHeaders?: (headers: HeadersDescriptor) => void, | |||
| headersLengthHint?: number, | |||
There was a problem hiding this comment.
Hints seems pretty vague. Can't I specify a max? maxHeadersLength?
There was a problem hiding this comment.
It's not a max because I don't want to compute the next headers length before deciding if it is going to be a header. I chose hint b/c I want it to communicate that this number is a target not a hard limit. You'll get a header object "near" this length. Also it's already a little imprecise b/c it's code unit length which can be 1x to 4x in byte length when sent via utf8. My guess is the average english website will have a byte length around close to 1x the headers length but in other character sets the headers are going to be longer
There was a problem hiding this comment.
I don't think HTTP headers really support utf8 in practice. Traditionally it was ISO-8859-1 but in practice only the ASCII subset should be used. So either we assume that the URLs have already been encoded or we should arguably encode the URLs ourselves otherwise.
sebmarkbage
reviewed
Nov 7, 2023
| request.completedRootSegment === null | ||
| ? request.pendingRootTasks === 0 | ||
| : request.completedRootSegment.status !== POSTPONED; | ||
| emitEarlyPreloads(request.renderState, request.resumableState, shellComplete); |
There was a problem hiding this comment.
This can potentially happen a lot when you drain a queue often. Unfortunate to add all these extra checks here.
Additionally, if this is happening in a drain event, you might be in a bad context to do something about it. Other other callback tend to be top of stack like scheduled work.
This is also impossible in all APIs but Node.
I wonder if we should just move this into its own function for Node or something.
sebmarkbage
reviewed
Nov 7, 2023
| // I extracted this function out because we want to ensure we consistently emit preloads before | ||
| // transitioning to the next request stage and this transition can happen in multiple places in this | ||
| // implementation. | ||
| function completeWork(request: Request) { |
There was a problem hiding this comment.
It's a little confusing that this name doesn't have anything to do with the same function name in Fiber.
There was a problem hiding this comment.
completeAll is analagous to onAllReady. similar to completeShell for onShellReady
sebmarkbage
reviewed
Nov 7, 2023
| referrerPolicy: props.refererPolicy, | ||
| }); | ||
| headers.highImagePreloads += header; | ||
| headers.remainingCapacity -= header.length; |
There was a problem hiding this comment.
If this ends up exceeding capacity, can't we just throw it away and and take the other path instead?
So we don't need the hacky "hint" which may or may not be fully honored.
There was a problem hiding this comment.
yeah, i suppose you only pay the cost once so we can do the limit thing. But per my other comment the limit is still quite imprecise
sebmarkbage
reviewed
Nov 7, 2023
| @@ -101,6 +103,12 @@ export function prepareHostDispatcher() { | |||
| ReactDOMCurrentDispatcher.current = ReactDOMServerDispatcher; | |||
| } | |||
|
|
|||
| export type HeadersDescriptor = { | |||
| Link: string, | |||
There was a problem hiding this comment.
Maybe this field should be optional just to indicate that in the future we might not include it in every call. Given that this type will be reflected in public types.
sebmarkbage
reviewed
Nov 7, 2023
| // This escaping function is only safe to use for href values being written into | ||
| // a "Link" header in between `<` and `>` characters. The primary concern with the href is | ||
| // to escape the bounding characters as well as new lines. This is unsafe to use in any other | ||
| // context |
There was a problem hiding this comment.
Is tabs a concern here?
There was a problem hiding this comment.
From my research no, it is specifically \r\n. However this is what the spec says, it's possible there are clients in the wild that are more permissive in what breaks out of the href parsing context
sebmarkbage
reviewed
Nov 7, 2023
| // This is hacky but we always append headers into each queue assuming it is a subsequent | ||
| // header. This means that if the string has any length at all it will start with ", " | ||
| // so we truncate it here. This is ugly but the alternatives are to use arrays and join | ||
| // or to length check the queue each time we write to it. This is the most efficient option |
There was a problem hiding this comment.
Well, this will always flatten the ropes and clone it I think so might not be so good. We should just check each time before we add. Since it's interned if (linkHeader !== '') linkHeader += ', '; might be fastest.
gnoff
force-pushed
the
fizz-on-headers
branch
from
18c3ec1
to
4c3bcb6
Compare
gnoff
force-pushed
the
fizz-on-headers
branch
from
4c3bcb6
to
f4899fd
Compare
sebmarkbage
approved these changes
Nov 7, 2023
gnoff
force-pushed
the
fizz-on-headers
branch
2 times, most recently
from
74128be
to
6523996
Compare
During a render it can be useful to emit headers to start the process of preloading without waiting for the shell to complete. If provided an `onHeadersReady` callback option Fizz will call this callback with some headers. Currently this is implemented as a set of preload link headers that are generated during the first performWork pass. After this pass preloads are handled as they were preious to this change and emitted as tags. Since headers are not streamed typically we are choosing an API that provides headers once. Because of this we need to choose a point at which to provide headers once even if we could in theory wait longer to accumulate more. It is possible that a better API is to allow the caller to tell Fizz when it wants whatever headers have accumulated however in practice this would require having some secondary header outside of react that is blocking sending headers and that is considered not a likely ocurrence In this commit we add the option to pass `onHeadersReady` to static and dynamic render entrypoints. This is not yet wired up to anything so it will never be called. This will be added in a subsequent commit.
gnoff
force-pushed
the
fizz-on-headers
branch
from
6523996
to
4140b11
Compare
github-actions bot
pushed a commit
that referenced
this pull request
Nov 7, 2023
Adds a new option to `react-dom/server` entrypoints.
`onHeaders: (headers: Headers) => void` (non node envs)
`onHeaders: (headers: { Link?: string }) => void` (node envs)
When any `renderTo...` or `prerender...` function is called and this
option is provided the supplied function will be called sometime on or
before completion of the render with some preload link headers.
When provided during a `renderTo...` the callback will usually be called
after the first pass at work. The idea here is we want to get a set of
headers to start the browser loading well before the shell is ready. We
don't wait for the shell because if we did we may as well send the
preloads as tags in the HTML.
When provided during a `prerender...` the callback will be called after
the entire prerender is complete. The idea here is we are not responding
to a live request and it is preferable to capture as much as possible
for preloading as Headers in case the prerender was unable to finish the
shell.
Currently the following resources are always preloaded as headers when
the option is provided
1. prefetchDNS and preconnects
2. font preloads
3. high priority image preloads
Additionally if we are providing headers when the shell is incomplete
(regardless of whether it is render or prerender) we will also include
any stylesheet Resources (ones with a precedence prop)
There is a second option `maxHeadersLength?: number` which allows you to
specify the maximum length of the header content in unicode code units.
This is what you get when you read the length property of a string in
javascript. It's improtant to note that this is not the same as the
utf-8 byte length when these headers are serialized in a Response. The
utf8 representation may be the same size, or larger but it will never be
smaller.
If you do not supply a `maxHeadersLength` we defaul to `2000`. This was
chosen as half the value of the max headers length supported by commonly
known web servers and CDNs. many browser and web server can support
significantly more headers than this so you can use this option to
increase the headers limit. You can also of course use it to be even
more conservative. Again it is important to keep in mind there is no
direct translation between the max length and the bytelength and so if
you want to stay under a certain byte length you need to be potentially
more aggressive in the maxHeadersLength you choose.
Conceptually `onHeaders` could be called more than once as new headers
are discovered however if we haven't started flushing yet but since most
APIs for the server including the web standard Response only allow you
to set headers once the current implementation will only call it one
time
DiffTrain build for [2983249](2983249)
gnoff
added a commit
to gnoff/next.js
that referenced
this pull request
Nov 7, 2023
- facebook/react#27641 - facebook/react#27661 - facebook/react#27640 - facebook/react#27595 - facebook/react#27600 - facebook/react#27598 - facebook/react#27590 - facebook/react#27606 - facebook/react#27608 - facebook/react#27601 - facebook/react#27616 - facebook/react#27615 - facebook/react#27614 - facebook/react#27599 - facebook/react#27597 - facebook/react#27525 - facebook/react#27571
kodiakhq bot
pushed a commit
to vercel/next.js
that referenced
this pull request
Nov 7, 2023
Updates React from 08a39539f to 2983249dd. ### React upstream changes - facebook/react#27641 - facebook/react#27661 - facebook/react#27640 - facebook/react#27595 - facebook/react#27600 - facebook/react#27598 - facebook/react#27590 - facebook/react#27606 - facebook/react#27608 - facebook/react#27601 - facebook/react#27616 - facebook/react#27615 - facebook/react#27614 - facebook/react#27599 - facebook/react#27597 - facebook/react#27525 - facebook/react#27571 Updates @types/react to 18.2.37 Updates @types/react-dom to 18.2.15
EdisonVan
pushed a commit
to EdisonVan/react
that referenced
this pull request
Apr 15, 2024
…k#27641) Adds a new option to `react-dom/server` entrypoints. `onHeaders: (headers: Headers) => void` (non node envs) `onHeaders: (headers: { Link?: string }) => void` (node envs) When any `renderTo...` or `prerender...` function is called and this option is provided the supplied function will be called sometime on or before completion of the render with some preload link headers. When provided during a `renderTo...` the callback will usually be called after the first pass at work. The idea here is we want to get a set of headers to start the browser loading well before the shell is ready. We don't wait for the shell because if we did we may as well send the preloads as tags in the HTML. When provided during a `prerender...` the callback will be called after the entire prerender is complete. The idea here is we are not responding to a live request and it is preferable to capture as much as possible for preloading as Headers in case the prerender was unable to finish the shell. Currently the following resources are always preloaded as headers when the option is provided 1. prefetchDNS and preconnects 2. font preloads 3. high priority image preloads Additionally if we are providing headers when the shell is incomplete (regardless of whether it is render or prerender) we will also include any stylesheet Resources (ones with a precedence prop) There is a second option `maxHeadersLength?: number` which allows you to specify the maximum length of the header content in unicode code units. This is what you get when you read the length property of a string in javascript. It's improtant to note that this is not the same as the utf-8 byte length when these headers are serialized in a Response. The utf8 representation may be the same size, or larger but it will never be smaller. If you do not supply a `maxHeadersLength` we defaul to `2000`. This was chosen as half the value of the max headers length supported by commonly known web servers and CDNs. many browser and web server can support significantly more headers than this so you can use this option to increase the headers limit. You can also of course use it to be even more conservative. Again it is important to keep in mind there is no direct translation between the max length and the bytelength and so if you want to stay under a certain byte length you need to be potentially more aggressive in the maxHeadersLength you choose. Conceptually `onHeaders` could be called more than once as new headers are discovered however if we haven't started flushing yet but since most APIs for the server including the web standard Response only allow you to set headers once the current implementation will only call it one time
bigfootjon
pushed a commit
that referenced
this pull request
Apr 18, 2024
Adds a new option to `react-dom/server` entrypoints.
`onHeaders: (headers: Headers) => void` (non node envs)
`onHeaders: (headers: { Link?: string }) => void` (node envs)
When any `renderTo...` or `prerender...` function is called and this
option is provided the supplied function will be called sometime on or
before completion of the render with some preload link headers.
When provided during a `renderTo...` the callback will usually be called
after the first pass at work. The idea here is we want to get a set of
headers to start the browser loading well before the shell is ready. We
don't wait for the shell because if we did we may as well send the
preloads as tags in the HTML.
When provided during a `prerender...` the callback will be called after
the entire prerender is complete. The idea here is we are not responding
to a live request and it is preferable to capture as much as possible
for preloading as Headers in case the prerender was unable to finish the
shell.
Currently the following resources are always preloaded as headers when
the option is provided
1. prefetchDNS and preconnects
2. font preloads
3. high priority image preloads
Additionally if we are providing headers when the shell is incomplete
(regardless of whether it is render or prerender) we will also include
any stylesheet Resources (ones with a precedence prop)
There is a second option `maxHeadersLength?: number` which allows you to
specify the maximum length of the header content in unicode code units.
This is what you get when you read the length property of a string in
javascript. It's improtant to note that this is not the same as the
utf-8 byte length when these headers are serialized in a Response. The
utf8 representation may be the same size, or larger but it will never be
smaller.
If you do not supply a `maxHeadersLength` we defaul to `2000`. This was
chosen as half the value of the max headers length supported by commonly
known web servers and CDNs. many browser and web server can support
significantly more headers than this so you can use this option to
increase the headers limit. You can also of course use it to be even
more conservative. Again it is important to keep in mind there is no
direct translation between the max length and the bytelength and so if
you want to stay under a certain byte length you need to be potentially
more aggressive in the maxHeadersLength you choose.
Conceptually `onHeaders` could be called more than once as new headers
are discovered however if we haven't started flushing yet but since most
APIs for the server including the web standard Response only allow you
to set headers once the current implementation will only call it one
time
DiffTrain build for commit 2983249.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Adds a new option to
react-dom/serverentrypoints.onHeaders: (headers: Headers) => void(non node envs)onHeaders: (headers: { Link?: string }) => void(node envs)When any
renderTo...orprerender...function is called and this option is provided the supplied function will be called sometime on or before completion of the render with some preload link headers.When provided during a
renderTo...the callback will usually be called after the first pass at work. The idea here is we want to get a set of headers to start the browser loading well before the shell is ready. We don't wait for the shell because if we did we may as well send the preloads as tags in the HTML.When provided during a
prerender...the callback will be called after the entire prerender is complete. The idea here is we are not responding to a live request and it is preferable to capture as much as possible for preloading as Headers in case the prerender was unable to finish the shell.Currently the following resources are always preloaded as headers when the option is provided
Additionally if we are providing headers when the shell is incomplete (regardless of whether it is render or prerender) we will also include any stylesheet Resources (ones with a precedence prop)
There is a second option
maxHeadersLength?: numberwhich allows you to specify the maximum length of the header content in unicode code units. This is what you get when you read the length property of a string in javascript. It's improtant to note that this is not the same as the utf-8 byte length when these headers are serialized in a Response. The utf8 representation may be the same size, or larger but it will never be smaller.If you do not supply a
maxHeadersLengthwe defaul to2000. This was chosen as half the value of the max headers length supported by commonly known web servers and CDNs. many browser and web server can support significantly more headers than this so you can use this option to increase the headers limit. You can also of course use it to be even more conservative. Again it is important to keep in mind there is no direct translation between the max length and the bytelength and so if you want to stay under a certain byte length you need to be potentially more aggressive in the maxHeadersLength you choose.Conceptually
onHeaderscould be called more than once as new headers are discovered however if we haven't started flushing yet but since most APIs for the server including the web standard Response only allow you to set headers once the current implementation will only call it one time