-
-
Notifications
You must be signed in to change notification settings - Fork 2.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
(feat) return updated config in integration hooks #9013
Conversation
🦋 Changeset detectedLatest commit: 60fea6e The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Really cool! I know this is not the initial goal of this PR but it would great to type the newConfig argument ( |
@florian-lefebvre, good point. Added. |
@@ -374,6 +375,7 @@ export async function runHookBuildSetup({ | |||
target, | |||
updateConfig: (newConfig) => { | |||
updatedConfig = mergeConfig(updatedConfig, newConfig); | |||
return { ...updatedConfig }; |
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.
why is this a spread? Does it need to be cloned?
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's spread in the original declaration for updatedConfig
, so I'm just maintaining consistency. The object is pretty deep so this feels like it should either be a true clone or a read-only object, but those both seem a bit outside the scope of this issue.
Ref:
astro/packages/astro/src/integrations/index.ts
Lines 72 to 77 in 06c51cc
let updatedConfig: AstroConfig = { ...settings.config }; | |
let updatedSettings: AstroSettings = { ...settings, config: updatedConfig }; | |
let addedClientDirectives = new Map<string, Promise<string>>(); | |
let astroJSXRenderer: AstroRenderer | null = null; | |
We're going to do a final patch for Astro 3.x, then once that is done this will be merged in and be part of Astro 4.0. |
@@ -2311,7 +2311,7 @@ export interface AstroIntegration { | |||
config: AstroConfig; | |||
command: 'dev' | 'build' | 'preview'; | |||
isRestart: boolean; | |||
updateConfig: (newConfig: Record<string, any>) => void; | |||
updateConfig: (newConfig: DeepPartial<AstroConfig>) => AstroConfig; |
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 certainly is a lot better than before, but it could maybe give a false sense of safety. {}
is technically Partial<URL>
, 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.
It would be an edge case admittedly, and the runtime logic might actually handle it too. I'll take a closer look when I can, but I'm not blocking a merge.
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.
Will be merging this for Astro 4 soon.
Changes
Summary of change in this discussion thread: withastro/roadmap#762
astro:config:setup
hook for integrations, calls toupdateConfig
now return a destructured instance of the updated configuration.updateConfig
property to be strongly typed deep partial of theAstroConfig
type.Testing
astro/test/units/integrations/api.test.js
.AstroAdapter
in a local environment.Docs