-
-
Notifications
You must be signed in to change notification settings - Fork 3.1k
feat(cloudflare): add experimental support for static headers #13959
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
feat(cloudflare): add experimental support for static headers #13959
Conversation
🦋 Changeset detectedLatest commit: a9ba349 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 |
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.
Thanks for this. I'm going to ask @ematipico for a review as he's the one who's working on this feature, but it looks good from my perspective.
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.
Thank you @Johannes-Andersen for your first contribution! I left some comments. I spotted some bugs, and asked a few questions.
One important thing that we have to address is to add use cases. Let me know if there's something that isn't clear.
packages/integrations/cloudflare/src/utils/generate-headers-file.ts
Outdated
Show resolved
Hide resolved
packages/integrations/cloudflare/src/utils/generate-headers-file.ts
Outdated
Show resolved
Hide resolved
| for (const line of raw.split(/\r?\n/)) { | ||
| if (!line.trim()) continue; | ||
| if (!/^\s/.test(line)) { | ||
| currentPattern = line.trim(); | ||
| headersByPattern.set(currentPattern, headersByPattern.get(currentPattern) || {}); | ||
| } else { | ||
| const [name, ...rest] = line.trim().split(':'); | ||
| headersByPattern.get(currentPattern)![name.trim()] = rest.join(':').trim(); | ||
| } | ||
| } |
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 the first time that this adapter is in charge of _headers, which means that Astro owns its contents every time it is generated. In fact, its relative is loaded from outDir, instead of publicDir.
If the assumption is correct, it means that we don't need to read its contents, because they are generated anew at every build.
EDIT: I see that the tests use the public directory, which means that the code needs to be updated to use publicDir configuration. Also, I am fine to have this behaviour different from Netlify, at the end, it's up to the adapter how to use the feature. We need to make sure to:
- update the documentation of the adapter, and explain that when
experimentalStaticHeadersis enabled, Astro will updatepublic/_headers - add more tests
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.
As I understand it, right now a user could create a _headers file in public and it would work. We want to be able to not break these sites, so it makes sense to update the file in outDir (merging the user headers with the added ones) and leave the one in public untouched.
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 that makes sense!
| const csp = heads.get('Content-Security-Policy'); | ||
| if (!csp) continue; | ||
|
|
||
| const pattern = segmentsToCfSyntax(route.segments, config); |
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.
Out of curiosity, is there a reason why you didn't use createHostRouteDefinition like we do in Netlify?
| const definition = createHostedRouteDefinition(route, config); |
The logic behind it has already been used for the _redirects file in Cloudflare, which means that it's battle-tested, and it provides all the information to create the correct Unix-like paths.
| /has-header | ||
| cdn-cache-control: public, max-age=3600 |
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.
Now that the adapter reads this file and updates it, we need to ensure that we join routes correctly. Which means that if this file specifies a route e.g. /server-island with some headers, and Astro emits the CSP header for this very route, the final file should contain both headers.
Can we add few more tests that cover more Astro routes:
- static routes e.g.
src/pages/server-island.astro - dynamic routes e.g.
src/pages/posts/[slug].astro - catch all route e.g.
src/pages/blog/[...catchAll].astro
And have few routes in _headers that match some of those routes and make sure they all have all headers merged.
/server-island
cdn-cache-control: public, max-age=3600
/posts/intro
cdn-cache-control: public, max-age=3600
/blog/hello-word
cdn-cache-control: public, max-age=3600
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.
I agree, I would add some more built-in routes to the tests
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.
@alexanderniebuhr I tried adding more. Would love to hear what more routes you would like tested :D
Thanks for the review! The feedback you left makes sense! I'll have a go at fixing them tomorrow. A bit busy today |
c5a5ff8 to
0667f93
Compare
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.
Thank you for addressing my comments! I tried to merge main again, because some tests were failing and I don't know why
|
@Johannes-Andersen we are in the process of changing the payload, so we're holding off merging the PR. However, in the meantime, can you please address the CI failures? |
|
@Johannes-Andersen heads-up that this PR #13972 is going to change how we generate the static headers for the page |
|
I will also review this tomorrow :) |
| const output = | ||
| [...headersByPattern] | ||
| .map(([pattern, headers]) => | ||
| [pattern, ...Object.entries(headers).map(([k, v]) => ` ${k}: ${v}`)].join('\n'), | ||
| ) | ||
| .join('\n\n') + '\n'; |
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.
Don't we have a shared util from astro for this?
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.
Be aware that this PR #13972 will change headersByPattern, so we will probably have to update this code.
Nonetheless, we have @astrojs/underscore-redirects that might provide something we can use here, but you should look into it
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 was thinking about printAsRedirects from @astrojs/underscore-redirects, but I'm not sure if the syntax is 100% equal
| async function loadExistingHeaders(publicUrl: URL): Promise<Map<string, Record<string, string>>> { | ||
| try { | ||
| const text = await readFile(publicUrl, 'utf-8'); | ||
| return text | ||
| .split(/\r?\n/) | ||
| .filter(Boolean) | ||
| .reduce( | ||
| (map, line) => { | ||
| if (!line.startsWith(' ')) { | ||
| map.current = line.trim(); | ||
| map.store.set(map.current, map.store.get(map.current) ?? {}); | ||
| } else { | ||
| const [key, ...rest] = line.trim().split(':'); | ||
| map.store.get(map.current)![key.trim()] = rest.join(':').trim(); | ||
| } | ||
| return map; | ||
| }, | ||
| { current: '', store: new Map<string, Record<string, string>>() }, | ||
| ).store; | ||
| } catch { | ||
| return new Map(); | ||
| } | ||
| } |
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.
Good for now, but in the long term I think we should also have this shared from astro, I feel like this could also apply to other providers not just Cloudflare. WDYT @ematipico?
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.
I'm not sure we can. Each adapter (i.e., hosting platform) has its own configuration file, with its own syntax. For example, with Netlify we use their config.json file. With the Node.js adapter, we created a custom JSON file with a different structure
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.
Interesting, I thought the _headers file is a common syntax, than disregard my 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.
I think the _headers file is a kinda-common syntax. I think it's generally supported in the same places that support _redirects, so there is a good argument for adding support in that package. We don't use it on Netlify now, as we use their new framework API. On Netlify at least, the _headers file is now mainly created by users rather than auto-generated by frameworks.
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 fixing a quick changeset nit!
Co-authored-by: Sarah Rainsberger <[email protected]>
|
For docs, noting that an accompanying docs PR should add (By the time this merges, there should already be something similar written for the Node, Netlify, and Vercel adapters, so you can basically copy from them.) |
|
@Johannes-Andersen Hi! Are you still interested in this PR? |
Got a bit much happening in life and at work at the moment 😅 |
|
No problem, thanks for opening the PR in the first place! |
Changes
This is my first ever change to astro. So I am not sure this is the correct way. Therefore a more thorough review may be required.
The PR aims to implement the experimental CSP policy to the cloudflare adapter.
Mainly copied work from: de82ef2
Testing
Added tests. Existing tests should still pass.
Validate CF docs if output matches.
Example output:
Docs
Docs needs to be added, todo based on feedback.