fix(cloudflare): added config for _routes.json generation

[schummar]

Changes

Follow up to #7846

• Adds options to switch between the different strategies when generating _routes.json. By default the strategy producing the smaller file will be chosen. If users want more control over it, they can override it by setting routes.strategy to "include" (current default behavior) or "exclude" (old default behavior).
• Additional options to add manual include and exclude entries.

export default defineConfig({
    adapter: cloudflare({
        mode: 'directory',
+       routes: {
+           strategy: 'include',
+           include: ['/foo'],
+           exclude: ['/bar'],
+       },
    }),
});

Testing

Added test cases ✅

Docs

Added docs ✅
/cc @withastro/maintainers-docs for feedback!

[changeset-bot]

🦋 Changeset detected

Latest commit: b07d3ff

The changes in this PR will be included in the next version bump.

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[sarah11918]

Thanks for providing this documentation @schummar! I left a comment re: the changelog, but I have more questions regarding the README before I can continue reviewing.

I'm confused about whether the 2nd and 3rd items are in fact the options for routes.strategy. Would that not make them e.g. routes.strategy.include and routes.strategy.exclude? Or, are they in fact other routes configurations (so you're adding three new configurations to routes )?

Because they are named identically, it is easy to wonder whether these are in fact the same thing, so a code configuration example would be helpful. Can you show an example of a configuration here? And then, I'll review the README! 🙌

EDIT: D'oh! This is once again the case of "devs put helpful stuff in the PR description that should be in the docs!" 😉

Reminder to all who go here: Most of this stuff goes in the docs! If you're telling your fellow devs about it, then maybe the users would benefit from knowing, too! 🙌

[sarah11918]

OK, @schummar , I found your code example and included it! 😄

Also, please see my comments re: documenting the config options. I'd also love for @alexanderniebuhr and @ematipico to take a look at my comments and provide guidance here!

(Admittedly, the details go beyond my direct knowledge and experience, but my role is to massage the docs to be as consistent as possible, and as accessible as possible to a wide audience and range of experience levels! So thank you in advance for helping me do that! 🙌 )

[alexanderniebuhr]

I have a few suggestions to enhance this:

1. To maintain consistency, could we rename the test file as follows?

renamed:    packages/integrations/cloudflare/test/routes-json.js -> packages/integrations/cloudflare/test/routes-json.test.js

2. I'd love to see a variety of test cases (non-blocking for me), such as:

• Tests for mode: 'advanced'
• Tests for both include & exclude strategy using a custom function (they should target the same files but with different syntax)
• Tests for functionPerRoute: true
• A test incorporating assets

3. Lastly, I'm concerned that the include strategy might not address all edge cases. (During my manual testing, I encountered some issues. I'd be happy to discuss this on a Discord thread in case I missed something.)
   If that's the case, I'd suggest not setting the default to auto. Perhaps making exclude the default would be more appropriate.

UPDATE: Please ignore point 3 for now. I believe I've discovered an existing bug that I need to address. I'm still in the process of evaluating it.

[sarah11918]

Just putting this here so I get notified when I should come back and look at updated documentation! 🙌

[schummar]

I have made some update but did not address all feedback yet. Will continue later, I'll notify you, when I'm done.

[schummar]

I have a few suggestions to enhance this:

1. To maintain consistency, could we rename the test file as follows?

renamed:    packages/integrations/cloudflare/test/routes-json.js -> packages/integrations/cloudflare/test/routes-json.test.js

Sorry for missing the ".test" part. Maybe third time's the charm 😁. I did the same adjustment for the test's fixtures, renaming the path from routesJson to routes-json.

2. I'd love to see a variety of test cases (non-blocking for me), such as:

• Tests for mode: 'advanced'

Done. Executing all test cases for both modes now.

• Tests for both include & exclude strategy using a custom function (they should target the same files but with different syntax)

Is this not already covered by the "with additional include entries"/"with additional exclude entries" test cases? Files do not really play a role here. The content of the functions folder is not considered at all, the user supplied values are simply added to the include/exclude arrays.

• Tests for functionPerRoute: true

Do you expect that option has any impact on the resulting _routes.json file? It should only influence the content of the functions folder and that does not play a role here.

• A test incorporating assets

I've added an asset file as well as a _redirects file to the fixtures' public folder.

3. Lastly, I'm concerned that the include strategy might not address all edge cases. (During my manual testing, I encountered some issues. I'd be happy to discuss this on a Discord thread in case I missed something.)
   If that's the case, I'd suggest not setting the default to auto. Perhaps making exclude the default would be more appropriate.

UPDATE: Please ignore point 3 for now. I believe I've discovered an existing bug that I need to address. I'm still in the process of evaluating it.

That is still an interesting question: Defaulting to the exclude strategy might be safer. However when having mostly static pages and only a few dynamic ones the include/auto strategies usually produce a lot better _routes.json files. As far as I know this is the most common use case for Astro, right? People will run into the 100 entries limit very frequently.

[alexanderniebuhr]

Is this not already covered by the "with additional include entries"/"with additional exclude entries" test cases? Files do not really play a role here. The content of the functions folder is not considered at all, the user supplied values are simply added to the include/exclude arrays.

I believe that users shouldn't be required to add custom functions manually. Nonetheless, this discussion is beyond the scope of this PR. It looks good to me!

Do you expect that option has any impact on the resulting _routes.json file? It should only influence the content of the functions folder and that does not play a role here.

Could you clarify this point? I'm uncertain why the functions folder is deemed irrelevant. The functionPerRoute will generate multiple files within the functions folder, as opposed to a singular [[path]].js. It's essential that these files are processed automatically to ensure accurate output for both modes (include/exclude) in the _routes.json.

As far as I know this is the most common use case for Astro, right? People will run into the 100 entries limit very frequently.

Indeed, the current default seems appropriate. However, if we start receiving feedback or reports, we can consider adjusting the default settings in the future.

[schummar]

Could you clarify this point? I'm uncertain why the functions folder is deemed irrelevant. The functionPerRoute will generate multiple files within the functions folder, as opposed to a singular [[path]].js. It's essential that these files are processed automatically to ensure accurate output for both modes (include/exclude) in the _routes.json.

What I mean is, the content of the functions folder is not take into account when generating the _routes.json file. As far as I know, functionPerRoute also does not change the content of the routes and pages variables. However, since I don't understand all implications, maybe it's better to just test it as well. Better safe than sorry. => I have adjusted it now.

When a future PR indroduces analzying custom functions in the functions folder, testing with different functionPerRoute settings will become essential anyway.

[alexanderniebuhr]

does not change the content of the routes and pages variables

not sure, but at least you should be able to rely on them. But thanks for adding tests :)

[schummar]

Just putting this here so I get notified when I should come back and look at updated documentation! 🙌

@sarah11918 are the docs ok now?

[sarah11918]

Thank you @schummar , the description of the three options is so much more user-friendly! I appreciate the extra attention there! Just one tiny comment on the separate include and exclude options for clarification, but these docs are looking great, so I've removed the blocking review.

As soon as everyone here is confident my comment below is satisfactorily represented in the docs, this will be good to go!

removing the block as the docs are in very good shape!

[alexanderniebuhr]

Great contribution. LGTM!