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

Cloudflare adapter incorrectly marks intermediate SSR page as static #289

Open
1 task
LexSwed opened this issue Jun 17, 2024 · 7 comments
Open
1 task
Labels
- P2: nice to have Not breaking anything but nice to have (priority) pkg: cloudflare

Comments

@LexSwed
Copy link

LexSwed commented Jun 17, 2024

Astro Info

Astro                    v4.10.3
Node                     v20.14.0
System                   macOS (arm64)
Package Manager          npm
Output                   hybrid
Adapter                  @astrojs/cloudflare
Integrations             astro-expressive-code
                         @astrojs/mdx
                         @astrojs/sitemap
                         @astrojs/tailwind
                         astro-robots-txt

Describe the Bug

When using @astrojs/[email protected] with the next project structure:

/ # static
/feed # SSR for feed filtering
/feed/[slug] # static

Generated _routes.json is:

{
  "version": 1,
  "include": ["/*"],
  "exclude": [
    "/",
    // static files
    "/feed/*"
  ]
}

Which makes /feed page to not server render and render root / instead. extending the adapter config with

"include": [{ "pattern": "/feed" }],

makes wrangler to fail due to Overlapping rules found., as /feed/* is still generated in "exclude".

What's the expected result?

Generated _routes.json works with /feed in include for SSR and /feed/* in exclude as static.

Link to Minimal Reproducible Example

https://github.com/LexSwed/me.git

Participation

  • I am willing to submit a pull request for this issue.
@LexSwed LexSwed changed the title Cloudflare adapter incorrectly marks intermediate SSR page as staitc Cloudflare adapter incorrectly marks intermediate SSR page as static Jun 17, 2024
@alexanderniebuhr
Copy link
Member

@LexSwed I don't think what you want to achieve works. You can't have the following, due to the overlapping rules issue:

{
  "version": 1,
  "include": [
	"/feed/"
  ],
  "exclude": [
    "/feed/*"
  ]
}

@LexSwed
Copy link
Author

LexSwed commented Jun 18, 2024

Thanks @alexanderniebuhr
Does it mean that my previous setup of having an SSR page in between static pages with slug is not possible any more?

@alexanderniebuhr
Copy link
Member

I'm not fully sure, if I understand correctly, but I think it should be. Could you provide a valid _routes.json, which you create by hand, which does work with wrangler. Then I can use that as a test case for the automatic generation.

@LexSwed
Copy link
Author

LexSwed commented Jun 18, 2024

In my case, every generated static path has to be included into the exclude.
So, right now a page with /feed/[slug].astro would get an entry /feed/*, which makes all /feed* pages static, including the root one, which should not be static.

// automatically generated
{
  "version": 1,
  "include": ["/*"],
  "exclude": [
    "/",
    // static files
    "/feed/*"
  ]
}

But since my root /feed page needs SSR, _routes.json should not have /feed/* in exclude, but rather put every individual page path that is static into exclude:

{
  "version": 1,
  "include": ["/*"],
  "exclude": [
    "/",
    "/_astro/*",
    // static files
    // /feed/[slug] but not /feed*
    "/feed/article-1",
    "/feed/article-2",
    "/feed/article-3",
    "/feed/article-4",
    "/feed/article-1/maybe/nested",
  ]
}

E.g. /feed/article-1/maybe could be SSRd as well, while /feed/article-4/maybe/nested - is static.

@alexanderniebuhr alexanderniebuhr added - P2: nice to have Not breaking anything but nice to have (priority) and removed needs response labels Jun 18, 2024
@alexanderniebuhr
Copy link
Member

Thanks for explaining, that specific case is not that easy to support, since it wouldn't work for projects with a lot of static pages, since there is a limit. I have to think about a way which allows to support this.
The quick solution we could add is that, as soon as a SSR page is found in a path, we will not exclude that, however that will lead to unnecessary fallback requests.

@LexSwed
Copy link
Author

LexSwed commented Jun 18, 2024

Ouff, thanks @alexanderniebuhr 🙇

Surprised it's not considered as a regression, it used to work on @astrojs/cloudflare@9 and from the release notes does not seem to be an intended change to stop supporting this behavior:

If you are using routes.strategy, you can remove it. You might observe a different dist/_routes.json file, but it should not affect your project's behavior.

Is _routes.json something Cloudflare setup specific, or part of how the integration works? Could the behavior of overlapping routes be changed maybe? Or maybe support more advanced patterns in there?

@alexanderniebuhr
Copy link
Member

Surprised it's not considered as a regression, it used to work on @astrojs/cloudflare@9

I understand that. However I want to note that it didn't work for other examples in v9. Many users ran into the maximum rules limit, if we don't use wildcards.

does not seem to be an intended change to stop supporting this behavior

No we don't intended to stop supporting this use-case, but the intention for the automatic generation is to work for as many projects as possible, without optimizing it for specific cases. That means that it will be less optimized than a manually created one.

Could the behavior of overlapping routes be changed maybe? Or maybe support more advanced patterns in there?

No. _routes.json is a Cloudflare platform specific file, we are working actively with the Cloudflare team to discuss this, since this is a micro optimization, in fact you could just use include: ["/*"] and it will still work, there are some hesitations to change the behavior for everything.

We can definitely fix the generation to render all routes again, but I don't think we can micro optimize it for cost for the time being.

@alexanderniebuhr alexanderniebuhr added this to the 11.1 milestone Jun 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
- P2: nice to have Not breaking anything but nice to have (priority) pkg: cloudflare
Projects
None yet
Development

No branches or pull requests

2 participants