Astro Integration System #2820
Astro Integration System #2820
Conversation
|
github-actions
bot
added
pkg: astro
| export default function createPlugin(): AstroIntegration { | ||
| let config: AstroConfig; | ||
| return { | ||
| name: '@astrojs/sitemap', |
There was a problem hiding this comment.
I really like this integration, glad to see this kind of thing can be external to the build now.
| } | ||
| } | ||
|
|
||
| // TODO: Add support for `injectElement()` for full HTML element injection, not just scripts. |
There was a problem hiding this comment.
Shouldn't something like that be part of rendering and not part of the build? Otherwise it won't happen in SSR.
There was a problem hiding this comment.
I'm not sure I know what you mean, any links to a better location in the codebase for this TODO would be appreciated!
There was a problem hiding this comment.
Agreed! This is part of my concern with a single hook for "build" without a "compile" vs "render" discrepancy. I guess we're assuming that "build" implies static build.
There was a problem hiding this comment.
I'll move this TODO somewhere else that doesn't imply it will be handled by static build. And yes, I'm +1 on that. Would love to chat afterwards about the best path forward to implement with SSR in mind.
| @@ -69,10 +75,19 @@ export async function render(renderers: Renderer[], mod: ComponentInstance, ssrO | |||
| children: '', | |||
| }); | |||
| scripts.add({ | |||
| props: { type: 'module', src: new URL('../../../runtime/client/hmr.js', import.meta.url).pathname }, | |||
| props: { type: 'module', src: '/@id/astro/client/hmr.js' }, | |||
There was a problem hiding this comment.
Wish I knew about this magic /@id sooner.
There was a problem hiding this comment.
Vite always keeps you guessing :)
There was a problem hiding this comment.
THIS WORKS WHAT THIS IS GAME CHANGING
| // We have a need to pass context based on configured state, | ||
| // that is different from the user-exposed configuration. | ||
| // TODO: Create an AstroConfig class to manage this, long-term. | ||
| _ctx: { |
There was a problem hiding this comment.
I don't know if I will die on this hill (I might) but I think mixing configuration and state is a mistake that will be hard to unravel. Probably the right thing to do is create a class/object that wraps the config and then pass that everywhere (I believe that is what ResolvedConfig is for in Vite).
But that would involve updating a crazy amount of code, so I understand not wanting to do that.
How about a compromise where this context object is kept in a global WeakMap?
interface Context { ... }
const contexts = new WeakMap<AstroConfig, Context>();
const getContext = (config: AstroConfig) => contexts.get(config);
There was a problem hiding this comment.
I agree with this. Not sure the best solution but I don't love shoving a stateful object in here.
There was a problem hiding this comment.
Yea, I tried to leave the TODO to capture that need here. I agree, a context object isn't really a property of your config. If anything, it's the opposite, so maybe AstroContext becomes the class that manages your config and other stuff.
If we're talking whats in scope for this PR and the short-term, I think I prefer the _ctx hack vs. maintaining a weakmap separately. It's not a hill that I'll die on either, but I don't see that as any easier/harder to unravel than _ctx.
The compromise that I'd suggest instead would be to fast-track that larger Class/Object refactor instead of letting this sit for to long post-merge. If we could commit to that (maybe @bholmesdev could take a look next week?) then I think _ctx is the better short-term path.
|
GitHub is being weird and stopped letting me add comments inside of code. Aside from the comments I left the biggest thing I was unsure about from reading this is how the scripts end up in the page. It looks like Lastly, I really liked the idea of splitting the PR into more digestable chunks, that helped a lot with reviewing. |
packages/astro/src/core/config.ts
Outdated
| @@ -36,7 +71,19 @@ export const AstroConfigSchema = z.object({ | |||
| .optional() | |||
| .default('./dist') | |||
| .transform((val) => new URL(val)), | |||
| renderers: z.array(z.string()).optional().default(['@astrojs/renderer-svelte', '@astrojs/renderer-vue', '@astrojs/renderer-react', '@astrojs/renderer-preact']), | |||
| integrations: z.array(z.any()).default([]), | |||
There was a problem hiding this comment.
Glad to see this defaults to none!
There was a problem hiding this comment.
Yea... are we okay with this? It makes it a more disruptive change if so. I might ping you all on Discord to confirm that this is what we want, since we don't need to make this breaking change now.
But like all things, maybe best to just rip the bandaid off now
There was a problem hiding this comment.
Oh wow, I had no clue it defaulted to all frameworks previously. I definitely think we'll move renderers out of the core and into separate packages down the road. The requirement to install every supported UI framework makes npm init astro + plain npm (no yarn or pnpm) a pain to use. As a stepping stone to this, I'd say we should default to none for 1.0.
Thanks, it took some time so that means a lot! I'll usually try to break something this big into entirely standalone smaller PRs. But in this case I just couldn't find a good way to do it, given how interconnected it all is. |
FredKSchott
force-pushed
the
new-integrations-system
branch
from
1b550c2
to
090b2c4
Compare
github-actions
bot
added
pkg: create-astro
FredKSchott
force-pushed
the
new-integrations-system
branch
2 times, most recently
from
7a710fb
to
9246346
Compare
FredKSchott
force-pushed
the
new-integrations-system
branch
8 times, most recently
from
74aaa1f
to
aa961a5
Compare
FredKSchott
force-pushed
the
new-integrations-system
branch
2 times, most recently
from
7be3e88
to
7c2edbb
Compare
|
|
||
| let SUFFIX = ''; | ||
| // Add HMR handling in dev mode. | ||
| if (!resolvedConfig.isProduction) { |
There was a problem hiding this comment.
Why is this needed? Pretty sure we are already handling this elsewhere.
There was a problem hiding this comment.
Oh this, was already happening above, weird.
There was a problem hiding this comment.
I'm happy to remove but yea, would love to keep moving parts to a minimum in this PR
FredKSchott
force-pushed
the
new-integrations-system
branch
from
a4d5bab
to
f94af7a
Compare
| : // Otherwise, you are following an import in the module import tree. | ||
| // You are safe to use getModuleById() here because Vite has already | ||
| // resolved the correct `id` for you, by creating the import you followed here. | ||
| new Set([viteServer.moduleGraph.getModuleById(id)!]); |
There was a problem hiding this comment.
Huh, nice find! Glad we can default to the ID-based system where possible
There was a problem hiding this comment.
Doh, I meant to call this out to you! The reason for the change was that we needed to support virtual imports that contain CSS but don't exist on file, like the ones added by our Tailwind hooks.
I'd love your help confirming this change is good, it made sense to me and I tested it more than any other part of this PR probably.
* update examples * add initial integrations * update tests * update astro * update ci * get final tests working * update injectelement todo * update ben code review * respond to final code review feedback
* update examples * add initial integrations * update tests * update astro * update ci * get final tests working * update injectelement todo * update ben code review * respond to final code review feedback
Changes
How to Review
This is all interconnected to the point that I couldn't easily break the PR into smaller, individual, functional PRs. But, I was able to break down this PR into 3 different commits for a bit easier review:
Testing
Docs