Skip to content
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

Merged
merged 10 commits into from
Nov 21, 2022

Conversation

AirBorne04
Copy link
Contributor

Changes

Testing

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!

@changeset-bot
Copy link

changeset-bot bot commented Nov 10, 2022

🦋 Changeset detected

Latest 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

@github-actions github-actions bot added the pkg: integration Related to any renderer integration (scope) label Nov 10, 2022
@AirBorne04 AirBorne04 force-pushed the cloudflare-assets branch 2 times, most recently from 86a86fe to 2bf0f1f Compare November 12, 2022 13:11
Copy link
Member

@bluwy bluwy left a 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!

@AirBorne04
Copy link
Contributor Author

I have a question only but rest are looks great to me!

What is the question? 😄 I also got one, should I add a Docs section?

Copy link
Member

@bluwy bluwy left a 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

packages/integrations/cloudflare/src/index.ts Outdated Show resolved Hide resolved
@sarah11918
Copy link
Member

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.)

Copy link
Member

@bluwy bluwy left a 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.

@AirBorne04 AirBorne04 force-pushed the cloudflare-assets branch 2 times, most recently from 0a16dd9 to b7b0e30 Compare November 15, 2022 16:25
Copy link
Member

@delucis delucis left a 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.

packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
packages/integrations/cloudflare/README.md Show resolved Hide resolved
Copy link
Member

@delucis delucis left a 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/*"]
}

packages/integrations/cloudflare/README.md Outdated Show resolved Hide resolved
@AirBorne04
Copy link
Contributor Author

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?

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.
In order to make this more transparent for the devs at build time we could add a little testing Script checking the include / exclude and print a warning about those cases?

@delucis
Copy link
Member

delucis commented Nov 17, 2022

Unfortunately it is more complex [...] they need to be listed explicitly

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 dist/ after the build is complete or something to dynamically generate a _routes.json).

Out of scope for this PR, but I could see the argument for allowing customization of _routes.json directly from the adapter, something like:

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.

@AirBorne04
Copy link
Contributor Author

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:

  • dynamic function invocation (supported)
  • serving static files (supported)
  • performing static redirects (not considered yet)

So what would make sense is including the redirect paths into the exclude path list. Than we truly support all native cloudflare functions.

@bluwy let me add this last edition before merge and we should be golden

@AirBorne04
Copy link
Contributor Author

AirBorne04 commented Nov 17, 2022

HI @delucis @bluwy,

i have added the aforementioned addition to also include the path from the _redirects file into the exclude list, like this the _redirects will also take effect. Now i really cannot think of any scenario where someone would want a custom _routes.json and hopefully this is covering 99,9% of the use cases. Therefore, imo ready to go 😄

@matthewp
Copy link
Contributor

@withastro/maintainers-docs need approval from someone on your team.

AirBorne04 and others added 6 commits November 21, 2022 09:30
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Co-authored-by: Chris Swithinbank <swithinbank@gmail.com>
Copy link
Member

@delucis delucis left a 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!

@bluwy bluwy merged commit 743000c into withastro:main Nov 21, 2022
@bluwy
Copy link
Member

bluwy commented Nov 21, 2022

Finally merged 😄 Thanks again @AirBorne04 for improving the Cloudflare integration.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pkg: integration Related to any renderer integration (scope)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants