withastro / adapters

Home for Astro's core maintained adapters
47 stars 26 forks source link

[Cloudflare adapters] Possible bug: too many _routes.json entries #172

Closed maddsua closed 4 months ago

maddsua commented 4 months ago

Astro Info

Astro                    v4.4.1
Node                     v20.9.0
System                   Windows (x64)
Package Manager          npm
Output                   hybrid
Adapter                  @astrojs/cloudflare
Integrations             @astrojs/sitemap
                         @astrojs/vue
                         @astrojs/tailwind

Describe the Bug

Setup: a project with cloudflare adapter in hybrid mode and routes strategy set to "include".

output: 'hybrid',
adapter: cloudflare({
  imageService: 'passthrough',
  routes: {
    strategy: 'include',
    include: [
      '/store/item/configure',
      '/api/*',
    ]
  }
})

By the very definition it should only include entries for what I specified manually, but instead it creates more than 100 records that look like this:

{
  "version": 1,
  "include": [
    "/*",
    "/api/*",
    "/*/store/item/configure"
  ],
  "exclude": [
    "/",
    "/uk/",
    "/ru/",
    "/pl/",
    "/de/",
    "/404.html",
    "/index.html",
    "..."
  ]
}

Why is this a problem? Cloudflare refuses to deploy a project and returns this error:

Invalid _routes.json file found at: dist/_routes.json Overlapping rules found. Please make sure that rules ending with a splat (eg. "/api/*") don't overlap any other rules (eg. "/api/foo"). This applies to both include and exclude rules individually.

Btw: Astro is broken on stackblitz again, it just doesn't build xD

What's the expected result?

It's not necesary to add anything to the exclude patterns list and that "/*" at the start of includes is not needed there either.

Link to Minimal Reproducible Example

https://stackblitz.com/edit/github-8ty8mk

Participation

maddsua commented 4 months ago

For now I'm just gonna use a manually generated routes files, but this needs to be fixed, and I think I can take care of this bug if the team gives a green light

alexanderniebuhr commented 4 months ago

@maddsua Thank you for the report. I do think your expected result is not correct. Just to let you know the routes.include, routes.exclude options do extend the _routes.json and don't replace them. So it is correct/expected that you get more definitions than the one you define in your adapter options. If you want fully manual control over the _routes.json file, the correct way is to have your own file in the public folder.

If you still think there is an issue, in the outlined behaviour from me above, we are happy for you to try fix that and submit a PR for review.

maddsua commented 4 months ago

@alexanderniebuhr Thanks for responding! Good to know that those parameters extend _routes.json and not overwrite it, but I have a completely empty _routes.json file (I don't have it at all to be exact) and just using routes.include causes cloudflare to reject the upload.

In my config, this is literally all that I put into it:

include: [
  '/store/item/configure',
  '/api/*',
]

Just 2 paths, and for some reason it adds /* to that and then proceeds to list all the routes that should be excluded from this pattern (it excludes all the static routes, of which there are more than a hundred). For the reference, this is what valid _routes.json looks like for this app:

{
  "version": 1,
  "include": [
    "/api/*",
    "/store/item/configure"
  ],
  "exclude": []
}
alexanderniebuhr commented 4 months ago

If I understand you correctly, the logic does work as expected. You are probably facing an edge case. I would suggest either trying to set routes.strategy: 'include', or use a custom _routes.json file in the public folder

maddsua commented 4 months ago

Umm, I have routes.strategy set to include, that's why I created this issue.

I'm gonna quone the comment line for this options:

For each page that is prerendered and whoose path is matched by an include entry, an entry in the exclude array will be generated.

Problem is, for whatever reason a /* entry gets added to the top of the include list which causes the creation of that huge exclude list and sub sequentially deploy failure

maddsua commented 4 months ago

I gonna jump to the code real quick, but it seems like there was an oopsie and /* insertion just doesn't get disabled for include strategy

maddsua commented 4 months ago

From what I learned so far, this edge case is caused by a /[...api].ts route existing in a project. Ok, fair enuff, but why isn't this behavior also caused by /[...index].astro 🤔

maddsua commented 4 months ago

Well you're right, this is an edge case indeed! It wasn't triggered by /[...index].astro simply because that path wasn't SSR-enabled. Now it makes sense lmao

maddsua commented 4 months ago

But I guess I have a new issue now =)