-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
Allow adapters to customize headers for fetch requests #14543
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
Conversation
…otection) Adds a new generic mechanism for adapters to inject headers into Astro's internal fetch calls. This enables features like Netlify's skew protection. Changes: - Add `runtimeConfig.internalFetchHeaders` to AstroAdapter interface - Create `astro:adapter-config/client` virtual module - Update Actions, View Transitions, Server Islands, and Prefetch to use adapter headers - Implement Netlify skew protection with DEPLOY_ID header - Generate `.netlify/v1/skew-protection.json` configuration file - Add comprehensive test fixture for skew protection
🦋 Changeset detectedLatest commit: a0424b6 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
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 PR is blocked because it contains a minor changeset. A reviewer will merge this at the next release if approved.
packages/integrations/netlify/test/functions/skew-protection.test.js
Outdated
Show resolved
Hide resolved
packages/integrations/netlify/test/functions/skew-protection.test.js
Outdated
Show resolved
Hide resolved
- Add type declarations for astro:adapter-config/client virtual module - Add manifest property to SSRResult interface - Fix Object.entries type assertions in prefetch and router - Add explicit return type to Netlify adapter's internalFetchHeaders function
Follow existing pattern of extracting specific fields from manifest rather than passing the entire manifest object to SSRResult. This matches how other fields like base, trailingSlash, and serverIslandNameMap are handled.
CodSpeed Performance ReportMerging #14543 will not alter performanceComparing Summary
|
- Remove incomplete test stubs for actions and view transitions - Remove unused manifestPath variable - Update prefetch comment to be less strict about dependencies
The test was checking ssr.mjs which is just a wrapper, but the actual serialized manifest is in the manifest_*.mjs file in the build directory.
This prevents timeouts in integration tests where client-side files importing the virtual module are processed during SSR.
commit: |
|
Is the virtual module meant to be exposed to users or is it purely internal? |
ematipico
left a comment
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.
The feature has a mixture of "client" and "runtime" concepts that seem to overlap, however we know that they are two different things.
I think we should normalise the nomenclature in our code and docs
It's more or less internal. I thought about making it exposed to the user so they could use it in their own |
| Adds support for `assetQueryParams` and `internalFetchHeaders` in the Netlify adapter. When deployed to Netlify, the adapter now: | ||
|
|
||
| - Includes the deployment ID in asset URLs via `assetQueryParams` (query parameter: `dpl`) | ||
| - Includes the deployment ID in internal fetch requests via `internalFetchHeaders` (header: `X-Netlify-Deploy-ID`) | ||
|
|
||
| This enables Netlify's skew protection feature, which ensures that client and server versions match during deployments by tracking the deployment ID across both asset requests and internal API calls. |
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 mostly implementation detail. I'd lead on the fact this adds support for skew protection, and then say that it adds the query params and fetch headers. Users don't care about which adapter API it uses.
sarah11918
left a comment
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.
OK, so this changed since I approved! 😄
Some suggestions, that I think somewhat addresses @ascorbic 's feedback!
ascorbic
left a comment
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.
The Netlify team said that the feature could do with having a config option, but make it on by default. The Vercel one is off by default. I wonder if we should align them: both on by default but with an opt-out.
Co-authored-by: Sarah Rainsberger <[email protected]>
Co-authored-by: Sarah Rainsberger <[email protected]>
…into netlify-skew-protection
|
@ascorbic Their suggestion was out of caution. I'd rather not add flag without knowing a use-case for it. If people complain for some reason we can add a flag later. I've moved the Vercel implementation to use the |
Co-authored-by: Steven <[email protected]>
|
@matthewp I think that as it is a brand new feature on Netlify and users will probably not even know they're using it, it would be good to at least have a way to opt-out in case it breaks stuff. |
|
@ascorbic If it breaks stuff they can downgrade the package until we fix it. If we add a flag and it breaks stuff they still need to find out and then set the flag to false. Not any easier than just downgrading. If we have a flag we have to keep it until a major at least, whereas if there's no flag we can either just fix the bug or if we find there's a real use-case for disabling it we can then add said flag. |
sarah11918
left a comment
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.
Just making it extra clear that docs is again approving the newest version of the changesets!
Docs PR content is also approved, and will be good to go! (Just fiddling with larger changes the newer content is causing in other pages like changed anchor links)
Changes
runtimeConfig.internalFetchHeadersto AstroAdapter interfaceastro:adapter-config/clientvirtual module.netlify/v1/skew-protection.jsonconfiguration file@astrojs/vercelskew protection doesn't work #14545Testing
Docs