-
-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
fix: add undocumented options in integrations-reference.mdx
#10822
fix: add undocumented options in integrations-reference.mdx
#10822
Conversation
The `string` type was definitely removed in withastro/astro#11987. Other pages was up-to-date but not the integrations reference.
In overview of the functions available, `injectRoute` and `InjectedType` was the only ones expanded. It might be better to be consistent with the others and since `InjectedRoute` and `InjectedType` can be imported, it could be nice to show the types there.
It was added in withastro/astro#11824 so it seems to be since v5.
It seems it was added in withastro/astro#7821 so in v4.7.0.
The other options of this hooks are not documented so I just added the option in the Quick API Reference and fixed the type in the `astro:build:ssr` type overview.
✅ Deploy Preview for astro-docs-2 ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site 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
|
Amazing, thank you so much for this initiative Armand! As you say, in draft so no rush. But would be great for at least @florian-lefebvre and @Fryuni to have a look over when it's ready before I do! You could also post this in the |
Oh definitely! There still some work to do here (mainly document what is not documented) so I'll try to do that with my knowledge but yeah, once done a review from our integrations expert would be much appreciated! 😄 |
Feel free to ping me once you're ready! |
Added in withastro/astro#4775 so v1.3.0
`entryPoints` added in withastro/astro#7220 so v2.7.0 `middlewareEntryPoint` added in withastro/astro#7532 so 2.8.0
I moved some hooks in the page to match the hooks order. I also fixed some `Next hook`/`Previous hook` which seemed wrong or at least outdated. Tested with a custom integration and logs.
We usually use `addMiddleware()` instead of `addMiddleware` in headings when the type is a function.
Oh okay that's why Actually, I read the description of |
<Since v="4.14.0" /> | ||
</p> | ||
|
||
The `component` property is a readonly string path relative to the project root, e.g `"src/pages/blog/[slug].astro"`. Its value helps you identify the route being set up. |
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 not always true, eg. for injected routes that reference package exports
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.
You mean when the injected routes comes from node_modules
? The path would not be ./node_modules/...
?
Reading the JSDoc, it seems a relative path is always the expected format, right? Looking at the hook implementation, I see route.component
being used in the logger... But I have no idea what else to expect than a path to a file. 😅
Do you have an example of what this might be (or a direction where I could look)?
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 is not only that. The reason we renamed to entrypoint
in the other hook is exactly because it is not always a component. I can be a TS endpoint, for example.
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 understand that, but I have to admit that I am having a bit of trouble understanding the problem with the description (except that the property name is wrong but it is the name currently in use and we are documenting what currently exists).
But again, I'm not the integration expert here... So maybe some things that seem obvious to you, aren't obvious to me due to a lack of knowledge in this context.
I had tested on my own website where I have both static routes, dynamic routes, endpoints, and injected routes (but which do not refer to node_modules
) and in every case, using console.log(route.component)
, I see:
src/pages/en/index.astro
src/pages/en/[slug].astro
src/pages/feed.xml.ts
src/components/atoms/button/button.stories.astro
(stories.astro
are my injected routes if you are wondering)
So it seems to me the description matches what I get. But I haven't tested with an import from a package, so maybe that's the problem...
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 was thinking about a package import
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.
So I tried to find an existing package that I could use to test... there is astrojs/web-vitals
but it still mentions Astro Studio, so I thought maybe it wasn't the best solution...
I went with a fake package (inspired by a fake fixture in the core repo) and pnpm workspaces:
packages:
- "fake-packages/*"
Using logger.info(route.component)
, I see [astro-route-setup] fake-packages/custom-404-pkg/404.astro
.
So it's using a relative path, right? However, perhaps because this is a workspace package, it's using the actual path instead of a path from node_modules
... But I guess for an installed package, it will be node-modules/fake-packages/...
.
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.
@Fryuni you probably know more about this than me
We should update it on the others and mark the |
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 great. Left some small notes for us to discuss.
Co-authored-by: Luiz Ferraz <[email protected]>
Co-authored-by: Luiz Ferraz <[email protected]>
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.
Review on talking and docing was great
Also checked for typos and came up empty
Great job 😄
Co-authored-by: Sarah Rainsberger <[email protected]> Co-authored-by: HiDeoo <[email protected]>
See #10822 Co-authored-by: Thomas Bonnet <[email protected]>
* i18n(fr): update `integrations-reference.mdx` See #10822 * fix locale in a link * add missing space Co-authored-by: Thomas Bonnet <[email protected]> --------- Co-authored-by: Thomas Bonnet <[email protected]>
I was looking at the Integrations reference page and noticed some formatting issues... Since Florian is about to give a talk on integrations, I figured it was a good time to do a more thorough check instead! Also, I was worried that you would be bored after
5.2
. 😄Description (required)
Sorry the description isn't really succinct but I'm trying to keep track of the why and when for each point.
In
upgrade-to/v5.mx
:addDevToolbarApp
code snippets (the path is not the same as the one used in the code block's title)In
integrations-reference.mdx
:Consistent formatting:
Type:
/Since
block to be consistent with the formatting of API referenceaddMiddleware
>addMiddleware()
)+
with ExpressiveCode notation to highlight added lines inAllow installation with astro add
injectRoute
andinjectTypes
consistent with the other options. These were the only objects expanded in the API quick reference, so:InjectedRoute
andInjectedType
can be imported, it could be nice to show the types thereExisting content:
refreshContent
,middlewareEntryPoint
andpages
options inQuick API Reference
logger
option in the type preview of some hooksHooks
section to theAstroIntegrationLogger
since each hook has alogger
optionconsole.log
)Previous hook
/Next hook
links (some was wrong/outdated or the anchor did not match the link)addDevToolbarApp
have been updated in the other pages, but not in this one...string
was removed in feat(next): TODOs astro#11987)injectRoute
,pattern
was displayed twice with a different type (one of them should beprerender
)addWatchFile
is a functionNew content:
buildOutput
option inastro:config:done
(added in Merge output: hybrid and output: static astro#11824, so v5)toolbar
option inastro:server:setup
(added in Fix astro/app import astro#7821, so v4.7.0)astro:build:setup
optionsastro:build:generated
optionsastro:build:ssr
optionsIntegrationResolvedRoute
andIntegrationRouteData
types to avoid too long blocks of code and to make it look more like documentation (see the issue reported by Fryuni on Discord )Remaining question: the
Integrations API
page becomes longer than the current Render context page (formerly API reference) we split. Could a section in the sidebar likeRuntime API
make sense here? Not necessarily now, but the page might get a bit too big for Algolia (~1680 lines, IIRC the former API Reference had a bit more than 2000 lines).Related issues & labels (optional)
add new content
,consistency/formatting
andimprove or update documentation