Skip to content
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

Merged
merged 4 commits into from
Mar 16, 2022
Merged

Integration System #138

merged 4 commits into from
Mar 16, 2022

Conversation

FredKSchott
Copy link
Member

@FredKSchott FredKSchott commented Mar 8, 2022

  • Start Date: 03-07-2022
  • Status: Draft

Summary

An integration system for extending Astro.

// astro.config.js
export default ({
  integrations: [
    import('@astrojs/vue'),
    import('@astrojs/tailwind'),
    [import('@astrojs/partytown'), {/* options */}],
  ],
});

// Static imports are also supported, 
// for users who want type checking.
// import vuePlugin from '@astrojs/vue';

Walkthrough from Tuesday, March 7:

Links

@FredKSchott FredKSchott marked this pull request as ready for review March 10, 2022 02:39
# Unresolved questions

- Bikeshedding all the things
- Can we pair this with an `astro add NAME` CLI command?
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

cc @JuanM04 who started exploring this in withastro/astro#2650

@FredKSchott FredKSchott changed the title [WIP] Integration System Integration System Mar 10, 2022
```ts
export interface AstroIntegration {
name: string;
hooks: {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there going to be non-astro: namespace hooks available here as well?

If not, I think the prefix is a bit redundant - and it could be safely assumed that anything defined in an AstroIntegration will be Astro related. In that case, it might be better to make the clarification/distinction between Vite hooks in the docs rather than in the API.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there going to be non-astro: namespace hooks available here as well?

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:

  • future custom hooks ("storybook:build" or "segment:event", for integrations that extend other integrations).
  • future support for vite hooks in an Astro integration ("vite:load", "vite:transform") as sugar for providing a Vite plugin in the astro:config:setup hook.)

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.

/** 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>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this hook could probably be used by an Image component to write optimized images to the dist folder, does that sound right?

Copy link
Contributor

@bholmesdev bholmesdev Mar 10, 2022

Choose a reason for hiding this comment

The 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 pages is the raw, compiled HTML, I don't think we could allow DOM crawling and parsing on SSR in particular.

I could see us dividing 2 separate hooks to address this: astro:compile and astro:render.

  • Compile: Gives you plain metadata about each page like pathname, dir, and potentially a metadata object if the meta proposal A single "meta" export for static frontmatter #135 is considered. We could also include an AST of some sort if we get that production-ready down the road. Image optimization would ideally happen here since SSR routes would also get picked up at build time.
  • Render: Gives you the rendered HTML output. Image optimization can happen here, as long as you're using optimization libraries fast enough for serverless requests.

Copy link
Member

@RafidMuhymin RafidMuhymin Mar 10, 2022

Choose a reason for hiding this comment

The 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 astro:build:start.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bholmesdev These hooks only run doing astro build and astro dev, not in SSR. The sort of image optimizations I'm thinking of could probably only be done for an SSG site.

Copy link
Member Author

@FredKSchott FredKSchott Mar 10, 2022

Choose a reason for hiding this comment

The 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.

hooks: {
/** SETUP */
/** Called on astro startup, lets you modify config */
'astro:config:setup': (options: ConfigSetupOptions) => void | Promise<void>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add the interface ConfigSetupOptions so we can see what is possible in this hook?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

@matthewp
Copy link
Contributor

I really like it. Some questions about details. Tacit approval from me assuming the details can be ironed out.

Comment on lines +120 to +122
# Alternatives

A previous design used a functional API more like this:
Copy link
Contributor

Choose a reason for hiding this comment

The 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!

Comment on lines +84 to +100
'astro:config:setup': (options: ConfigSetupOptions) => void | Promise<void>;
/** 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>;
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Comment on lines +23 to +34
// 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 */})
],
});

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 feasible

Copy link
Contributor

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 😆

  1. Allowing the [nestedArraySyntax, { /** with options */ }] removes type safety while configuring. You'd need to import a typecast for that { optionsObject } to offer any intellisense.
  2. Allowing users to import without an await teaches a pretty dangerous pattern. As a user that hasn't seen dynamic imports, I'd assume I could just import without await anywhere in my Astro templates too. This is obviously a pretty dangerous assumption!
  3. Allowing 2 different syntaxes could introduce all sorts of confusion for tutorial writers and boilerplate users.

I say we use the static import at the top of the file and ditch the second syntax entirely.

Copy link
Member Author

@FredKSchott FredKSchott Mar 10, 2022

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")
]
+ // At the top of your file:
+ import tailwindPlugin from '@astrojs/tailwind';

+ // In the config:
integrations: [
+  tailwindPlugin(),
]

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.

@bholmesdev
Copy link
Contributor

bholmesdev commented Mar 15, 2022

RFC call notes: 03-15-2022

  • 🚀 Reached consensus on the idea of "integrations" and current hooks proposed
  • 🚀 Reached consensus on dissolving renderers into the integrations API
  • Questions for future exploration:
    • What params should astro:build:done provide? Should the SSR manifest be included in the list of rendered pages?
    • How can integrations run on SSR routes? Do we need a way to exclude integration dependencies from the serverless deployment (i.e. don't ship the tailwind dependency to all your SSR routes)?

@FredKSchott FredKSchott merged commit edc5450 into main Mar 16, 2022
@FredKSchott FredKSchott deleted the integrations branch March 16, 2022 22:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants