withastro / adapters

Home for Astro's core maintained adapters
46 stars 24 forks source link

Cloudflare adapter incorrectly marks intermediate SSR page as static #289

Open LexSwed opened 1 week ago

LexSwed commented 1 week ago

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/cloudflare@10.4.1 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

alexanderniebuhr commented 1 week ago

@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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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 commented 1 week ago

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.