-
-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
New: astro add
support instructions for integrations and adapters
#946
Conversation
astro add
support instructions for integrations and adaptersastro add
support instructions for integrations and adapters
✅ Deploy Preview for astro-docs-2 ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
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 @bholmesdev! I added a few suggestions 🙌
+ import myadapter from 'myadapter'; | ||
|
||
export default defineConfig({ | ||
+ adapter: myadapter(), |
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.
We are using a specific pattern of camelCase and kebab-case in the SSR page and this pattern is also used in the Netlify Edge Functions adapter ('@astrojs/netlify/edge-functions'). I think it would be nice to use the same pattern everywhere in the docs, what do you think? This need to be changed in a few other places, I'm going to add the changes as suggestions.
+ import myadapter from 'myadapter'; | |
export default defineConfig({ | |
+ adapter: myadapter(), | |
+ import myAdapter from 'my-adapter'; | |
export default defineConfig({ | |
+ adapter: myAdapter(), |
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.
@Yan-Thomas This sadly does NOT work! astro add
will actually set the following when using dashes:
import my from 'my-adapter'
This avoids logging such a shortcoming. I had your solution before testing and discovering this. I know, it's annoying 😓
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.
That's unfortunate! 😢 This means we need to change the pages in where we use this pattern then, I can take care of this tomorrow, thanks for testing and finding this behavior!
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.
Oh boy, good to know! Tbh, I’d rather fix the underlying behavior before editing the docs. Hoping it’s easy to solve 😄 Hang tight!
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.
Just checking, but it seems all of our own adapters and integrations are all single words... so I guess we don't have this issue (of which case to use) when describing any of our own. It seems to only be an issue to document right now because we're trying to use a 2-word phrase "my adapter."
Is there a one-word, generic term that fits so that we avoid the issue? (That isn't "adapter"? 😅 )
Do we provide any suggestions for people naming their own? Is anyone likely to run into a problem choosing a particular multi-word name?
Otherwise, LGTM when you both are happy with the generic placeholder name!
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.
@sarah11918 Think I'll just use example
to fix it 🙃
8779e84
to
2e5473b
Compare
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 good, Ben!
Non-blocking but couple of questions:
-
Is there a reason the adapter reference doesn’t include the same warning as the integrations reference re: default exports? From what I can see, the same seems like it would apply.
-
Do you think it would make sense to link to these from the “Publish to NPM” page? Like
:::tip Want to support `astro add`? See instructions for adding `astro add` support to third-party [integrations](#) and [adapters](#). :::
@delucis That's because we don't update your config for third party adapters! As noted at the end, users are instructed to update their config manually. |
What kind of changes does this PR include?
Description