-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
refactor(adapter-reference): improve adapter API docs #12673
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
refactor(adapter-reference): improve adapter API docs #12673
Conversation
✅ Deploy Preview for astro-docs-2 ready!Built without sensitive environment variables
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
|
Co-authored-by: Matt Kane <m@mk.gg>
Co-authored-by: Matt Kane <m@mk.gg>
florian-lefebvre
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.
This looks great! I always found our Adapter API docs a bit weak compared to the Integration API ones, love it!
Co-authored-by: Matt Kane <m@mk.gg> Co-authored-by: Sarah Rainsberger <5098874+sarah11918@users.noreply.github.com>
Co-authored-by: Sarah Rainsberger <5098874+sarah11918@users.noreply.github.com>
Co-authored-by: Sarah Rainsberger <5098874+sarah11918@users.noreply.github.com>
Co-authored-by: Armand Philippot <git@armand.philippot.eu>
|
I think everyone has done some amazing work supporting @ArmandPhilippot 's efforts! Only two points where I feel we really need to do a tiny bit better (called in both Matt and Florian to help), and once those feel good, this puppy LGTM! |
|
Thanks everyone for the really helpful feedbacks! ❤️ I added a new While doing so, I noticed we document the methods available from
We also have some static methods for
I'm not quite sure how to make the distinction between the static methods and the others with the current structure.
|
|
@florian-lefebvre Are you available to give feedback on Armand's latest commit and questions? |
Co-authored-by: Armand Philippot <git@armand.philippot.eu>
|
I think we should document static methods as well, they are useful. It's fine to document these alongside the standard methods and just say they're static with a code example. For example usage of the node app specific static methods, I think this file is pretty good: https://github.com/withastro/astro/blob/5dbd59bdced080e3a9ae95ad91bbe737a2e3ba55/packages/integrations/node/src/serve-app.ts |
|
Thanks, I just pushed a new commit to document them. Please check the accuracy because I'm not really sure how this can be used (e.g. I wrap I added And, if anyone want to check the versions:
|
|
New static methods were just added to address a CVE FYI withastro/astro#14743 |
|
Oh thanks! I wouldn't have thought to check the latest version... I'll add them to the docs tomorrow! |
Co-authored-by: Matt Kane <m@mk.gg> Co-authored-by: Sarah Rainsberger <5098874+sarah11918@users.noreply.github.com>
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.
Great work, Armand! Just noting that when you're happy, I'm happy! (You can also request a Yan final boss review at any time!)
|
Thanks! And yeah, I'm glad we now have all those public APIs documented! But I must say, I didn't expected that when I started: And yes, a review from @yanthomasdev is always useful, if you have any spare time! 😄 Same for Florian or Matt, if you have time to check the accuracy of the new entries it would be helpful. |
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.
Looks pretty good! I think the the app methods could use more concrete examples perhaps (or maybe that'd belong to a guide, idk) but not blocking at all! This PR is big enough already, thanks a lot!
yanthomasdev
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.
Good work, here's my final boss
Co-authored-by: Yan <61414485+yanthomasdev@users.noreply.github.com>
|
I agree with Florian, some examples could probably be improved, but without building an adapter to determine what constitutes a useful minimal code snippet, it's a bit difficult (for me) to trim what is necessary or not in the current adapters. Showing too much/too specific code could also be confusing for users I think. At least the docs are now a bit more detailed for those who wish to build their own adapter! So, this is NWTWWHB and we can always improve the code snippets later if/when we get feedbacks. I think this looks ready to be merged! Thank you all again for your help! |

Description (required)
<Since />.createExports(): a second argument exists to retrieve data fromsetAdapter().astro/appAPIs:app.getAllowedDomains(),app.removeBase(),app.setCookieHeaders()app.match()type: a second argument exists.envGetSecretfrom "Adapter features" to "Astro features".h2.Exports>createExports()andStart>start()in "Build a server entrypoint" with some explanations about their shape.See the relevant links to check types/default/since:
experimentalStaticHeaders: added in feat(csp): add static header adapter function astro#13926AdapterSupportandAdapterSupportWithMessage: added in refactor: rework supportedAstroFeatures astro#11806suppressinAdapterSupportWithMessage: added in Add option to suppress adapter feature support log astro#13842previewEntrypoint: added in Node.js standalone mode + support for astro preview astro#5056client: added in Allow adapters to customize headers for fetch requests astro#14543adapterFeatures: added in feat: adapter features, deprecate astro configs astro#7839buildOutput: added in feat(hybrid): Clean logging and misc tweaks for hybrid removal astro#11942 (and feat: changesets for the hybrid removal astro#11941 for the changesets)supportedAstroFeatures: added in feat: astro features astro#7815envGetSecret: added in feat(astro): experimental astro:env astro#10974i18nDomains: added in feat(i18n): domain support astro#9143 (there was ani18nproperty before, but since this uses another name, this is "new")sharpImageService: added in refactor: rework supportedAstroFeatures astro#11806envGetSecretandi18nDomainswere added at the same time as the corresponding experimental feature, I'm not sure if we should use the stable release or not for them. Currently, I'm using the version of the experimental release.astro/apppublic APIs (only the missing ones):getAdapterLogger: added in refactor: use new Astro internals astro#8254 but I can't find it in the changelog... looking at the merge data, this was 2 days beforev3.0.0so I think it's fine to use this version.getAllowedDomains: added in withastro/astro@6ee63bf which matchesv5.14.2removeBase: add in Handlebasein adapters astro#5290setCookieHeaders: added in Astro.cookies implementation astro#4876setManifestData: added in refactor: use SSR manifest in dev astro#7587Room for improvement: except for
envGetSecretthat was already documented, theAstro featuressection is pretty succinct. I'm not sure how these features can/should be used, so I have not developed them.Not documented
app.setManifestDatais publicly available when creating a newApp(astro/app) but I couldn't find an usage in the source code so I wonder if this is a leftover that should be removed or something like that.Related issues & labels (optional)
add new content,consistency/formatting,improve or update documentation