-
Notifications
You must be signed in to change notification settings - Fork 87
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: add more measures to prevent using data-cache for blob operations #2775
fix: add more measures to prevent using data-cache for blob operations #2775
Conversation
📊 Package size report 0.03%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
6d77ce4
to
adb7ab3
Compare
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.
All good, just one non-blocking question
attemptToGetOriginalFetch(globalThis.fetch), | ||
) | ||
const getFetchBeforeNextPatchedIt = () => | ||
extendedGlobalThis[FETCH_BEFORE_NEXT_PATCHED_IT] ?? fetchBeforeNextPatchedItFallback |
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.
Is this necessary? You're calling setFetchBeforeNextPatchedIt
in the global scope of server.ts
, so would the fallback ever be used?
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.
It should not be needed when running full on server handler, no.
I did want to make this module to not necessarily rely on setup (might be useful for unit tests, tho not sure if we actually have any that would would fall into using this module today)
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.
This is also just using pattern of allowing few points of failure because of overall messiness with patched fetch - if for whatever reason we don't call setFetchBeforeNextPatchedIt
before we execute getFetchBeforeNextPatchedIt
- I do want to use rest of defensive measures (such as trying to grab _originalFetch
first and ultimately adding next.internal = true
param to fetch
calls) and continue to have this fallback just fits the best here (IMO)
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.
So I'll go ahead and merge as-is
Description
Our current way of attempting to use unpatched fetch for blob operations seems not sufficient so this change attempt to make sure we avoid this by doing couple of things:
fetch
wrapper that would force setnext.internal = true
in RequestInit argument as that's how Vercel's data fetch seems to ensure their suspense-cache requests don't get accidental and not intentional fetch caching.Relevant links (GitHub issues, etc.) or a picture of cute animal
https://linear.app/netlify/issue/FRB-1686/try-to-store-unpatched-fetch-earlier