From c4b377a4b53bb82cfac7c2a05e0f0e1f9fbfacab Mon Sep 17 00:00:00 2001 From: Lukas Taegert-Atkinson Date: Sat, 6 Aug 2022 07:04:27 +0200 Subject: [PATCH] Allow object form for addon hooks --- src/rollup/rollup.ts | 7 +- src/rollup/types.d.ts | 4 +- src/utils/PluginDriver.ts | 106 +++++++++--------- src/utils/error.ts | 18 +++ .../samples/invalid-addon-hook/_config.js | 13 +++ .../samples/invalid-addon-hook/foo.js | 1 + .../samples/invalid-addon-hook/main.js | 1 + .../non-function-hook-async/_config.js | 8 +- test/hooks/index.js | 4 +- 9 files changed, 101 insertions(+), 61 deletions(-) create mode 100644 test/function/samples/invalid-addon-hook/_config.js create mode 100644 test/function/samples/invalid-addon-hook/foo.js create mode 100644 test/function/samples/invalid-addon-hook/main.js diff --git a/src/rollup/rollup.ts b/src/rollup/rollup.ts index c4f99b695b6..f34d06ad11b 100644 --- a/src/rollup/rollup.ts +++ b/src/rollup/rollup.ts @@ -1,7 +1,7 @@ import { version as rollupVersion } from 'package.json'; import Bundle from '../Bundle'; import Graph from '../Graph'; -import { getSortedPlugins } from '../utils/PluginDriver'; +import { getSortedValidatedPlugins } from '../utils/PluginDriver'; import type { PluginDriver } from '../utils/PluginDriver'; import { ensureArray } from '../utils/ensureArray'; import { errAlreadyClosed, errCannotEmitFromOptionsHook, error } from '../utils/error'; @@ -113,7 +113,10 @@ async function getInputOptions( if (!rawInputOptions) { throw new Error('You must supply an options object to rollup'); } - const rawPlugins = getSortedPlugins('options', ensureArray(rawInputOptions.plugins) as Plugin[]); + const rawPlugins = getSortedValidatedPlugins( + 'options', + ensureArray(rawInputOptions.plugins) as Plugin[] + ); const { options, unsetOptions } = normalizeInputOptions( await rawPlugins.reduce(applyOptionHook(watchMode), Promise.resolve(rawInputOptions)) ); diff --git a/src/rollup/types.d.ts b/src/rollup/types.d.ts index 8ae2e6a0511..baae42b5daa 100644 --- a/src/rollup/types.d.ts +++ b/src/rollup/types.d.ts @@ -468,9 +468,7 @@ type MakeAsync any> = ( ...a: Parameters ) => ReturnType | Promise>; -type AllowEnforceOrderHook any> = - | T - | { handler: T; order?: 'pre' | 'post' | null }; +type AllowEnforceOrderHook = T | { handler: T; order?: 'pre' | 'post' | null }; export type PluginHooks = { [K in keyof FunctionPluginHooks]: K extends AsyncPluginHooks diff --git a/src/utils/PluginDriver.ts b/src/utils/PluginDriver.ts index a99e7528fca..a0f7890d7cc 100644 --- a/src/utils/PluginDriver.ts +++ b/src/utils/PluginDriver.ts @@ -1,7 +1,6 @@ import type Chunk from '../Chunk'; import type Graph from '../Graph'; import type Module from '../Module'; -import { InputPluginHooks } from '../rollup/types'; import type { AddonHookFunction, AddonHooks, @@ -19,22 +18,23 @@ import type { SerializablePluginCache, SyncPluginHooks } from '../rollup/types'; +import { InputPluginHooks } from '../rollup/types'; import { FileEmitter } from './FileEmitter'; import { getPluginContext } from './PluginContext'; -import { errInputHookInOutputPlugin, error } from './error'; +import { + errInputHookInOutputPlugin, + errInvalidAddonPluginHook, + errInvalidAsyncPluginHook, + error +} from './error'; import { getOrCreate } from './getOrCreate'; import { throwPluginError, warnDeprecatedHooks } from './pluginUtils'; -/** - * Get the inner type from a promise - * @example ResolveValue> -> string - */ -type ResolveValue = T extends Promise ? K : T; /** * Coerce a promise union to always be a promise. * @example EnsurePromise> -> Promise */ -type EnsurePromise = Promise>; +type EnsurePromise = Promise>; /** * Get the type of the first argument in a function. * @example Arg0<(a: string, b: number) => void> -> string @@ -139,13 +139,13 @@ export class PluginDriver { args: Parameters, replaceContext?: ReplaceContext | null, skipped?: ReadonlySet | null - ): Promise> { - let promise: Promise> = Promise.resolve(undefined as any); + ): Promise | null> { + let promise: Promise | null> = Promise.resolve(null); for (const plugin of this.getSortedPlugins(hookName)) { if (skipped && skipped.has(plugin)) continue; promise = promise.then(result => { if (result != null) return result; - return this.runHook(hookName, args, plugin, false, replaceContext); + return this.runHook(hookName, args, plugin, replaceContext); }); } return promise; @@ -172,7 +172,7 @@ export class PluginDriver { ): Promise { const promises: Promise[] = []; for (const plugin of this.getSortedPlugins(hookName)) { - const hookPromise = this.runHook(hookName, args, plugin, false, replaceContext); + const hookPromise = this.runHook(hookName, args, plugin, replaceContext); if (!hookPromise) continue; promises.push(hookPromise); } @@ -194,7 +194,7 @@ export class PluginDriver { for (const plugin of this.getSortedPlugins(hookName)) { promise = promise.then(arg0 => { const args = [arg0, ...rest] as Parameters; - const hookPromise = this.runHook(hookName, args, plugin, false, replaceContext); + const hookPromise = this.runHook(hookName, args, plugin, replaceContext); if (!hookPromise) return arg0; return hookPromise.then(result => reduce.call(this.pluginContexts.get(plugin), arg0, result, plugin) @@ -228,17 +228,19 @@ export class PluginDriver { hookName: H, initialValue: T | Promise, args: Parameters, - reduce: ( - reduction: T, - result: ResolveValue>, - plugin: Plugin - ) => T, - replaceContext?: ReplaceContext + reduce: (reduction: T, result: Awaited>, plugin: Plugin) => T ): Promise { let promise = Promise.resolve(initialValue); - for (const plugin of this.getSortedPlugins(hookName)) { + for (const plugin of this.getSortedPlugins( + hookName, + (handler: unknown, hookName: string, plugin: Plugin) => { + if (typeof handler !== 'string' && typeof handler !== 'function') { + return error(errInvalidAddonPluginHook(hookName, plugin.name)); + } + } + )) { promise = promise.then(value => { - const hookPromise = this.runHook(hookName, args, plugin, true, replaceContext); + const hookPromise = this.runHook(hookName, args, plugin); if (!hookPromise) return value; return hookPromise.then(result => reduce.call(this.pluginContexts.get(plugin), value, result, plugin) @@ -273,15 +275,18 @@ export class PluginDriver { let promise = Promise.resolve(); for (const plugin of this.getSortedPlugins(hookName)) { promise = promise.then( - () => this.runHook(hookName, args, plugin, false, replaceContext) as Promise + () => this.runHook(hookName, args, plugin, replaceContext) as Promise ); } return promise; } - private getSortedPlugins(hookName: AsyncPluginHooks | AddonHooks): Plugin[] { + private getSortedPlugins( + hookName: AsyncPluginHooks | AddonHooks, + validateHandler?: (handler: unknown, hookName: string, plugin: Plugin) => void + ): Plugin[] { return getOrCreate(this.sortedPlugins, hookName, () => - getSortedPlugins(hookName, this.plugins) + getSortedValidatedPlugins(hookName, this.plugins, validateHandler) ); } @@ -290,47 +295,41 @@ export class PluginDriver { * @param hookName Name of the plugin hook. Must be either in `PluginHooks` or `OutputPluginValueHooks`. * @param args Arguments passed to the plugin hook. * @param plugin The actual pluginObject to run. - * @param permitValues If true, values can be passed instead of functions for the plugin hook. - * @param hookContext When passed, the plugin context can be overridden. + * @param replaceContext When passed, the plugin context can be overridden. */ private runHook( hookName: H, args: Parameters, - plugin: Plugin, - permitValues: true, - hookContext?: ReplaceContext | null + plugin: Plugin ): EnsurePromise>; private runHook( hookName: H, args: Parameters, plugin: Plugin, - permitValues: false, - hookContext?: ReplaceContext | null + replaceContext?: ReplaceContext | null ): Promise>; - private runHook( + // Implementation signature + private runHook( hookName: H, - args: Parameters, + args: unknown[], plugin: Plugin, - permitValues: boolean, - hookContext?: ReplaceContext | null - ): Promise> { + replaceContext?: ReplaceContext | null + ): Promise | undefined { const hook = plugin[hookName]; - if (!hook) return undefined as any; + // TODO Lukas may no longer be needed if we always pre-filter + if (!hook) return undefined; const handler = typeof hook === 'object' ? hook.handler : hook; let context = this.pluginContexts.get(plugin)!; - if (hookContext) { - context = hookContext(context, plugin); + if (replaceContext) { + context = replaceContext(context, plugin); } let action: [string, string, Parameters] | null = null; return Promise.resolve() .then(() => { - // TODO Lukas move validation to sort function - // permit values allows values to be returned instead of a functional hook if (typeof handler !== 'function') { - if (permitValues) return handler; - return throwInvalidHookError(hookName, plugin.name); + return handler; } // eslint-disable-next-line @typescript-eslint/ban-types const hookResult = (handler as Function).apply(context, args); @@ -371,20 +370,20 @@ export class PluginDriver { * @param hookName Name of the plugin hook. Must be in `PluginHooks`. * @param args Arguments passed to the plugin hook. * @param plugin The acutal plugin - * @param hookContext When passed, the plugin context can be overridden. + * @param replaceContext When passed, the plugin context can be overridden. */ private runHookSync( hookName: H, args: Parameters, plugin: Plugin, - hookContext?: ReplaceContext + replaceContext?: ReplaceContext ): ReturnType { const hook = plugin[hookName]; if (!hook) return undefined as any; let context = this.pluginContexts.get(plugin)!; - if (hookContext) { - context = hookContext(context, plugin); + if (replaceContext) { + context = replaceContext(context, plugin); } try { @@ -400,10 +399,14 @@ export class PluginDriver { } } -// TODO Lukas we can do plugin hook validation in this function -export function getSortedPlugins( +export function getSortedValidatedPlugins( hookName: AsyncPluginHooks | AddonHooks, - plugins: readonly Plugin[] + plugins: readonly Plugin[], + validateHandler = (handler: unknown, hookName: string, plugin: Plugin) => { + if (typeof handler !== 'function') { + error(errInvalidAsyncPluginHook(hookName, plugin.name)); + } + } ): Plugin[] { const pre: Plugin[] = []; const normal: Plugin[] = []; @@ -412,6 +415,7 @@ export function getSortedPlugins( const hook = plugin[hookName]; if (hook) { if (typeof hook === 'object') { + validateHandler(hook.handler, hookName, plugin); if (hook.order === 'pre') { pre.push(plugin); continue; @@ -420,6 +424,8 @@ export function getSortedPlugins( post.push(plugin); continue; } + } else { + validateHandler(hook, hookName, plugin); } normal.push(plugin); } diff --git a/src/utils/error.ts b/src/utils/error.ts index f17ea17f0ab..8f9116d9da9 100644 --- a/src/utils/error.ts +++ b/src/utils/error.ts @@ -255,6 +255,24 @@ export function errInvalidOption( }; } +export function errInvalidAddonPluginHook(hook: string, plugin: string): RollupLogProps { + return { + code: Errors.INVALID_PLUGIN_HOOK, + hook, + message: `Error running plugin hook ${hook} for plugin ${plugin}, expected a string, a function hook or an object with a "handler" string or function.`, + plugin + }; +} + +export function errInvalidAsyncPluginHook(hook: string, plugin: string): RollupLogProps { + return { + code: Errors.INVALID_PLUGIN_HOOK, + hook, + message: `Error running plugin hook ${hook} for plugin ${plugin}, expected a function hook or an object with a "handler" function.`, + plugin + }; +} + export function errInvalidRollupPhaseForAddWatchFile(): RollupLogProps { return { code: Errors.INVALID_ROLLUP_PHASE, diff --git a/test/function/samples/invalid-addon-hook/_config.js b/test/function/samples/invalid-addon-hook/_config.js new file mode 100644 index 00000000000..95617db72ea --- /dev/null +++ b/test/function/samples/invalid-addon-hook/_config.js @@ -0,0 +1,13 @@ +module.exports = { + description: 'throws when providing a non-string value for an addon hook', + options: { + plugins: { + intro: 42 + } + }, + generateError: { + code: 'ADDON_ERROR', + message: + 'Could not retrieve intro. Check configuration of plugin at position 1.\n\tError Message: Error running plugin hook intro for plugin at position 1, expected a string, a function hook or an object with a "handler" string or function.' + } +}; diff --git a/test/function/samples/invalid-addon-hook/foo.js b/test/function/samples/invalid-addon-hook/foo.js new file mode 100644 index 00000000000..7a4e8a723a4 --- /dev/null +++ b/test/function/samples/invalid-addon-hook/foo.js @@ -0,0 +1 @@ +export default 42; diff --git a/test/function/samples/invalid-addon-hook/main.js b/test/function/samples/invalid-addon-hook/main.js new file mode 100644 index 00000000000..a25cfbd9058 --- /dev/null +++ b/test/function/samples/invalid-addon-hook/main.js @@ -0,0 +1 @@ +export default () => import('./foo.js'); diff --git a/test/function/samples/non-function-hook-async/_config.js b/test/function/samples/non-function-hook-async/_config.js index d17469d3926..bfba8d840ea 100644 --- a/test/function/samples/non-function-hook-async/_config.js +++ b/test/function/samples/non-function-hook-async/_config.js @@ -6,10 +6,10 @@ module.exports = { } }, error: { - code: 'PLUGIN_ERROR', + code: 'INVALID_PLUGIN_HOOK', hook: 'resolveId', - message: 'Error running plugin hook resolveId for at position 1, expected a function hook.', - plugin: 'at position 1', - pluginCode: 'INVALID_PLUGIN_HOOK' + message: + 'Error running plugin hook resolveId for plugin at position 1, expected a function hook or an object with a "handler" function.', + plugin: 'at position 1' } }; diff --git a/test/hooks/index.js b/test/hooks/index.js index 7ae444216b0..e0c9e0fc874 100644 --- a/test/hooks/index.js +++ b/test/hooks/index.js @@ -30,8 +30,8 @@ describe('hooks', () => { format: 'es' }); const fileNames = (await readdir(TEMP_DIR)).sort(); - assert.deepStrictEqual(fileNames, ['chunk.js', 'input.js']); await remove(TEMP_DIR); + assert.deepStrictEqual(fileNames, ['chunk.js', 'input.js']); }); it('supports buildStart and buildEnd hooks', () => { @@ -567,8 +567,8 @@ describe('hooks', () => { ] }); await bundle.write({ format: 'es', file }); - assert.strictEqual(callCount, 1); await remove(TEMP_DIR); + assert.strictEqual(callCount, 1); }); it('supports this.cache for plugins', () =>