diff --git a/core/cli/src/install.ts b/core/cli/src/install.ts index 8a77afb59..537001c57 100644 --- a/core/cli/src/install.ts +++ b/core/cli/src/install.ts @@ -31,7 +31,7 @@ async function asyncFilter(items: T[], predicate: (item: T) => Promise keep).map(({ item }) => item) } -const loadHookEntrypoints = async ( +export const loadHookEntrypoints = async ( logger: Logger, config: ValidConfig ): Promise>> => { @@ -54,13 +54,9 @@ export const loadHookInstallations = async ( config: ValidConfig ): Promise> => { const hookClassResults = await loadHookEntrypoints(logger, config) - const installationResults = ( - await hookClassResults - .map((hookClasses) => - reducePluginHookInstallations(logger, config, hookClasses, config.plugins['app root']) - ) - .awaitValue() - ).flatMap((installation) => installation) + const installationResults = hookClassResults.flatMap((hookClasses) => + reducePluginHookInstallations(logger, config, hookClasses, config.plugins['app root']) + ) const installationsWithoutConflicts = installationResults.flatMap((installations) => { const conflicts = findConflicts(installations) diff --git a/core/cli/src/plugin/reduce-installations.ts b/core/cli/src/plugin/reduce-installations.ts index aed78ff50..fcf46ef64 100644 --- a/core/cli/src/plugin/reduce-installations.ts +++ b/core/cli/src/plugin/reduce-installations.ts @@ -46,23 +46,19 @@ const extractForHook = (installation: HookInstallation | Conflict, plugin: Plugin -): Promise)[]>> { +): Validated<(HookInstallation | Conflict)[]> { if (!plugin.rcFile || config.resolutionTrackers.reducedInstallationPlugins.has(plugin.id)) { return valid([]) } config.resolutionTrackers.reducedInstallationPlugins.add(plugin.id) const rawChildInstallations = reduceValidated( - await Promise.all( - (plugin.children ?? []).map((child) => - reducePluginHookInstallations(logger, config, hookModules, child) - ) - ) + (plugin.children ?? []).map((child) => reducePluginHookInstallations(logger, config, hookModules, child)) ).map((installations) => installations.flat()) if (!rawChildInstallations.valid) { diff --git a/core/cli/test/plugin/files/single-install/merge-children/.toolkitrc.yml b/core/cli/test/plugin/files/single-install/merge-children/.toolkitrc.yml new file mode 100644 index 000000000..41bb74e56 --- /dev/null +++ b/core/cli/test/plugin/files/single-install/merge-children/.toolkitrc.yml @@ -0,0 +1,3 @@ +plugins: + - './a' + - './b' diff --git a/core/cli/test/plugin/files/single-install/merge-children/a/.toolkitrc.yml b/core/cli/test/plugin/files/single-install/merge-children/a/.toolkitrc.yml new file mode 100644 index 000000000..c004d8f62 --- /dev/null +++ b/core/cli/test/plugin/files/single-install/merge-children/a/.toolkitrc.yml @@ -0,0 +1,6 @@ +version: 2 + +options: + hooks: + - TestHook: + a: true diff --git a/core/cli/test/plugin/files/single-install/merge-children/b/.toolkitrc.yml b/core/cli/test/plugin/files/single-install/merge-children/b/.toolkitrc.yml new file mode 100644 index 000000000..7259da3d2 --- /dev/null +++ b/core/cli/test/plugin/files/single-install/merge-children/b/.toolkitrc.yml @@ -0,0 +1,6 @@ +version: 2 + +options: + hooks: + - TestHook: + b: true diff --git a/core/cli/test/plugin/files/single-install/nested-override/.toolkitrc.yml b/core/cli/test/plugin/files/single-install/nested-override/.toolkitrc.yml new file mode 100644 index 000000000..43c53afe7 --- /dev/null +++ b/core/cli/test/plugin/files/single-install/nested-override/.toolkitrc.yml @@ -0,0 +1,2 @@ +plugins: + - './b' diff --git a/core/cli/test/plugin/files/single-install/nested-override/a/.toolkitrc.yml b/core/cli/test/plugin/files/single-install/nested-override/a/.toolkitrc.yml new file mode 100644 index 000000000..c004d8f62 --- /dev/null +++ b/core/cli/test/plugin/files/single-install/nested-override/a/.toolkitrc.yml @@ -0,0 +1,6 @@ +version: 2 + +options: + hooks: + - TestHook: + a: true diff --git a/core/cli/test/plugin/files/single-install/nested-override/b/.toolkitrc.yml b/core/cli/test/plugin/files/single-install/nested-override/b/.toolkitrc.yml new file mode 100644 index 000000000..dff0a3a4c --- /dev/null +++ b/core/cli/test/plugin/files/single-install/nested-override/b/.toolkitrc.yml @@ -0,0 +1,9 @@ +version: 2 + +plugins: + - '../a' + +options: + hooks: + - TestHook: + b: true diff --git a/core/cli/test/plugin/files/single-install/override-children/.toolkitrc.yml b/core/cli/test/plugin/files/single-install/override-children/.toolkitrc.yml new file mode 100644 index 000000000..47d439f34 --- /dev/null +++ b/core/cli/test/plugin/files/single-install/override-children/.toolkitrc.yml @@ -0,0 +1,8 @@ +plugins: + - './a' + - './b' + +options: + hooks: + - TestHook: + c: true diff --git a/core/cli/test/plugin/files/single-install/override-children/a/.toolkitrc.yml b/core/cli/test/plugin/files/single-install/override-children/a/.toolkitrc.yml new file mode 100644 index 000000000..c004d8f62 --- /dev/null +++ b/core/cli/test/plugin/files/single-install/override-children/a/.toolkitrc.yml @@ -0,0 +1,6 @@ +version: 2 + +options: + hooks: + - TestHook: + a: true diff --git a/core/cli/test/plugin/files/single-install/override-children/b/.toolkitrc.yml b/core/cli/test/plugin/files/single-install/override-children/b/.toolkitrc.yml new file mode 100644 index 000000000..7259da3d2 --- /dev/null +++ b/core/cli/test/plugin/files/single-install/override-children/b/.toolkitrc.yml @@ -0,0 +1,6 @@ +version: 2 + +options: + hooks: + - TestHook: + b: true diff --git a/core/cli/test/plugin/reduce-installations.test.ts b/core/cli/test/plugin/reduce-installations.test.ts new file mode 100644 index 000000000..48f12ad7c --- /dev/null +++ b/core/cli/test/plugin/reduce-installations.test.ts @@ -0,0 +1,308 @@ +import path from 'path' +import { Hook, HookClass } from '@dotcom-tool-kit/base' +import { isConflict } from '@dotcom-tool-kit/conflict' +import { z } from 'zod' +import winston, { Logger } from 'winston' +import { loadConfig } from '../../src/config' +import { HookModule, reducePluginHookInstallations } from '../../src/plugin/reduce-installations' + +const logger = winston as unknown as Logger + +const testHookSchema = z.any() + +class TestHook extends Hook< + { + hook: typeof testHookSchema + }, + void +> { + static schema = z.any() + + async isInstalled() { + return true + } + + // eslint-disable-next-line @typescript-eslint/no-empty-function + async install() {} +} + +const hookClasses = { + TestHook: { + hookClass: TestHook as HookClass, + schema: testHookSchema + } +} satisfies Record + +describe('reducePluginHookInstallations', () => { + afterEach(() => { + jest.restoreAllMocks() + }) + + describe('single installation per plugin', () => { + it("should return conflict if mergeChildInstallations returns conflict that's not resolved by overrideChildInstallations", async () => { + const config = await loadConfig(logger, { + root: path.resolve(__dirname, './files/single-install/merge-children') + }) + + jest.spyOn(TestHook, 'mergeChildInstallations').mockImplementation( + // NOTE: this is the default implementation from the Hook base class, + // repeating it here in the mock for clarity and consistency with other tests + (plugin, childInstallations) => [ + { + plugin, + conflicting: childInstallations.flatMap((installation) => + isConflict(installation) ? installation.conflicting : installation + ) + } + ] + ) + + const mergedInstallations = reducePluginHookInstallations( + logger, + config, + hookClasses, + config.plugins['app root'] + ).unwrap('hook installations were invalid') + + expect(mergedInstallations).toMatchObject([ + { + plugin: config.plugins['app root'], + conflicting: [ + { + plugin: config.plugins['./a'], + options: { + a: true + } + }, + { + plugin: config.plugins['./b'], + options: { + b: true + } + } + ] + } + ]) + }) + + it('should return resolved installation returned by mergeChildInstallations', async () => { + const config = await loadConfig(logger, { + root: path.resolve(__dirname, './files/single-install/merge-children') + }) + + jest.spyOn(TestHook, 'mergeChildInstallations').mockImplementation((plugin, childInstallations) => [ + { + plugin, + forHook: 'TestHook', + hookConstructor: TestHook, + options: Object.fromEntries( + childInstallations.flatMap((child) => (isConflict(child) ? [] : Object.entries(child.options))) + ) + } + ]) + + const mergedInstallations = reducePluginHookInstallations( + logger, + config, + hookClasses, + config.plugins['app root'] + ).unwrap('hook installations were invalid') + + expect(mergedInstallations).toMatchObject([ + { + plugin: config.plugins['app root'], + options: { + a: true, + b: true + } + } + ]) + }) + + it('should return resolved installation returned by overrideChildInstallations', async () => { + const config = await loadConfig(logger, { + root: path.resolve(__dirname, './files/single-install/override-children') + }) + + jest.spyOn(TestHook, 'overrideChildInstallations').mockImplementation( + // NOTE: this is the default implementation from the Hook base class, + // repeating it here in the mock for clarity and consistency with other tests + (plugin, parentInstallation, _childInstallations) => [parentInstallation] + ) + + const mergedInstallations = reducePluginHookInstallations( + logger, + config, + hookClasses, + config.plugins['app root'] + ).unwrap('hook installations were invalid') + + expect(mergedInstallations).toMatchObject([ + { + plugin: config.plugins['app root'], + options: { + c: true + } + } + ]) + }) + + it('should return resolved installation merged by overrideChildInstallations', async () => { + const config = await loadConfig(logger, { + root: path.resolve(__dirname, './files/single-install/override-children') + }) + + jest.spyOn(TestHook, 'overrideChildInstallations').mockImplementation( + // NOTE: this implementation naively merges installations between parents and children. + // that's not a usual use case, but CircleCi and PackageJson both do this in limited cases. + (plugin, parentInstallation, childInstallations) => [ + { + ...parentInstallation, + options: Object.fromEntries( + [parentInstallation, ...childInstallations].flatMap((installation) => + isConflict(installation) + ? installation.conflicting.flatMap((child) => Object.entries(child.options)) + : Object.entries(installation.options) + ) + ), + plugin + } + ] + ) + + const mergedInstallations = reducePluginHookInstallations( + logger, + config, + hookClasses, + config.plugins['app root'] + ).unwrap('hook installations were invalid') + + expect(mergedInstallations).toMatchObject([ + { + plugin: config.plugins['app root'], + options: { + a: true, + b: true + } + } + ]) + }) + + it('should return conflict returned by overrideChildInstallations', async () => { + const config = await loadConfig(logger, { + root: path.resolve(__dirname, './files/single-install/override-children') + }) + + jest.spyOn(TestHook, 'overrideChildInstallations').mockImplementation( + // NOTE: check which plugin is overriding; only return a conflict at the app root stage + (plugin, parentInstallation, childInstallations) => + plugin.id === 'app root' + ? [ + { + plugin, + conflicting: childInstallations.flatMap((installation) => + isConflict(installation) ? installation.conflicting : installation + ) + } + ] + : Hook.overrideChildInstallations(plugin, parentInstallation, childInstallations) + ) + + const mergedInstallations = reducePluginHookInstallations( + logger, + config, + hookClasses, + config.plugins['app root'] + ).unwrap('hook installations were invalid') + + expect(mergedInstallations).toMatchObject([ + { + plugin: config.plugins['app root'], + conflicting: [ + { + plugin: config.plugins['./a'], + options: { + a: true + } + }, + { + plugin: config.plugins['./b'], + options: { + b: true + } + } + ] + } + ]) + }) + + it('should return resolved installation returned by overrideChildInstallations at multiple layers of nesting', async () => { + const config = await loadConfig(logger, { + root: path.resolve(__dirname, './files/single-install/nested-override') + }) + + jest.spyOn(TestHook, 'overrideChildInstallations').mockImplementation( + // NOTE: this is the default implementation from the Hook base class, + // repeating it here in the mock for clarity and consistency with other tests + (plugin, parentInstallation, _childInstallations) => [parentInstallation] + ) + + const mergedInstallations = reducePluginHookInstallations( + logger, + config, + hookClasses, + config.plugins['app root'] + ).unwrap('hook installations were invalid') + + expect(mergedInstallations).toMatchObject([ + { + plugin: config.plugins['./b'], + options: { + b: true + } + } + ]) + }) + + it('should return resolved installation merged by overrideChildInstallations at multiple layers of nesting', async () => { + const config = await loadConfig(logger, { + root: path.resolve(__dirname, './files/single-install/nested-override') + }) + + jest.spyOn(TestHook, 'overrideChildInstallations').mockImplementation( + // NOTE: this implementation naively merges installations between parents and children. + // that's not a usual use case, but CircleCi and PackageJson both do this in limited cases. + (plugin, parentInstallation, childInstallations) => [ + { + ...parentInstallation, + options: Object.fromEntries( + [parentInstallation, ...childInstallations].flatMap((installation) => + isConflict(installation) + ? installation.conflicting.flatMap((child) => Object.entries(child.options)) + : Object.entries(installation.options) + ) + ), + plugin + } + ] + ) + + const mergedInstallations = reducePluginHookInstallations( + logger, + config, + hookClasses, + config.plugins['app root'] + ).unwrap('hook installations were invalid') + + expect(mergedInstallations).toMatchObject([ + { + plugin: config.plugins['./b'], + options: { + a: true, + b: true + } + } + ]) + }) + }) +}) diff --git a/lib/base/src/hook.ts b/lib/base/src/hook.ts index 2f568e70e..f8bf0d3a1 100644 --- a/lib/base/src/hook.ts +++ b/lib/base/src/hook.ts @@ -39,6 +39,10 @@ export abstract class Hook< plugin: Plugin, childInstallations: (HookInstallation | Conflict)[] ): (HookInstallation | Conflict)[] { + if (childInstallations.length === 1) { + return childInstallations + } + return [ { plugin,