-
-
Notifications
You must be signed in to change notification settings - Fork 49
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
Cloudflare: _routes.json is truncated #374
Comments
Yeah we needed to rethink the implementation, due to some limitations. Could you explain what a more |
Sure. I realize there are probably some special cases to consider, but on the surface it could work like this - that's how I built it at the time:
Example: Let's say we have Current output - with 90% of static pages becoming dynamic when they should not: {
"version": 1,
"include": [
"/*"
],
"exclude": [
"/",
"/_astro/*",
"/favicon.svg",
"/static/0",
"/static/1",
...
"/static/95"
]
} Nicer output - which was produced with version <= 9: {
"version": 1,
"include": [
"/_image",
"/dynamic/*"
],
"exclude": []
} If there are a few more dynamic routes and the static ones become the exception, it could also become: {
"version": 1,
"include": [
"/*"
],
"exclude": [
"/static/*"
]
} And if there is a mix of dynamic and static routes in the same folder, something like: {
"version": 1,
"include": [
"/mostlyDynamic/*"
],
"exclude": [
"/mostlyDynamic/exceptThisOne" // excludes override includes if I remember correctly
]
} Yes, that brings more complexity into the code, but right now the automatic generation is a bit misleading in my opinion, because it simply does not do what you expect. So if you are not against it in principle, I could try my luck. I would help to be aware of any special cases that might not be obvious. Like
|
This is a false assumption. We can't go by the size only. Some project specific circumstances, force to use a
Actually for your example the current output is the correct one. To be clear I'm open for a better approach, but we need to discuss this further. Compatibility goes above optimization here (means if we loose some static pages in the exclude array, but won't have an error for other pages, which would otherwise be not routed correctly, we would always prefer the one which routes more things dynamically) I'm happy to give more insights, but I hope this makes sense. |
By choosing the smaller result I mean between multiple correct options, the smaller one should be chosen. Of course, not throwing errors and always returning a proper result is the most important thing. But if we can make sure of that, a smaller solution or rather a more complete solution would be better, right?
Correct in the sense that users always see the correct page - yes. Correct in the sense that performance and costs are as you would expect - no. This really can be a problem for even moderately popular sites. 100k requests per day is not unrealistic - since you would possibly have many requests per page visit if you invoke a function for almost every page and many of the assets on them. So the question is, is it possible to guarantee correctness (at least when using Astro's own mechanisms - if you use custom functions, I guess anything goes). I'm interested in the circumstances that "force to use a |
Only have time to answer quickly, will answer in detail later. But if the user doesn't have a pre-rendered static /404 route, we have to use |
Ah, ok. What does the function do in that case? From the Cloudflare docs (https://developers.cloudflare.com/pages/configuration/serving-pages):
When I test this without custom 404 on Cloudflare and access a route that does not exists, I simply see the homepage. Not the same Astro generated 404 page that I see when developing. |
Exactly what is described, this is not the correct behavior. Similar case happens if you have static routes and potential dynamic routes under the same subpaths. So to summarize, we are interested in every idea to make this better, as long as it is compatible and guarantee correctness for every scenario. If we would have to decide if we need a documentation which tells users, what not to do vs. just routing to SSR, we would always want to fallback to SSR instead of the limitation. On another note, I wonder if the |
I was testing with the current version.
Yes, those cases need to be handled correctly. And that - figuring out which wildcard path can clash with wich other wildcard path - will be the main difficulty.
I will think about it. And if it seems doable, I will present my ideas. Thanks for the feedback and hints so far 😀
We have a static 404 page, but the output is as posted earlier. |
If you have a static 404 page, which results in P.S.: Another note here, I have some early access to upcoming Cloudflare changes, and it seems like there might be a new mode we need to support, which doesn't support a |
Took some time, but now I got around to try my luck. And in fact, there was a small issue that prevented the routes generation from behaving like you said. So without big/dangerous changes, we can already do a lot better. See #423
Maybe you are talking about the new "Static Assets" and "Frameworks" feature, that is in the Cloudflare Workers docs now. Looks interesting. Not sure if this will work for Pages as well, but the thought of not bothering with a _routes.json, but instead having Cloudflare detecting static assets automatically would be very cool indeed. |
Seems like this is related, but when I add a custom top level 404.astro, the deployment fails with the error: And the built
This is with |
Im out of pocket for a few days, but in the meantime you can check out this repo/branch: https://github.com/OpenMaine/maineballot/tree/add-404 I was able to use a dynamic 404 (although Cloudflare only seems to render correctly for the /404 route specifically, but not other pages with a 404 response). If I use a static 404.astro, I get the output above. |
Thanks for the answer. This is quite big project, so I can't promise I have time to use that. A minimal reproducible example would be required. However I try my best, and if I find something, I'll let you know. But no promises. |
It's probably because you have A |
I can make a smaller repro, just in a few days when I get home. I'll let you know. |
@alexanderniebuhr Here's a repro using the blog starter https://github.com/IHIutch/astro-cf. Please let me know if there's anything I can do to help! |
I'll take a look. Thank you! |
Astro Info
Describe the Bug
Hi, I have worked on automatic
_routes.json
generation some time ago. I tried to make the file as small as possible, because Cloudflare imposes a surprising limit of 100 rules. I guess there were issues or the feature or it was not possible to maintain with the big refactor or something. In any case, it's back to the old, simple way of generating the file.Simple is good of course, but the way the limit is handled now is: After 100, the rest of the rules is simply cut off. I was very surprised that my page was not working as expected - because many of my static routes resulted in function invocations. That can be a lot slower and it can become really expensive, if one it not careful.
What's the expected result?
Hi, I have worked on automatic
_routes.json
generation some time ago. I tried to make the file as small as possible, because Cloudflare imposes a surprising limit of 100 rules. I guess there were issues or the feature or it was not possible to maintain with the big refactor or something. In any case, it's back to the old, simple way of generating the file now.Simple is good of course, but the way the limit is handled now is: After 100, the rest of the rules is simply cut off. I was very surprised that my page was not working as expected. A lot of static routes are resulting in function invocations instead! That can be a lot slower and expensive if you have a bit more traffic. I think, truncating is not the best solution. It is a common use case that sites have many static pages and only a few dynamic ones.
Would you be interested in investigating a more efficient approach once again? If so, what were the reasons/problems that prompted that simplification?
Link to Minimal Reproducible Example
https://stackblitz.com/edit/github-qvzkwv?file=dist%2F_routes.json
Participation
The text was updated successfully, but these errors were encountered: