Update v7 upgrade guide changes#13944
Conversation
✅ Deploy Preview for astro-docs-2 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Lunaria Status Overview🌕 This pull request will trigger status changes. Learn moreBy default, every PR changing files present in the Lunaria configuration's You can change this by adding one of the keywords present in the Tracked Files
Warnings reference
|
matthewp
changed the title
- Move reference/experimental-flags/advanced-routing.mdx to reference/advanced-routing.mdx - Move reference/experimental-flags/route-caching.mdx to reference/route-caching.mdx - Remove experimental references from both pages (titles, config examples, text) - Update sidebar: remove from experimental group, add to reference/other - Update EN internal links in upgrade guide, astro-content, content-loader-reference
github-actions
Bot
added
the
i18n
ArmandPhilippot
left a comment
ArmandPhilippot
left a comment
There was a problem hiding this comment.
The upgrade guide looks to me!
I'll just note that usually we add the breaking changes once the current release no longer supports them. That's why I still haven't merged #13886 (withastro/astro#16749 is not merged yet). Are we sure advanced routing stabilization, route caching stabilization and @astrojs/db removal will be released at the same time?
However, for the pages that are no longer experimental, I think this requires more work. Some contents sound more like a guide than a reference or belong to another page (e.g. render context). I left a few comments to illustrate my thinking.
Also, we'll need to replace the content of https://docs.astro.build/en/guides/integrations-guide/db/ (probably with something similar to https://docs.astro.build/en/guides/integrations-guide/lit/).
And regarding the translations:
We usually remove the translations for experimental flags when they are stabilized instead of renaming them because we do not accept AI translations (at least not reviewed thoroughly by a native speaker).
I know there are not many changes (yet 😄 ) but I can see a few things that are wrong with the French translation of Advanced routing (we still have a few experimental, the non breaking spaces were intentional, and at least one sentence is missing a verb now).
I suspect this might be the same for the Korean translation, and so in any case we'll need a Lunaria directive to not track them.
So, I think it's easier to just remove the translations? I mean:
- it's quite easy to retrieve the deleted file content from the Git history, so translators do not have to retranslate everything.
- if we dispatch the existing content, keeping the translations no longer makes sense.
I can help with the changes if you want! (probably next week, though) But, because this means more changes it might be easier to review with a separate PR for each.
| 'reference/container-reference', | ||
| 'reference/programmatic-reference', | ||
| 'reference/advanced-routing', | ||
| 'reference/route-caching', |
There was a problem hiding this comment.
We usually keep the experimental API at the end:
| 'reference/container-reference', | |
| 'reference/programmatic-reference', | |
| 'reference/advanced-routing', | |
| 'reference/route-caching', | |
| 'reference/advanced-routing', | |
| 'reference/route-caching', | |
| 'reference/container-reference', | |
| 'reference/programmatic-reference', |
| title: Route caching | ||
| sidebar: | ||
| label: Route caching | ||
| i18nReady: false |
There was a problem hiding this comment.
Ah, that explains why there were no translations! Well, we want this to be translated now this is stable:
| i18nReady: false | |
| i18nReady: true |
| @@ -1,5 +1,5 @@ | |||
| --- | |||
| title: Experimental route caching | |||
| title: Route caching | |||
There was a problem hiding this comment.
consistency nit: the other items are all suffixed by API, so it looks a bit odd that two of them are not.
| title: Route caching | |
| title: Route caching API |
But, maybe this should be something like:
| title: Route caching | |
| title: Cache provider API |
Because some contents require a reorganization: some sections belongs to Render context reference and maybe to the Routing guide. And this page could end up talking only about cache providers.
| @@ -1,5 +1,5 @@ | |||
| --- | |||
| title: Experimental advanced routing | |||
| title: Advanced routing | |||
There was a problem hiding this comment.
consistency nit: the other items are all suffixed by API, so it looks a bit odd that two of them are not.
| title: Advanced routing | |
| title: Advanced routing API |
But, I wonder if this shouldn't be a guide (maybe in Routing) rather than a reference? Except "astro/fetch handler reference" which I think makes sense to have next to the other Runtime APIs (astro/app, astro/zod, etc.)
| }); | ||
| ``` | ||
|
|
||
| ## Using route caching |
There was a problem hiding this comment.
I wonder if this section shouldn't be moved to the Routing guide with maybe a link in the Endpoints guide.
|
|
||
| Per-route `cache.set()` calls merge with config-level route rules. Route code can override or extend the defaults set in config. | ||
|
|
||
| ## Cache providers |
There was a problem hiding this comment.
I think it's the true reference for route caching, in the same way as Font Provider and Session Driver.
| ## Error handling | ||
|
|
||
| ### `CacheNotEnabled` | ||
|
|
||
| Thrown when `cache.invalidate()` is called without a configured cache provider. Other cache methods (`set()`, `tags`, `options`) no-op when caching is not configured, logging a one-time warning on the first `set()` call. | ||
|
|
||
| ### `CacheProviderNotFound` | ||
|
|
||
| Thrown at build time when the configured cache provider cannot be resolved. This typically means the package is not installed or the import path is incorrect. |
There was a problem hiding this comment.
I'm not sure this is relevant to keep that since we already have Cache errors reference.
| ## Further reading | ||
|
|
||
| For full details and to give feedback on this experimental API, see the [Route Caching RFC](https://github.com/withastro/roadmap/pull/1245). |
There was a problem hiding this comment.
We no longer need this.
|
|
||
| #### What should I do? | ||
|
|
||
| If you have an existing `src/fetch.ts` file that is not related to advanced routing, you have two options: |
There was a problem hiding this comment.
nit: it seems there are three options (rename the existing file, disable, or choose another name for Astro fetch file)
| If you have an existing `src/fetch.ts` file that is not related to advanced routing, you have two options: | |
| If you have an existing `src/fetch.ts` file that is not related to advanced routing, you have three options: |
| }); | ||
| ``` | ||
|
|
||
| You can also point `fetchFile` to a different file name if you want to use advanced routing but need to keep `src/fetch.ts` for another purpose: |
There was a problem hiding this comment.
And because we use bold for the first two options, maybe this should be also be in bold:
| You can also point `fetchFile` to a different file name if you want to use advanced routing but need to keep `src/fetch.ts` for another purpose: | |
| **Configure `fetchFile` with a different file name** if you want to use advanced routing but need to keep `src/fetch.ts` for another purpose: |
|
When you'll be back, I have applied my thinking in the following PRs:
This might require resolving conflicts when merging, but because of the number of changes, it seems easier to review. Feel free to commit directly if you had other ideas (advanced routing location, route caching guide URL, etc.)! But I think the content should be divided this way to maintain consistency. |
|
Does this mean this PR can be closed? Thanks so much for doing that! |
I think so! I wasn't sure if you would prefer that I rebase the other PRs to your branch to do more edits or make edits directly on my branches if needed.
No problem! I knew you were off most of the last week and because 7 is near I didn't want us to find ourselves in a rush at the last minute. 😄 |
|
Ok going to close, thank a lot! Will take a look at your PRs. |
3 participants
Adds missing sections to the v7 upgrade guide:
rustCompiler,advancedRouting, androuteCachingflag removalsrc/app.ts: Documents thatsrc/app.tsis now reserved for advanced routing, with instructions to rename or setfetchFile: null@astrojs/db: Documents the package removal with alternatives (Node.js built-innode:sqlite, Drizzle ORM, other database libraries)