From 89f2300d3dd9056d0bc4c619630002bd76381b96 Mon Sep 17 00:00:00 2001 From: Patrik Oldsberg Date: Wed, 18 Sep 2024 12:03:13 +0200 Subject: [PATCH] frontend-plugin-api: add ability to override params from .make Signed-off-by: Patrik Oldsberg --- .../src/wiring/createExtension.test.ts | 29 ++++ .../src/wiring/createExtension.ts | 134 ++++++++++-------- .../wiring/createExtensionBlueprint.test.tsx | 113 +++++++++++++++ .../src/wiring/createExtensionBlueprint.ts | 12 +- 4 files changed, 227 insertions(+), 61 deletions(-) diff --git a/packages/frontend-plugin-api/src/wiring/createExtension.test.ts b/packages/frontend-plugin-api/src/wiring/createExtension.test.ts index b5d846871f..e07647b644 100644 --- a/packages/frontend-plugin-api/src/wiring/createExtension.test.ts +++ b/packages/frontend-plugin-api/src/wiring/createExtension.test.ts @@ -744,6 +744,35 @@ describe('createExtension', () => { ); }); + it('should not have params unless explicitly defined', () => { + const ext = createExtension({ + attachTo: { id: 'root', input: 'blob' }, + output: [stringDataRef], + factory() { + return [stringDataRef('0')]; + }, + }); + + ext.override({ + // @ts-expect-error - params are not allowed + params: {} as any, + }); + + ext.override({ + // @ts-expect-error - params are not provided + factory(origFactory, { params }) { + return origFactory({ + // @ts-expect-error - params are not allowed + params: { + ...params, + }, + }); + }, + }); + + expect(ext).toBeDefined(); + }); + it('should be able to override input values', () => { const outputRef = createExtensionDataRef().with({ id: 'output', diff --git a/packages/frontend-plugin-api/src/wiring/createExtension.ts b/packages/frontend-plugin-api/src/wiring/createExtension.ts index ac7b1de432..97cb47a4fd 100644 --- a/packages/frontend-plugin-api/src/wiring/createExtension.ts +++ b/packages/frontend-plugin-api/src/wiring/createExtension.ts @@ -155,6 +155,7 @@ export type ExtensionDefinitionParameters = { { optional: boolean; singleton: boolean } >; }; + params?: object; }; /** @public */ @@ -177,42 +178,53 @@ export type ExtensionDefinition< >; }, >( - args: { - attachTo?: { id: string; input: string }; - disabled?: boolean; - inputs?: TExtraInputs & { - [KName in keyof T['inputs']]?: `Error: Input '${KName & - string}' is already defined in parent definition`; - }; - output?: Array; - config?: { - schema: TExtensionConfigSchema & { - [KName in keyof T['config']]?: `Error: Config key '${KName & - string}' is already defined in parent schema`; + args: Expand< + { + attachTo?: { id: string; input: string }; + disabled?: boolean; + inputs?: TExtraInputs & { + [KName in keyof T['inputs']]?: `Error: Input '${KName & + string}' is already defined in parent definition`; }; - }; - factory?( - originalFactory: (context?: { - config?: T['config']; - inputs?: ResolveInputValueOverrides>; - }) => ExtensionDataContainer>, - context: { - node: AppNode; - apis: ApiHolder; - config: T['config'] & { - [key in keyof TExtensionConfigSchema]: z.infer< - ReturnType - >; + output?: Array; + config?: { + schema: TExtensionConfigSchema & { + [KName in keyof T['config']]?: `Error: Config key '${KName & + string}' is already defined in parent schema`; }; - inputs: Expand>; - }, - ): Iterable; - } & VerifyExtensionFactoryOutput< - AnyExtensionDataRef extends UNewOutput - ? NonNullable - : UNewOutput, - UFactoryOutput - >, + }; + factory?( + originalFactory: ( + context?: Expand< + { + config?: T['config']; + inputs?: ResolveInputValueOverrides>; + } & ([T['params']] extends [never] + ? {} + : { params?: Partial }) + >, + ) => ExtensionDataContainer>, + context: { + node: AppNode; + apis: ApiHolder; + config: T['config'] & { + [key in keyof TExtensionConfigSchema]: z.infer< + ReturnType + >; + }; + inputs: Expand>; + }, + ): Iterable; + } & ([T['params']] extends [never] + ? {} + : { params?: Partial }) + > & + VerifyExtensionFactoryOutput< + AnyExtensionDataRef extends UNewOutput + ? NonNullable + : UNewOutput, + UFactoryOutput + >, ): ExtensionDefinition<{ kind: T['kind']; namespace: T['namespace']; @@ -274,6 +286,7 @@ export function createExtension< >; output: UOutput; inputs: TInputs; + params: never; kind: string | undefined extends TKind ? undefined : TKind; namespace: string | undefined extends TNamespace ? undefined : TNamespace; name: string | undefined extends TName ? undefined : TName; @@ -320,6 +333,7 @@ export function createExtension< >; output: UOutput; inputs: TInputs; + params: never; kind: string | undefined extends TKind ? undefined : TKind; namespace: string | undefined extends TNamespace ? undefined : TNamespace; name: string | undefined extends TName ? undefined : TName; @@ -362,6 +376,7 @@ export function createExtension< >; output: UOutput; inputs: TInputs; + params: object; kind: string | undefined extends TKind ? undefined : TKind; namespace: string | undefined extends TNamespace ? undefined : TNamespace; name: string | undefined extends TName ? undefined : TName; @@ -428,15 +443,6 @@ export function createExtension< 'Cannot override an extension that is not declared using the new format with outputs as an array', ); } - const newOptions = options as CreateExtensionOptions< - TKind, - TNamespace, - TName, - UOutput, - TInputs, - TConfigSchema, - UFactoryOutput - >; // TODO(Rugvip): Making this a type check would be optimal, but it seems // like it's tricky to add that and still have the type @@ -446,48 +452,60 @@ export function createExtension< 'Refused to override output without also overriding factory', ); } + // TODO(Rugvip): Similar to above, would be nice to error during type checking, but don't want to complicate the types too much + if (overrideOptions.params && overrideOptions.factory) { + throw new Error( + 'Refused to override params and factory at the same time', + ); + } return createExtension({ - kind: newOptions.kind, - namespace: newOptions.namespace, - name: newOptions.name, - attachTo: overrideOptions.attachTo ?? newOptions.attachTo, - disabled: overrideOptions.disabled ?? newOptions.disabled, - inputs: { ...overrideOptions.inputs, ...newOptions.inputs }, + kind: options.kind, + namespace: options.namespace, + name: options.name, + attachTo: overrideOptions.attachTo ?? options.attachTo, + disabled: overrideOptions.disabled ?? options.disabled, + inputs: { ...overrideOptions.inputs, ...options.inputs }, output: (overrideOptions.output ?? - newOptions.output) as AnyExtensionDataRef[], + options.output) as AnyExtensionDataRef[], config: - newOptions.config || overrideOptions.config + options.config || overrideOptions.config ? { schema: { - ...newOptions.config?.schema, + ...options.config?.schema, ...overrideOptions.config?.schema, }, } : undefined, factory: ({ node, apis, config, inputs }) => { if (!overrideOptions.factory) { - return newOptions.factory({ + return options.factory({ node, apis, config: config as any, inputs: inputs as any, - }); + params: overrideOptions.params, + // TODO(Rugvip): This is a bit of a hack to send the params + // through to the blueprint factory, might be that there's a + // better way to do this + } as Parameters[0]); } const parentResult = overrideOptions.factory( (innerContext): ExtensionDataContainer => { return createExtensionDataContainer( - newOptions.factory({ + options.factory({ node, apis, config: (innerContext?.config ?? config) as any, inputs: resolveInputOverrides( - newOptions.inputs, + options.inputs, inputs, innerContext?.inputs, ) as any, - }) as Iterable, - newOptions.output, + params: innerContext?.params, + // TODO(Rugvip): Same as above + } as Parameters[0]) as Iterable, + options.output, ); }, { diff --git a/packages/frontend-plugin-api/src/wiring/createExtensionBlueprint.test.tsx b/packages/frontend-plugin-api/src/wiring/createExtensionBlueprint.test.tsx index 16c763d725..79ccce31ff 100644 --- a/packages/frontend-plugin-api/src/wiring/createExtensionBlueprint.test.tsx +++ b/packages/frontend-plugin-api/src/wiring/createExtensionBlueprint.test.tsx @@ -836,4 +836,117 @@ describe('createExtensionBlueprint', () => { ), ).toEqual([testDataRef1('foo'), testDataRef2('bar')]); }); + + it('should be possible to override extensions resulting from .make', () => { + const testDataRef1 = createExtensionDataRef().with({ id: 'test1' }); + const testDataRef2 = createExtensionDataRef().with({ id: 'test2' }); + + function getOutputs(ext: ExtensionDefinition) { + const tester = createExtensionTester(ext); + return { + test1: tester.get(testDataRef1), + test2: tester.get(testDataRef2), + }; + } + + const blueprint = createExtensionBlueprint({ + kind: 'test-extension', + attachTo: { id: 'test', input: 'default' }, + output: [testDataRef1, testDataRef2], + *factory(params: { test1: string; test2: string }) { + yield testDataRef1(params.test1); + yield testDataRef2(params.test2); + }, + }); + + const extension = blueprint.make({ + params: { + test1: 'orig-1', + test2: 'orig-2', + }, + }); + + expect(getOutputs(extension)).toEqual({ + test1: 'orig-1', + test2: 'orig-2', + }); + + expect( + getOutputs( + extension.override({ + params: { + test1: 'override-1', + test2: 'override-2', + }, + }), + ), + ).toEqual({ + test1: 'override-1', + test2: 'override-2', + }); + + // Partial override + expect( + getOutputs( + extension.override({ + params: { + test2: 'override-2', + }, + }), + ), + ).toEqual({ + test1: 'orig-1', + test2: 'override-2', + }); + + expect( + getOutputs( + extension.override({ + factory(origFactory) { + return origFactory({ + params: { + test1: 'override-1', + test2: 'override-2', + }, + }); + }, + }), + ), + ).toEqual({ + test1: 'override-1', + test2: 'override-2', + }); + + // Partial override via factory + expect( + getOutputs( + extension.override({ + factory(origFactory) { + return origFactory({ + params: { + test2: 'override-2', + }, + }); + }, + }), + ), + ).toEqual({ + test1: 'orig-1', + test2: 'override-2', + }); + + expect(() => + getOutputs( + extension.override({ + params: { + test1: 'override-1', + test2: 'override-2', + }, + factory(origFactory) { + return origFactory(); + }, + }), + ), + ).toThrow('Refused to override params and factory at the same time'); + }); }); diff --git a/packages/frontend-plugin-api/src/wiring/createExtensionBlueprint.ts b/packages/frontend-plugin-api/src/wiring/createExtensionBlueprint.ts index 40d1284b15..fb9a4de830 100644 --- a/packages/frontend-plugin-api/src/wiring/createExtensionBlueprint.ts +++ b/packages/frontend-plugin-api/src/wiring/createExtensionBlueprint.ts @@ -124,6 +124,7 @@ export interface ExtensionBlueprint< configInput: T['configInput']; output: T['output']; inputs: T['inputs']; + params: T['params']; }>; /** @deprecated namespace is no longer required, you can safely remove this option and it will default to the `pluginId`. It will be removed in a future release. */ make< @@ -232,6 +233,7 @@ export interface ExtensionBlueprint< kind: T['kind']; namespace: undefined; name: string | undefined extends TNewName ? T['name'] : TNewName; + params: T['params']; }>; /** @deprecated namespace is no longer required, you can safely remove this option and it will default to the `pluginId`. It will be removed in a future release. */ makeWithOverrides< @@ -479,9 +481,13 @@ export function createExtensionBlueprint< output: options.output as AnyExtensionDataRef[], config: options.config, factory: ctx => - options.factory(args.params, ctx) as Iterable< - ExtensionDataValue - >, + options.factory( + // TODO(Rugvip): The `ctx` here might actually have a `params` key + // when the extension has been overridden. It's currently hidden in + // the types and there might be a better way to do this. + { ...args.params, ...(ctx as any).params }, + ctx, + ) as Iterable>, }) as ExtensionDefinition; }, makeWithOverrides(args) {