-
Notifications
You must be signed in to change notification settings - Fork 138
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
fix: no keep alive for large payloads #1416
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
Size Change: +516 B (+0.04%) Total Size: 1.21 MB
ℹ️ View Unchanged
|
// so let's get the best of both worlds and only set keepalive for POST requests | ||
// where the body is less than 64kb | ||
// NB this is fetch keepalive and not http keepalive | ||
keepalive: options.method === 'POST' && (estimatedSize || 0) < 64 * 1024, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could abstract this but doesn't seem necessary given the comment explains a lot
keepalive: options.method === 'POST' && (estimatedSize || 0) < 64 * 1024, | |
keepalive: options.method === 'POST' && (estimatedSize || 0) < MAX_KEEPALIVE_KB, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah, i'm always torn by the tension between "clarify intent" and "have fewest parts"
if you make it a const it's arguably clearer but i need to look at multiple places to be sure i understand
we set keepalive to true for any request that is a POST
the fetch spec at https://fetch.spec.whatwg.org/#http-network-or-cache-fetch says
read more at e.g. https://javascript.info/fetch-api#:~:text=But%20the%20keepalive%20option%20tells,for%20keepalive%20requests%20is%2064KB.
it is likely that a replay payload would be >64kb compressed
not all browsers obey this spec equally (of course) so it won't always fail, so it's not broken consistently enough to stand out.
but locally I can see network requests failing for this reason
fetch keepalive is intended for situations including those like analytics (👋 posthog) where you might want to allow a request to finish even though the page is navigating so we don't want to turn it off completely
lets estimate body size and set keepalive when its safe (which it should be for most analytics events)