-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
feat: buildApp hook #19971
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: buildApp hook #19971
Changes from all commits
0d00051
0e3196a
1caad17
9be0ade
fa6912d
dcea0b1
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 |
|---|---|---|
|
|
@@ -1494,6 +1494,7 @@ function areSeparateFolders(a: string, b: string) { | |
| export class BuildEnvironment extends BaseEnvironment { | ||
| mode = 'build' as const | ||
|
|
||
| isBuilt = false | ||
| constructor( | ||
| name: string, | ||
| config: ResolvedConfig, | ||
|
|
@@ -1548,12 +1549,6 @@ export interface BuilderOptions { | |
| buildApp?: (builder: ViteBuilder) => Promise<void> | ||
| } | ||
|
|
||
| async function defaultBuildApp(builder: ViteBuilder): Promise<void> { | ||
| for (const environment of Object.values(builder.environments)) { | ||
| await builder.build(environment) | ||
| } | ||
| } | ||
|
|
||
| export const builderOptionsDefaults = Object.freeze({ | ||
| sharedConfigBuild: false, | ||
| sharedPlugins: false, | ||
|
|
@@ -1565,7 +1560,7 @@ export function resolveBuilderOptions( | |
| ): ResolvedBuilderOptions | undefined { | ||
| if (!options) return | ||
| return mergeWithDefaults( | ||
| { ...builderOptionsDefaults, buildApp: defaultBuildApp }, | ||
| { ...builderOptionsDefaults, buildApp: async () => {} }, | ||
| options, | ||
| ) | ||
| } | ||
|
|
@@ -1602,10 +1597,41 @@ export async function createBuilder( | |
| environments, | ||
| config, | ||
| async buildApp() { | ||
| return configBuilder.buildApp(builder) | ||
| // order 'pre' and 'normal' hooks are run first, then config.builder.buildApp, then 'post' hooks | ||
| let configBuilderBuildAppCalled = false | ||
| for (const p of config.getSortedPlugins('buildApp')) { | ||
| const hook = p.buildApp | ||
| if ( | ||
| !configBuilderBuildAppCalled && | ||
| typeof hook === 'object' && | ||
| hook.order === 'post' | ||
| ) { | ||
| configBuilderBuildAppCalled = true | ||
| await configBuilder.buildApp(builder) | ||
| } | ||
| const handler = getHookHandler(hook) | ||
| await handler(builder) | ||
| } | ||
| if (!configBuilderBuildAppCalled) { | ||
| await configBuilder.buildApp(builder) | ||
| } | ||
| // fallback to building all environments if no environments have been built | ||
| if ( | ||
| Object.values(builder.environments).every( | ||
| (environment) => !environment.isBuilt, | ||
| ) | ||
| ) { | ||
| for (const environment of Object.values(builder.environments)) { | ||
| await builder.build(environment) | ||
| } | ||
| } | ||
|
Comment on lines
+1618
to
+1627
Contributor
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. Putting // always build rest of unbuilt environments
for (const environment of Object.values(builder.environments)) {
if (!environment.isBuilt) {
await builder.build(environment)
}
}I don't see anything significant right now, so either way seems fine to start with, but I just wanted to check if I'm missing something.
Contributor
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. Well, actually this seems like it's also about
Member
Author
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. The idea of the post hook was to give a way to plugins to build environments only if no other plugin has built them. See the problem described in the linked discussion. And they also want to make sure to do build related tasks after all environments have been built. Once But to the point in your other thread, I see now that it isn't only about order and the We could have an
Contributor
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.
Oh right, I remembered that's the whole point of providing |
||
| }, | ||
| async build(environment: BuildEnvironment) { | ||
| return buildEnvironment(environment) | ||
| async build( | ||
| environment: BuildEnvironment, | ||
| ): Promise<RollupOutput | RollupOutput[] | RollupWatcher> { | ||
| const output = await buildEnvironment(environment) | ||
| environment.isBuilt = true | ||
| return output | ||
| }, | ||
| } | ||
|
|
||
|
|
@@ -1667,3 +1693,5 @@ export async function createBuilder( | |
|
|
||
| return builder | ||
| } | ||
|
|
||
| export type BuildAppHook = (this: void, builder: ViteBuilder) => Promise<void> | ||
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.
Instead of dcea0b1, can the default behavior adjusted by checking
environment.isBuiltfor each environment here?I think the difference is whether
plugin.buildApppartially building environments should cause Vite to build the rest of environments. As a default behavior, this seemed natural to me.plugin.buildAppcan still technically avoid that by doingdelete builder.environments["noNeedThisEnv"]🤔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.
We can't do that except if we move the order post hooks before config.builder.buildApp, and we also may deprecate and remove config.builder.buildApp in the future