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

fix: cloudflare routing 404 #870

Merged
merged 1 commit into from
Sep 12, 2024

Conversation

rmarscher
Copy link
Contributor

This fixes cloudflare static vs dynamic routing for the home index page. But there are likely more bugs with the _routes.json when trying to nest a dynamic route under a static route path. By using _routes.json, we can route static html files directly off Cloudflare's CDN and not incur any worker function changes. But we need to make sure it only matches the static html routes.

Right now, it is doing a very basic walk of the public output dir to generate a _routes.json. We need to generate a more accurate list of exclude patterns. I am opening this PR as a draft until I can complete this. Fixes #869

Copy link

vercel bot commented Sep 10, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
waku ✅ Ready (Inspect) Visit Preview Sep 11, 2024 5:26am

Copy link

codesandbox-ci bot commented Sep 10, 2024

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

@dai-shi
Copy link
Owner

dai-shi commented Sep 10, 2024

Let me merge #866 first.

@rmarscher
Copy link
Contributor Author

Wow. #866 is an exciting PR. I will rebase and see if I can finish the rest of the cloudflare routing updates needed here.

@rmarscher
Copy link
Contributor Author

rmarscher commented Sep 10, 2024

I updated the _routes.json to always start with "/assets/*" in the exclude and then walk the public directory and add the rest of the files, replacing /index.html with / in the exclude.

I noticed a couple new issues while working on this.

  1. The static html routes are put in index.html files and redirect from the non-trailing slash to the trailing slash. When I changed a route from static to dynamic, my redirect was still cached but I now received an error from https://github.com/dai-shi/waku/blob/main/packages/waku/src/lib/renderers/utils.ts#L14 about input should not end in /. I cleared the redirect cache and I was able to access it without the trailing slash. I think we might need to open another discussion or issue around trailing slash configuration. Most meta frameworks have a config to allow either. For cloudflare, we could move /path/index.html files to /path.html which cloudflare will route statically as /path.
    Edit: I opened a discussion on trailing slashes here Trailing slash configuration #873

  2. Dynamic routes on cloudflare are failing. I am working to determine which recent update caused it. It is a new issue since 0.21.1. The error is with trying to import node:fs/promises in fsRouter.node:fs is not available in cloudflare even with nodejs_compat_v2. https://developers.cloudflare.com/workers/runtime-apis/nodejs/#nodejs-api-polyfills
    Edit: I created a separate issue to investigate this: Dynamic routes on Cloudflare fail trying to import node:fs/promises #871

@tylersayshi
Copy link
Contributor

For cloudflare, we could move /path/index.html files to /path.html which cloudflare will route statically as /path

Could you please link to the docs for the cloudflare path support?

@rmarscher
Copy link
Contributor Author

For cloudflare, we could move /path/index.html files to /path.html which cloudflare will route statically as /path

Could you please link to the docs for the cloudflare path support?

No problem! https://developers.cloudflare.com/pages/configuration/serving-pages/

@rmarscher
Copy link
Contributor Author

Here are other docs to reference.

We are using "advanced mode" so everything routes through a single /worker.js/index.js file right now (the root dir is actually named worker.js which is a trick other projects use to encapsulate many files into the advanced mode function).
https://developers.cloudflare.com/pages/functions/advanced-mode/

There is also additional relevant documentation here. Once advanced mode is enabled, a _routes.json file is required to split dynamic routes that are handler by the worker function and static routes that should be served off the CDN. https://developers.cloudflare.com/pages/functions/routing/

Copy link
Owner

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't test it, but looks fine to me. Can I merge?

@tylersayshi
Copy link
Contributor

I didn't test it, but looks fine to me. Can I merge?

lgtm. Thanks for the docs and the change @rmarscher !

@dai-shi
Copy link
Owner

dai-shi commented Sep 12, 2024

Thanks for confirming.

@dai-shi dai-shi merged commit 57c6c13 into dai-shi:main Sep 12, 2024
28 checks passed
@dai-shi dai-shi mentioned this pull request Sep 12, 2024
83 tasks
@rmarscher
Copy link
Contributor Author

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cloudflare pages deploy results in 404 status when index page is "static"
3 participants