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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Nov 30, 2023
Merged

(feat) return updated config in integration hooks #9013

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
a25af0c
feat: return updated config in setup hook
nerdoza Nov 6, 2023
5eace19
test: add test for returned value
nerdoza Nov 6, 2023
d0e8837
chore: add changeset
nerdoza Nov 6, 2023
1e73b3b
fix: correct unit test
nerdoza Nov 6, 2023
fd9efee
fix: correct chai assertion
nerdoza Nov 6, 2023
d377267
feat: add strong typing to udpateConfig parameter
nerdoza Nov 7, 2023
abba7a3
fix: update solid package integration typing
nerdoza Nov 7, 2023
c2d425a
chore: rename RecursivePartial to DeepPartial
nerdoza Nov 7, 2023
089f034
chore: move DeepPartial type to internal util
nerdoza Nov 8, 2023
06c51cc
Merge remote-tracking branch 'upstream/main' into feat/update-config-…
nerdoza Nov 26, 2023
60fea6e
Update .changeset/wild-boats-wait.md
bluwy Nov 30, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/wild-boats-wait.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'astro': patch
---

Returns the updated config in the integration `astro:config:setup` hook's `updateConfig()` API
4 changes: 2 additions & 2 deletions packages/astro/src/@types/astro.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ import type { DevOverlayToggle } from '../runtime/client/dev-overlay/ui-library/
import type { DevOverlayTooltip } from '../runtime/client/dev-overlay/ui-library/tooltip.js';
import type { DevOverlayWindow } from '../runtime/client/dev-overlay/ui-library/window.js';
import type { AstroComponentFactory, AstroComponentInstance } from '../runtime/server/index.js';
import type { OmitIndexSignature, Simplify } from '../type-utils.js';
import type { DeepPartial, OmitIndexSignature, Simplify } from '../type-utils.js';
import type { SUPPORTED_MARKDOWN_FILE_EXTENSIONS } from './../core/constants.js';

export { type AstroIntegrationLogger };
Expand Down Expand Up @@ -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;
Copy link
Contributor

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.

Copy link
Contributor

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.

addRenderer: (renderer: AstroRenderer) => void;
addWatchFile: (path: URL | string) => void;
injectScript: (stage: InjectedScriptStage, content: string) => void;
Expand Down
2 changes: 2 additions & 0 deletions packages/astro/src/integrations/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,7 @@ export async function runHookConfigSetup({
},
updateConfig: (newConfig) => {
updatedConfig = mergeConfig(updatedConfig, newConfig) as AstroConfig;
return { ...updatedConfig };
},
injectRoute: (injectRoute) => {
updatedSettings.injectedRoutes.push(injectRoute);
Expand Down Expand Up @@ -374,6 +375,7 @@ export async function runHookBuildSetup({
target,
updateConfig: (newConfig) => {
updatedConfig = mergeConfig(updatedConfig, newConfig);
return { ...updatedConfig };
Copy link
Contributor

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?

Copy link
Contributor Author

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;

},
logger: getLogger(integration, logger),
}),
Expand Down
10 changes: 10 additions & 0 deletions packages/astro/src/type-utils.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,3 +30,13 @@ export type ValueOf<T> = T[keyof T];

// Gets the type of the values of a Map
export type MapValue<T> = T extends Map<any, infer V> ? V : never;

// Allow the user to create a type where all keys are optional.
// Useful for functions where props are merged.
export type DeepPartial<T> = {
[P in keyof T]?: T[P] extends (infer U)[]
? DeepPartial<U>[]
: T[P] extends object | undefined
? DeepPartial<T[P]>
: T[P];
};
27 changes: 27 additions & 0 deletions packages/astro/test/units/integrations/api.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,33 @@ describe('Integration API', () => {
expect(updatedViteConfig).to.haveOwnProperty('define');
});

it('runHookBuildSetup should return updated config', async () => {
let updatedInternalConfig;
const updatedViteConfig = await runHookBuildSetup({
config: {
integrations: [
{
name: 'test',
hooks: {
'astro:build:setup'({ updateConfig }) {
updatedInternalConfig = updateConfig({
define: {
foo: 'bar',
},
});
},
},
},
],
},
vite: {},
logger: defaultLogger,
pages: new Map(),
target: 'server',
});
expect(updatedViteConfig).to.be.deep.equal(updatedInternalConfig);
});

it('runHookConfigSetup can update Astro config', async () => {
const site = 'https://test.com/';
const updatedSettings = await runHookConfigSetup({
Expand Down
4 changes: 2 additions & 2 deletions packages/integrations/solid/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import type { AstroIntegration, AstroRenderer } from 'astro';
import type { AstroConfig, AstroIntegration, AstroRenderer } from 'astro';
import solid, { type Options as ViteSolidPluginOptions } from 'vite-plugin-solid';

async function getViteConfiguration(isDev: boolean, { include, exclude }: Options = {}) {
Expand Down Expand Up @@ -34,7 +34,7 @@ async function getViteConfiguration(isDev: boolean, { include, exclude }: Option
ssr: {
external: ['babel-preset-solid'],
},
};
} satisfies AstroConfig['vite'];
}

function getRenderer(): AstroRenderer {
Expand Down
Loading