-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
docs: document Environment API breaking changes #12809
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
base: v6
Are you sure you want to change the base?
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
|
ArmandPhilippot
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks Matthew, this looks great. I left a few suggestions, mostly about headings/code snippet consistency.
My main concern is the "RouteData.generate() Removed" section. I don't think we document RouteData directly, but I can see it used in the Integrations page to transmit properties. Looking at the implementation PR, it seems generate() still exist under another format. So, maybe this is the change we want to document here? (but maybe I'm wrong, the implementation PR contains a lot of changes, I haven't checked all the details yet 😅 )
Also, while I was quickly looking at the implementation PR, I noticed we have two new properties for setAdapter(): we'll need to document them in the Adapter API guide under Building an adapter. We now have a subheading for each available prop, so we'll need to add devEntrypoint and entryType there.
Co-authored-by: Armand Philippot <[email protected]>
Co-authored-by: Armand Philippot <[email protected]>
Co-authored-by: Armand Philippot <[email protected]>
Co-authored-by: Armand Philippot <[email protected]>
Co-authored-by: Armand Philippot <[email protected]>
| ## Environment API Changes | ||
|
|
||
| Astro v6.0 introduces significant changes to how Astro manages different runtime environments (client, server, and prerender) using Vite's new Environments API. Integration and adapter maintainers should pay special attention to these changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this @matthewp ! Letting you know that I'm working on this, because it's a lot of content especially at the beginning of the guide that probably only affects integration/adapter authors. I'm thinking we could handle it similar to our removing experimental flags: mention the changes and link to the item entry that shows up in its regular place below:
So my suggestion would be to address the Vite Environment API briefly, like a dependency (it kind of is, as a thing we're using) maybe something like:
| ## Environment API Changes | |
| Astro v6.0 introduces significant changes to how Astro manages different runtime environments (client, server, and prerender) using Vite's new Environments API. Integration and adapter maintainers should pay special attention to these changes. | |
| ### Vite Environment API | |
| <SourcePR number="14306" title="feat: integrate vite environments"/> | |
| Astro v6.0 introduces significant changes to how Astro manages different runtime environments (client, server, and prerender) after an internal refactor to use [Vite's new Environments API](https://vite.dev/guide/api-environment). | |
| Integration and adapter maintainers should pay special attention to changes affecting these parts of the Integration API and Adapter API: | |
| - [integration hooks and HMR access patterns](#changed-integration-hooks-and-hmr-access-patterns-integration-api) | |
| - [`SSRManifest` structure](#changed-ssrmanifest-interface-structure-adapter-api) | |
| - [generating routes with `RouteData`](#removed-routedatagenerate-integration-api) | |
| - [routes with percent-encoded percent signs (e.g. `%25`)](#removed-percent-encoding-in-routes) | |
| - [astro:ssr-manifest` virtual module](#removed-astrossrmanifest-virtual-module-integration-api) |
And then that's it, short and sweet, and those 5 entries of things that were changed/removed etc. we would add to the lists below. Part of my thinking is yes, these changes were all made when implementing Vite's API, but as breaking changes, they aren't otherwise in their own "category", and people still need to see ALL the breaking adapter/integration API changes. So, this makes people aware that there was a big refactor and lots of changes to those APIs as a result, but doesn't make e.g. someone who just needs to know how to update their content collections navigate through/past all this.
So, I'm working on a version that then moves the content below into place, and traces each of those changes back to the rest of docs to the thing that they relate to to make sure that e.g. removed things are in fact removed from existing docs.
That means that @ArmandPhilippot and I might have some questions to verify regular docs content too as we go through this! (I already suspect that RouteData.generate needs some attention, because I already came across something in an earlier PR with @florian-lefebvre that our docs currentlysay RouteData was deprecated and to use the IntegrationsRouteData (which is a subset of the internal RouteData I believe?)
So be prepared for some questions as we work through this, but I think something like my suggestion above might be a good way to handle this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copy pasting from Discord
Re "RouteData.generate()": I think it makes sense to document
RouteDataon the adapter reference but not on the integrations reference (because they haveIntegrationRouteResolvedor something, hence why it was marked as deprecated there). And aboutgenerate()in particular, I don't remember exactly but I think there's a utility instead now
|
I just added a commit to reorganize and start to put these in our format.
|
|
@sarah11918 to understand what is next, it looks like you've made the changes from the comment yourself. Is there anything remaining for me to do? I need to find places in the regular docs related to these changes and update those? Anything else? |
|
@matthewp I hope it's not too inconvenient, since I needed to move things around it was easier for me to make a single commit. I normally prefer leaving suggestions, but that wouldn't have been efficient here! What you can do:
What I will do with @ArmandPhilippot :
|
sarah11918
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, left a note on each individual entry with either "docs don't need updating" confirmation, or the things for @matthewp to look at current docs to see if updates are necessary there (and/or questions for @ArmandPhilippot to help get that done!)
So current Matthew tasks are:
- review each individual entry as written here for the upgrade guide and make any suggestions needed
- address each of these following 7 comments (you can resolve the ones that say no docs changes needed!) so we know we're aligning docs with these updates
| ### Removed: `astro:ssr-manifest` virtual module (Integration API) | ||
| {/* I can't find any record of this ever being documented anywhere in docs, but I am assuming it was deprecated at some point, and since it's removed, we don't need anyting in docs now */} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| ### Removed: `astro:ssr-manifest` virtual module (Integration API) | |
| {/* I can't find any record of this ever being documented anywhere in docs, but I am assuming it was deprecated at some point, and since it's removed, we don't need anyting in docs now */} | |
| ### Removed: `astro:ssr-manifest` virtual module (Integration API) |
I don't believe we've ever showed using this module in docs. But, Armand found that this was publicly exposed. So, we can document its removal to be safe, but no changes to docs should be needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it was never talked about, just created for the bling project that briefly existed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't knowif you want me to accept the commit or not, so I'll let you do that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! The commit was just to remove my code comment here that was a placeholder, so I can totally do that!
|
|
||
| <ReadMore>Learn more about [dynamic SSG routes with `getStaticPaths()`](/en/guides/routing/#static-ssg-mode).</ReadMore> | ||
|
|
||
| ### Changed: Integration hooks and HMR access patterns (Integration API) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things to check:
The use of target in 'astro:build:setup' is shown in a few places on the Integration API page (but not the Adapter API at all):
https://docs.astro.build/en/reference/integrations-reference/#quick-api-reference
In the Quick API reference code block:
- is the
targetparameter of 'astro:build:setup' removed entirely? Or does it still exist, it's just not used withvite?
https://docs.astro.build/en/reference/integrations-reference/#astrobuildsetup
In the API reference itself for /en/reference/integrations-reference/#astrobuildsetup
- also remove mention of
targethere? Is this still valid use, or has the parameter been removed entirely?
https://docs.astro.build/en/reference/integrations-reference/#target-option
API reference for target itself
- has this been updated or removed? Does it exist for the usage shown, or does that need updating too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my understanding, this only means the if condition is no longer required because somehow vite.environments will be able to infer the current environment. So, target is still there, with the same type. I don't think we need any changes there.
(Because it's not necessarily easy to browse all the files in the PR, I pulled the branch locally to check the current code in that branch.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might not have actually been removed yet but it has no functional use-case any more, so I would remove all documentation about it. Using vite environments config is the way to do what you used to use this for.
I'll make a note to make sure we remove it from the code.
|
|
||
| <ReadMore>Learn more about [Vite Environments API](https://vite.dev/guide/api-environment) and [integration hooks](/en/reference/integrations-reference/#astrobuildsetup).</ReadMore> | ||
|
|
||
| ### Changed: `SSRManifest` interface structure (Adapter API) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Things to check:
On the Integrations API page:
Is the typing manifest: SerializedSSRManifest; still correct in code samples (it's just that SerializedSSRManifest itself has changed)? Will note: we don't mention SSRManifest at all, we only ever refer to SerializedSSRManifest
https://docs.astro.build/en/reference/integrations-reference/#manifest-option
- we link to source code to define that, rather than to something else in docs. We will have to verify this link (assuming that we do not move this into docs proper)
On the Adapter API page:
- we describe in types
(manifest: SSRManifest, .....)(start()andcreateExports()), again, assuming that sayingmanifestis of typeSSRManifestdoes not need updating just becauseSSRManifestmight have changed
https://docs.astro.build/en/reference/adapter-reference/#envgetsecret
In describing env support, we also import and use SSRManifest type, which should be fine.
ALSO CHECKING re: createExports() function -- I know the implementation PR documents in a @astrojs/cloudflare major that this function is no longer the pattern to use for creating a custom entrypoint. BUT, we are not removing this function? It still exists and should be documented here, and still works the same? (It's just no longer used for that particular purpose)?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(it's just that SerializedSSRManifest itself has changed)?
SSRManifest changed and, indirectly, SerializedSSRManifest because this type is based on SSRManifest. But because we don't documented any of those types, I don't think the docs need an update here outside the upgrade guide.
Will note: we don't mention SSRManifest at all, we only ever refer to SerializedSSRManifest
I don't think SSRManifest is directly available for integrations authors, only for adapters authors. So, I don't think this affects our current docs.
We will have to verify this link (assuming that we do not move this into docs proper)
The link is still relevant. But, yes, this should probably be proper docs. I recall adding this link based on feedbacks but I don't recall the full context (too internal? only missing docs?). A lot happens since I've updated that page. 😄
On the Adapter API page:
Yeah, I don't think this affects the current docs because we don't document what SSRManifest is, so the changes are currently "invisible" regarding docs. Only the upgrade guide requires this info I think.
re: createExports() function
I don't think I can answer that one. I was already in an unfamiliar directory when I refactored that page. I noticed the same thing as you, but honestly, I don't know.
Regarding the recurrent question around SSRManifest and SerializedSSRManifest:
Because this is public, yes, we should consider documenting them... Not sure where though, because SSRManifest is only relevant for Adapters authors (I think?). So, linking from the Integration API to the Adapter API (to avoid repeating props description) might be a bit awkward. Something to think about, but not directly related to this PR. This can be done in main (with proper updates for v6), or in v6. No need to do this with this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
SerializedSSRManifest is for the astro:build:ssr hook, that has changed indirectly as @ArmandPhilippot says.
BUT, we are not removing this function? It still exists and should be documented here, and still works the same?
Yes, this still exists and works exactly the same as before.
| {/* These shouldn't be a breaking change, since they're new. But they should be documented in the appropriate API page if these are publicly available! */} | ||
| {/* STUFF FOR REGULAR DOCS, PROBABLY?? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't believe any of this stuff is documented anywhere. I don't know whether we should? If it's new properties available to use, we normally wouldn't include that in a breaking changes guide (because adding new things generally isn't breaking). Is this the case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- The
srcDirchange should be here - The
hrefRootremoval too, even if it is not documented, because this is currently available for Adapters authors (at least) - But, yes, I believe the new properties should not be there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds right to me.
| **Removed `hrefRoot` property:** | ||
| The `hrefRoot` property is no longer available on the manifest. | ||
| **Async methods for dynamic data:** | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops! These got stuck in the code comment, and are likely breaking changes since they are removed or updated and should be reintroduced!
(It does feel weird to say "here are new changed properties for this thing we don't actually show in docs" though. @ArmandPhilippot do you think it's worth figuring out how to document this one thing in the docs itself, even if the page is not totally updated vs. linking to the code sample?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure what you mean by "vs. linking to the code sample".
I don't know how many people use these properties, but because they are available in manifest we'll need to document them at some point. Now, I don't think this needs to be now and only mentioning this in the upgrade guide is enough?
Sorry, if I don't fully answer your question. As I said, I'm not sure I got it right. 😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I struggled with this too, these are deep APIs for adapters only, and not stuff we even intended for them to use other than to pass to the App class. I think we've only ever documented new App(manifest). But since the adapter does get this object it might use it for other means. That makes it difficult to document in this way since we don't really know the use-cases.
Happy with whatever solution you prefer.
| <ReadMore>Learn more about [the `astro:config` virtual module](/en/reference/modules/astro-config/).</ReadMore> | ||
| ### Removed: `RouteData.generate()` (Integration API) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is the one that will need the most figuring out from our end, so starting that thread here.
In the v5 upgrade guide, we had an entry about replacing RouteData with IntegrationRouteData. We gave the instruction:
Update your adapter to change the type of entryPoints from RouteData to IntegrationRouteData.
And maybe we didn't update the docs properly last time, or maybe this isn't related, but there are still places where we show entrypoint: RouteData['component'];. As I think Armand mentioned earlier, I think RouteData is internal, or meant to be, and IntegrationRouteData might be the subset that is meant to actually be used?
But admittedly, I'm not familiar here, and maybe I need Armand to be asking the questions to clarify whether we mean to be showing RouteData anywhere? Whether we mean for everyone to be using IntegrationRouteData everywhere instead? These are questions I might not even be asking properly!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah I think changing this to IntegrationRouteData is fair. The RouteData object is available to adapters via the manifest, but the most part IntegrationRouteData is where people are more likely to use this (in the hooks). It's mostly the same object so documenting that this property is removed still seems worthy to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just checked: RouteData is importable but I don't know if this a side effect or not. When I look at a core file importing it, it comes from types/public/internal.js... So, yeah, this sounds like internal. 😅
But this is available for users? I can see it in use in RenderOptions (Adapter API):
/**
* **Advanced API**: you probably do not need to use this.
*
* Default: `app.match(request)`
*/
routeData?: RouteData;So, someone could have used routeData.generate().
I'm not sure if this is accessible elsewhere. But because this is importable and usable through an "Advanded API", documenting RouteData.generate() removal in the upgrade guide probably makes sense.
And, it doesn't seem the API available for Integrations authors has changed? I think I got confused when I was looking at the diff earlier:
-generate: route.generate,
+generate: getRouteGenerator(route.segments, trailingSlash),But I can see, in the branch, that IntegrationResolvedRoute (the type replacing IntegrationRouteData) still uses the types we use in docs:
generate: (data?: any) => string;
So, I wasn't sure why my previous comment was resolved but, now I'm looking at the codebase and not only at the PR, I think this is okay.
In the v5 upgrade guide, we had an entry about replacing RouteData with IntegrationRouteData. We gave the instruction:
Yeah, I don't recall when IntegrationRouteData has been deprecated but maybe we should have used IntegrationResolvedRoute, here? (see also #12789)
We don't have any Since for them... not sure if this because both were available for a long time or if this is an oversight from me (ie. I thought about using <Since /> for functions, but not for types).
As I think Armand mentioned earlier, I think RouteData is internal, or meant to be
From what I see now, RouteData is a bit tricky: this sounds like it was meant to be internal but got exposed at some point for advanced use case only? That's probably why we don't document it... But because this is importable and available for Adapter authors, maybe we should (that's also what Florian suggests in the issue I linked above)? And maybe this should be moved outside of types/internal in core?
and IntegrationRouteData might be the subset that is meant to actually be used?
Not quite a subset, but it reuses some properties to avoid redeclaring the same types (probably because internally they are tied). So, from what I see RouteData is available in adapters and IntegrationResolvedRoute (the replacement of IntegrationRouteData) in integrations.
But admittedly, I'm not familiar here, and maybe I need Armand to be asking the questions to clarify whether we mean to be showing RouteData anywhere? Whether we mean for everyone to be using IntegrationRouteData everywhere instead? These are questions I might not even be asking properly!
My answer is quite verbose but to summarize: I think the current docs for the upgrade guide is fine.
However, yes, I think there are some work to do around RouteData:
- is this internal? Should this type be moved outside
types/internalto make it more clear if this is meant to be public or not? - if this is not meant to be public, we probably need to update the
RouteData["something"]references in docs to show the actual types? - if this is meant to be public, we probably need to document
RouteDatasomewhere to avoid linking to the code source directly and to give proper docs for the properties available there. Even if this is advanced use cases, if this is public I think we should have docs for this?
And final though: unless I missed something here, maybe the heading should refer to Adapter API instead of Integration API?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's meant to be public only in that its on the manifest which is available to adapters. Otherwise no, I don't believe so.
Co-authored-by: Armand Philippot <[email protected]>
Description (required)
Updates the v6 guide with breaking changes from the Environment API refactor
Related issues & labels (optional)
For Astro version:
6.x. See astro PR #14306.