-
Notifications
You must be signed in to change notification settings - Fork 31
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
Integration System #138
Integration System #138
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,171 @@ | ||
- Start Date: 03-07-2022 | ||
- Reference Issues: <!-- related issues, otherwise leave empty --> | ||
- Implementation PR: <!-- leave empty --> | ||
|
||
# Summary | ||
|
||
An integration system for extending Astro. | ||
|
||
# Example | ||
|
||
```js | ||
// astro.config.js | ||
export default ({ | ||
integrations: [ | ||
import('@astrojs/vue'), | ||
import('@astrojs/tailwind'), | ||
[import('@astrojs/partytown'), {/* options */}], | ||
], | ||
}); | ||
``` | ||
|
||
```js | ||
// Static imports are also supported, for type checking. | ||
import vuePlugin from '@astrojs/vue'; | ||
import tailwindPlugin from '@astrojs/tailwind'; | ||
import partytownPlugin from '@astrojs/partytown'; | ||
|
||
export default ({ | ||
integrations: [ | ||
vuePlugin(), | ||
tailwindPlugin(), | ||
partytownPlugin({/* options */}) | ||
], | ||
}); | ||
``` | ||
|
||
# Motivation | ||
|
||
Astro currently has no integration or plugin system of its own. Some users have been able to hook into Astro's internal Vite plugin system to extend Astro, which lets you control the build pipeline. However, this only gives you access to extend one piece of what Astro does (the build). The rest of Astro remains out of touch. | ||
|
||
Adding a first-class integration system would unlock a few huge wins for Astro: | ||
1. Empower users to do more by extending Astro themselves (not blocked by what Astro core can/can't do). | ||
2. Empower users to do more by reusing shared extensions (easy to add "X" to an Astro project). | ||
3. Empower more user-land experimentation, reducing how much Astro core blocks what a user can/can't do with Astro. | ||
4. Organize our codebase by refactoring more logic into internal integrations or moved out of core entirely into external integrations. | ||
|
||
To illustrate this, compare Partytown's documentation for getting started wtih Astro vs. getting started with Nuxt: | ||
- Astro: [3 steps across frontmatter, template, and a custom npm script](https://partytown.builder.io/astro) | ||
- Nuxt: [1 step](https://partytown.builder.io/nuxt) | ||
|
||
Tailwind suffers from a similar difficult setup story in Astro. | ||
|
||
# Detailed design | ||
|
||
**Background:** This API was reached through weeks of experimentation of different designs (see alternatives below). To test the work, I designed and build the following integrations, which are useful for illustrating this RFC: | ||
|
||
- **Renderers:** [`lit`](https://github.com/withastro/astro/blob/wip-integrations-4/packages/integrations/lit/index.js), [`svelte`](https://github.com/withastro/astro/blob/wip-integrations-4/packages/integrations/svelte/index.js), [`react`](https://github.com/withastro/astro/blob/wip-integrations-4/packages/integrations/react/index.js), [`preact`](https://github.com/withastro/astro/blob/wip-integrations-4/packages/integrations/preact/index.js), [`vue`](https://github.com/withastro/astro/blob/wip-integrations-4/packages/integrations/vue/index.js), [`solid`](https://github.com/withastro/astro/blob/wip-integrations-4/packages/integrations/solid/index.js) | ||
- **Libraries:** [`tailwind`](https://github.com/withastro/astro/blob/wip-integrations-4/packages/integrations/tailwind/index.js), [`partytown`](https://github.com/withastro/astro/blob/wip-integrations-4/packages/integrations/partytown/index.js), [`turbolinks`](https://github.com/withastro/astro/blob/wip-integrations-4/packages/integrations/turbolinks/index.js) | ||
- **Features:** [`sitemap`](https://github.com/withastro/astro/blob/wip-integrations-4/packages/integrations/sitemap/index.js) | ||
|
||
## Integration Usage API | ||
|
||
```js | ||
// astro.config.js | ||
export default ({ | ||
integrations: [ | ||
import('@astrojs/vue'), | ||
// or, with options | ||
[import('@astrojs/vue'), {/* options */}], | ||
// or, as a static ESM import at the top of the file | ||
vuePlugin({/* options */}) | ||
], | ||
}); | ||
``` | ||
|
||
## Integration Author API | ||
|
||
```ts | ||
export interface AstroIntegration { | ||
name: string; | ||
hooks: { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Are there going to be non- If not, I think the prefix is a bit redundant - and it could be safely assumed that anything defined in an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I don't have an answer! This was me being defensive because I wasn't sure and wanted to err on the side of caution. I do agree that any hook we add support for is implicitly an "astro" hook. The two use-cases I could see this being useful for later were:
I generally avoid worrying too much about future features, but in this case if the only cost is a few potentially redundant letters on an API that is mainly for advanced users, then I think that's a strong case for the more verbose style as written. |
||
/** SETUP */ | ||
/** Called on astro startup, lets you modify config */ | ||
'astro:config:setup': (options: ConfigSetupOptions) => void | Promise<void>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you add the interface There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. {
/** not-yet-completed Astro config object, can be modified in-place */
config: AstroConfig;
/** The astro command that was run. */
command: 'dev' | 'build';
/** Add a renderer (ex: React, Svelte). */
addRenderer: (renderer: AstroRenderer) => void;
/** Inject a script. */
injectScript: (stage: 'beforeHydration' | 'head' | 'bundle', content: string) => void;
/** Inject an HTML element. */
injectHtml: (stage: 'head' | 'body', element: string) => void;
} I left this out because this is still being finalized and still subject to change, and hoped we could consider it an implementation detail of the larger integration system. I'm open to all feedback on this now but would also love to keep working on it up the the final PR. |
||
/** Called after config is finalized, lets you store config object for later */ | ||
'astro:config:done': (options: { config: Readonly<AstroConfig> }) => void | Promise<void>; | ||
|
||
/** DEV */ | ||
/** Called on server setup, lets you modify the server */ | ||
'astro:server:setup': (options: { server: vite.ViteDevServer }) => void | Promise<void>; | ||
/** Called on server startup, lets you read the dev server address/URL */ | ||
'astro:server:start': (options: { address: AddressInfo }) => void | Promise<void>; | ||
/** Called on server exit */ | ||
'astro:server:done': () => void | Promise<void>; | ||
|
||
/** BUILD */ | ||
/** Called on build start, lets you modify the server */ | ||
'astro:build:start': () => void | Promise<void>; | ||
/** Called on build done, lets you read metadata about the build */ | ||
'astro:build:done': (options: { pages: string[]; dir: URL }) => void | Promise<void>; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this hook could probably be used by an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure how this would work with the dev server and SSR though? Assuming this array of I could see us dividing 2 separate hooks to address this:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @matthewp I think so. But if it was the legacy build, I think (I haven't checked the source code) it could be possible to write the optimized images to the dist folder during There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @bholmesdev These hooks only run doing There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's hard to say without us having a clear understanding of implementation, but as a concept I really like the idea of building our own features as integrations as a way to dogfood and keep improving the integration API for others.
Comment on lines
+84
to
+100
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A common issue I have faced when writing Vite plugins is that hooks are run multiple times. Will these hooks too get called multiple times? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, that's 100% a commitment here. That's a great example of why having our own integration system is important, since we can't make guarantees about how Vite plugins are run since we aren't the ones running them. |
||
}; | ||
} | ||
``` | ||
|
||
|
||
### Integration Author API: Hooks | ||
|
||
- The **Hook** is the main primitive of this proposed integration system. | ||
- Hooks optimize for maximum flexibility for the integration author: you can use our provided helper methods to perform common tasks during each hook, or write your own custom logic for advanced needs. | ||
- Hooks are conceptually aligned with how Rollup and Vite plugins work. This lets us pass some hooks (like 'astro:server:start') to Vite (the Vite `configureServer()` hook) with trivial effort. | ||
- The `hooks: {}` API conceptually matches Rollup & Vite but was designed to avoid the risk of conflict that would have been introduced had we literally extending the Vite plugin idea. | ||
- This is why we prefix all hooks with `astro:`. | ||
|
||
|
||
# Drawbacks | ||
|
||
- **Breaking changes across an integration system are expensive.** This can be mitigated until v1.0.0, see adoption strategy below. | ||
|
||
|
||
# Alternatives | ||
|
||
A previous design used a functional API more like this: | ||
Comment on lines
+120
to
+122
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I want to call out that 11ty uses this system as well. It has one clear drawback: asynchronous blocks are harder to support. 11ty has some internal reasons why asynchronous config functions aren't possible. But even if async was supported, users may accidentally block crucial, possibly unrelated code in the config. I think dividing Astro's lifecycle into hooks will help us manage promises more effectively! |
||
|
||
``` | ||
export default function(integration) { | ||
integration.injectScript(...); | ||
integration.modifyConfig(...); | ||
integration.configureServer(...); | ||
integration.mountDirectory(...); | ||
} | ||
``` | ||
|
||
This produced nice, linear code but at the expense of internal Astro complexity and limited flexibility that eventually blocked some integrations from being possible: | ||
|
||
1. **Complexity:** Astro had to run the entire integration upfront on startup, and then cache these results for later when needed. Many of the methods (like `configureServer`) ended up acting more like hooks anyway. | ||
2. **Inflexible:** Integrations like Partytown couldn't work with a provided `mountDirectory` helper method because because they need to run their own `fs` logic on the final build directory. | ||
|
||
Advanced use-cases like this essentially required hooks to perform custom logic as needed, so the design shifted away from "helpers that do everything for you" and towards "provide hooks with helpers availble if needed". | ||
|
||
# Adoption strategy | ||
|
||
## Experimental Flag | ||
|
||
This proposal suggests only supporting official integrations to start, and mark 3rd-part integrations as experimental via something like a config flag (`--experimental-integrations`) until we hit `v1.0.0-beta`. | ||
|
||
This would let us test the integration system and respond to user feedback before finalizing. | ||
|
||
## Renderers | ||
|
||
The `renderers` API is deprecated by this proposal, with all `renderers` becoming `integrations`: `@astrojs/renderer-vue` -> `@astrojs/vue`. | ||
|
||
With a lot of work, we could do this in a backwards compatible way. However, I would like to avoid that complexity (and the potential for bugs that comes with it) and do this in a breaking change for the following reasons: | ||
|
||
1. **low-effort to upgrade:** updating your renderers to integrations would involve changing your config file only. A codemod could be provided to make this even easier. | ||
1. **easy to assist:** Unlike past breaking changes, this will be fairly easy for Astro to provide helpful output to migrate from one to the other. | ||
|
||
``` | ||
$ astro build | ||
|
||
Astro renderers are no longer supported! | ||
Update your config to use Astros new integration system: | ||
|
||
- renderers: ["@astrojs/vue"] | ||
+ integrations: [import("@astrojs/vue")] | ||
``` | ||
|
||
# Unresolved questions | ||
|
||
- Bikeshedding all the things | ||
- Can we pair this with an `astro add NAME` CLI command? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. cc @JuanM04 who started exploring this in withastro/astro#2650 |
||
- Do we use this as a chance to remove the built-in renderers, and force users to install all framework integrations themselves as npm packages? I would like to save that for a later breaking change, since it would make this breaking change more complex (see the adoption strategy defined above). |
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 is just personal preference, but I much prefer this syntax as it feels familiar to importing components and styles in an Astro component
Is there a benefit to also supporting the
import("@astrojs/vue")
syntax as well? I've always been a bit thrown off with tools that have multiple ways for config (different file types, extensions, CJS vs. ESM, etc), I'd love to see one supported approach if that's feasibleThere 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.
To be honest... I only see cons with that
import
syntax 😆[nestedArraySyntax, { /** with options */ }]
removes type safety while configuring. You'd need to import a typecast for that{ optionsObject }
to offer any intellisense.import
without anawait
teaches a pretty dangerous pattern. As a user that hasn't seen dynamic imports, I'd assume I could justimport
withoutawait
anywhere in my Astro templates too. This is obviously a pretty dangerous assumption!I say we use the static
import
at the top of the file and ditch the second syntax entirely.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.
lol, I kind of knew this would be unpopular even as I wrote it :) This isn't a hill I'll die on, but to at least make the case for it:
Static imports are more verbose, requiring code in two different places (the import and the
integrations
array entry). If I imagine someone running through a tutorial, I find either of the first two examples much easier to follow:integrations: [ + "tailwind" ]
integrations: [ + import("@astrojs/tailwind") ]
Yes, I know, the irony isn't lost on me that this is the same argument for inline asset references in the template, vs. ESM imports of assets 🙈 .
I miss that you could do this in CJS:
require('@astrojs/tailwind')()
I don't think it's impossible to support two different styles here, but I agree its extra complexity that we shouldn't take on lightly or without good reason. If tutorial authors felt like they had to document both APIs, then that would be a good reason not to pursue this.
Also worth calling out that an awesome
astro add tailwind
CLI command that code-mods your config file for you would solve the "user following a tutorial" problem, which is the main problem I was trying to design for here.