-
-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
Use Cloudflare Pages to serve static assets and support _headers
, _redirects
and _routes.json
#5347
Conversation
🦋 Changeset detectedLatest commit: 4c94f62 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 |
86a86fe
to
2bf0f1f
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.
I have a question only but rest are looks great to me!
2bf0f1f
to
36d52f5
Compare
What is the question? 😄 I also got one, should I add a Docs section? |
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.
LOL I wrote the review and didn't submit it 🤦
should I add a Docs section?
Yes I think it would be great to explain the special files 👍 especially that specifying a _routes.json
would remove some optimization
36d52f5
to
060d552
Compare
53fa908
to
3befc0d
Compare
Heads up from Docs that this PR is on our radar, and note that any docs to accompany this should go right in this README, which will be mirrored into docs. So, no separate PR to docs is necessary, but also, if you have more you want to document, be sure to put it in the README. (And, reminder, this one should not merge until a Docs maintainer approves the README.) |
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.
Changes LGTM! Waiting for docs approval first before merging.
0a16dd9
to
b7b0e30
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.
Hi @AirBorne04! Thanks for this contribution — I’m here with the promised docs review 🎉
I’ve suggested a couple of small rewrites to try and help with clarity — I hope I’ve understood the features correctly as I don’t have direct experience with this adapter.
I also left one question at the end that could require extra docs depending on the answer.
443f058
to
41c09e2
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.
Thanks @AirBorne04! Left some small tweaks to the second paragraph, but much clearer 🙌
Do you think we can offer a snippet people can copy/paste to build from that gives them the default behaviour? Would something like this work or is it more complex?
{
"version": 1,
"include": ["/*"],
"exclude": ["/dist/*"]
}
Unfortunately it is more complex, it actually lists all static files (present after build) because it might be that ’config.json’ in any path is a static file or a dynamic api Route managed by Astro or both :) Therefore they need to be listed explicitly, even when static files are placed in a subfolder the ui framework might compile static files into the root. |
OK, thanks! So in that case it sounds like it might be most practical to think of this optimization as “not possible” if you opt out (i.e. a solution likely needs some hacky glob of Out of scope for this PR, but I could see the argument for allowing customization of cloudflare({
extendRoutes: {
include: [],
exclude: [],
},
}) But I’m not on top of all the use cases, so I’ll leave that to the folks working with this adapter more directly. |
Thinking about it I can actually only think of one valid use case and that is when using redirects, which I have not considered before. There are only three types of responses from the pages platform:
So what would make sense is including the redirect paths into the @bluwy let me add this last edition before merge and we should be golden |
17eb8ea
to
2983abb
Compare
i have added the aforementioned addition to also include the path from the |
@withastro/maintainers-docs need approval from someone on your team. |
adding special configurations
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
… + docs adjustment
aa99700
to
f598704
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.
Looks good to me!
Finally merged 😄 Thanks again @AirBorne04 for improving the Cloudflare integration. |
Changes
_headers
,_redirects
androutes.json
filesdist
folder. #5245 and follow up on Handlebase
in adapters #5290Testing
Did test run the Solid and React example projects with the cloudflare adapter.
Docs
There could be a new section on the Docs explaining how to use
_headers
,_redirects
and_routes.json
files with cloudflare. Linking to their Docs for headers, redirects and routes. One speciality with the_routes.json
file is that it is auto generated for the assets to be excluded from functions invocation. If there is a routes file present from the user it will not be replaced, but therefore the asset handling is falling back to the worker, which resolves the assets./cc @withastro/maintainers-docs for feedback!