From 788e4a7f19b366e49cc1d7c167041124351bc60a Mon Sep 17 00:00:00 2001 From: Levente Balogh Date: Mon, 20 Mar 2023 10:41:15 +0100 Subject: [PATCH] Extensions: Simplify the `configure()` function (#64958) refactor: stop passing the extension to the `configure()` function --- packages/grafana-data/src/types/app.ts | 4 +-- .../services/pluginExtensions/extensions.ts | 3 ++ .../plugins/extensions/errorHandling.test.ts | 17 ++++------- .../plugins/extensions/errorHandling.ts | 4 +-- .../extensions/registryFactory.test.ts | 28 +++++-------------- .../plugins/extensions/registryFactory.ts | 26 ++++------------- .../app/features/plugins/extensions/types.ts | 2 +- .../plugins/extensions/validateLink.test.ts | 15 +++------- .../plugins/extensions/validateLink.ts | 4 +-- 9 files changed, 33 insertions(+), 70 deletions(-) diff --git a/packages/grafana-data/src/types/app.ts b/packages/grafana-data/src/types/app.ts index a37a9c85fb3..fc1373530c6 100644 --- a/packages/grafana-data/src/types/app.ts +++ b/packages/grafana-data/src/types/app.ts @@ -74,7 +74,7 @@ export type AppPluginExtensionLinkConfig = { description: string; placement: string; path: string; - configure?: (extension: AppPluginExtensionLink, context?: C) => Partial | undefined; + configure?: (context?: C) => Partial | undefined; }; export type AppPluginExtensionCommandConfig = { @@ -82,7 +82,7 @@ export type AppPluginExtensionCommandConfig = { description: string; placement: string; handler: (context?: C, helpers?: AppPluginExtensionCommandHelpers) => void; - configure?: (extension: AppPluginExtensionCommand, context?: C) => Partial | undefined; + configure?: (context?: C) => Partial | undefined; }; export class AppPlugin extends GrafanaPlugin> { diff --git a/packages/grafana-runtime/src/services/pluginExtensions/extensions.ts b/packages/grafana-runtime/src/services/pluginExtensions/extensions.ts index a5dee9d514a..97678d7539b 100644 --- a/packages/grafana-runtime/src/services/pluginExtensions/extensions.ts +++ b/packages/grafana-runtime/src/services/pluginExtensions/extensions.ts @@ -20,9 +20,12 @@ export function getPluginExtensions( const extensions = configureFuncs.reduce((result, configure) => { const extension = configure(context); + + // If the configure() function returns `undefined`, the extension is not displayed if (extension) { result.push(extension); } + return result; }, []); diff --git a/public/app/features/plugins/extensions/errorHandling.test.ts b/public/app/features/plugins/extensions/errorHandling.test.ts index a6cb6bdcdcc..75e12ecbcf0 100644 --- a/public/app/features/plugins/extensions/errorHandling.test.ts +++ b/public/app/features/plugins/extensions/errorHandling.test.ts @@ -13,11 +13,6 @@ describe('error handling for extensions', () => { }); const context = {}; - const extension: AppPluginExtensionLink = { - title: 'Go to page one', - description: 'Will navigate the user to page one', - path: `/a/${pluginId}/one`, - }; it('should return configured link if configure is successful', () => { const configureWithErrorHandling = errorHandler(() => { @@ -26,7 +21,7 @@ describe('error handling for extensions', () => { }; }); - const configured = configureWithErrorHandling(extension, context); + const configured = configureWithErrorHandling(context); expect(configured).toEqual({ title: 'This is a new title', @@ -38,7 +33,7 @@ describe('error handling for extensions', () => { throw new Error(); }); - const configured = configureWithErrorHandling(extension, context); + const configured = configureWithErrorHandling(context); expect(configured).toBeUndefined(); }); @@ -47,7 +42,7 @@ describe('error handling for extensions', () => { const promisebased = (async () => {}) as ConfigureFunc; const configureWithErrorHandling = errorHandler(promisebased); - const configured = configureWithErrorHandling(extension, context); + const configured = configureWithErrorHandling(context); expect(configured).toBeUndefined(); }); @@ -56,7 +51,7 @@ describe('error handling for extensions', () => { const objectbased = {} as ConfigureFunc; const configureWithErrorHandling = errorHandler(objectbased); - const configured = configureWithErrorHandling(extension, context); + const configured = configureWithErrorHandling(context); expect(configured).toBeUndefined(); }); @@ -65,7 +60,7 @@ describe('error handling for extensions', () => { const returnString = (() => '') as ConfigureFunc; const configureWithErrorHandling = errorHandler(returnString); - const configured = configureWithErrorHandling(extension, context); + const configured = configureWithErrorHandling(context); expect(configured).toBeUndefined(); }); @@ -74,7 +69,7 @@ describe('error handling for extensions', () => { const returnUndefined = () => undefined; const configureWithErrorHandling = errorHandler(returnUndefined); - const configured = configureWithErrorHandling(extension, context); + const configured = configureWithErrorHandling(context); expect(configured).toBeUndefined(); }); diff --git a/public/app/features/plugins/extensions/errorHandling.ts b/public/app/features/plugins/extensions/errorHandling.ts index 9ef7b5ef0ee..c7a51e32176 100644 --- a/public/app/features/plugins/extensions/errorHandling.ts +++ b/public/app/features/plugins/extensions/errorHandling.ts @@ -12,14 +12,14 @@ export function handleErrorsInConfigure(options: Options) { const { pluginId, title, logger } = options; return (configure: ConfigureFunc): ConfigureFunc => { - return function handleErrors(extension, context) { + return function handleErrors(context) { try { if (!isFunction(configure)) { logger(`[Plugins] ${pluginId} provided invalid configuration function for extension '${title}'.`); return; } - const result = configure(extension, context); + const result = configure(context); if (result instanceof Promise) { logger( `[Plugins] ${pluginId} provided an unsupported async/promise-based configureation function for extension '${title}'.` diff --git a/public/app/features/plugins/extensions/registryFactory.test.ts b/public/app/features/plugins/extensions/registryFactory.test.ts index 891594dba9d..372efae9844 100644 --- a/public/app/features/plugins/extensions/registryFactory.test.ts +++ b/public/app/features/plugins/extensions/registryFactory.test.ts @@ -8,15 +8,15 @@ import { PluginExtensionRegistry } from '@grafana/runtime'; import { createPluginExtensionRegistry } from './registryFactory'; -const validateLink = jest.fn((configure, extension, context) => configure?.(extension, context)); -const configureErrorHandler = jest.fn((configure, extension, context) => configure?.(extension, context)); +const validateLink = jest.fn((configure, context) => configure?.(context)); +const configureErrorHandler = jest.fn((configure, context) => configure?.(context)); const commandErrorHandler = jest.fn((configure, context) => configure?.(context)); jest.mock('./errorHandling', () => ({ ...jest.requireActual('./errorHandling'), handleErrorsInConfigure: jest.fn(() => { return jest.fn((configure) => { - return jest.fn((extension, context) => configureErrorHandler(configure, extension, context)); + return jest.fn((context) => configureErrorHandler(configure, context)); }); }), handleErrorsInHandler: jest.fn(() => { @@ -30,7 +30,7 @@ jest.mock('./validateLink', () => ({ ...jest.requireActual('./validateLink'), createLinkValidator: jest.fn(() => { return jest.fn((configure) => { - return jest.fn((extension, context) => validateLink(configure, extension, context)); + return jest.fn((context) => validateLink(configure, context)); }); }), })); @@ -205,15 +205,10 @@ describe('createPluginExtensionRegistry()', () => { const [configure] = registry[linkConfig.placement]; const context = {}; - const configurable = { - title: linkConfig.title, - description: linkConfig.description, - path: linkConfig.path, - }; configure(context); - expect(validateLink).toBeCalledWith(expect.any(Function), configurable, context); + expect(validateLink).toBeCalledWith(expect.any(Function), context); }); it('should wrap configure function with extension error handling', () => { @@ -232,15 +227,10 @@ describe('createPluginExtensionRegistry()', () => { const [configure] = registry[linkConfig.placement]; const context = {}; - const configurable = { - title: linkConfig.title, - description: linkConfig.description, - path: linkConfig.path, - }; configure(context); - expect(configureErrorHandler).toBeCalledWith(expect.any(Function), configurable, context); + expect(configureErrorHandler).toBeCalledWith(expect.any(Function), context); }); it('should return undefined if returned by the provided extension config', () => { @@ -394,15 +384,11 @@ describe('createPluginExtensionRegistry()', () => { const [configure] = registry[commandConfig1.placement]; const context = {}; - const configurable = { - title: commandConfig1.title, - description: commandConfig2.description, - }; configure(context); // The error handler is wrapping (decorating) the configure function, so it can provide standard error messages - expect(configureErrorHandler).toBeCalledWith(expect.any(Function), configurable, context); + expect(configureErrorHandler).toBeCalledWith(expect.any(Function), context); }); it('should return undefined if returned by the provided extension config', () => { diff --git a/public/app/features/plugins/extensions/registryFactory.ts b/public/app/features/plugins/extensions/registryFactory.ts index 807d7f6d7b7..916206bb214 100644 --- a/public/app/features/plugins/extensions/registryFactory.ts +++ b/public/app/features/plugins/extensions/registryFactory.ts @@ -86,13 +86,7 @@ function createCommandRegistryItem( const handler = catchErrorsInHandler(handlerWithHelpers); const extensionFactory = createCommandFactory(pluginId, config, handler); - - const configurable: AppPluginExtensionCommand = { - title: config.title, - description: config.description, - }; - - const mapper = mapToConfigure(extensionFactory, configurable); + const mapper = mapToConfigure(extensionFactory); const catchErrorsInConfigure = handleErrorsInConfigure(options); return mapper(catchErrorsInConfigure(configure)); @@ -110,14 +104,7 @@ function createLinkRegistryItem( const options = { pluginId: pluginId, title: config.title, logger: console.warn }; const extensionFactory = createLinkFactory(pluginId, config); - - const configurable: AppPluginExtensionLink = { - title: config.title, - description: config.description, - path: config.path, - }; - - const mapper = mapToConfigure(extensionFactory, configurable); + const mapper = mapToConfigure(extensionFactory); const withConfigureErrorHandling = handleErrorsInConfigure(options); const validateLink = createLinkValidator(options); @@ -125,7 +112,7 @@ function createLinkRegistryItem( } function createLinkFactory(pluginId: string, config: AppPluginExtensionLinkConfig) { - return (override: Partial, context?: object): PluginExtensionLink => { + return (override: Partial): PluginExtensionLink => { const title = override?.title ?? config.title; const description = override?.description ?? config.description; const path = override?.path ?? config.path; @@ -160,16 +147,15 @@ function createCommandFactory( } function mapToConfigure( - commandFactory: (override: Partial, context?: object) => T | undefined, - configurable: C + extensionFactory: (override: Partial, context?: object) => T | undefined ): (configure: ConfigureFunc) => PluginExtensionRegistryItem { return (configure) => { return function mapToExtension(context?: object): T | undefined { - const override = configure(configurable, context); + const override = configure(context); if (!override) { return undefined; } - return commandFactory(override, context); + return extensionFactory(override, context); }; }; } diff --git a/public/app/features/plugins/extensions/types.ts b/public/app/features/plugins/extensions/types.ts index fa04cb7b346..03bcceae68b 100644 --- a/public/app/features/plugins/extensions/types.ts +++ b/public/app/features/plugins/extensions/types.ts @@ -1,4 +1,4 @@ import type { AppPluginExtensionCommandConfig } from '@grafana/data'; export type CommandHandlerFunc = AppPluginExtensionCommandConfig['handler']; -export type ConfigureFunc = (extension: T, context?: object) => Partial | undefined; +export type ConfigureFunc = (context?: object) => Partial | undefined; diff --git a/public/app/features/plugins/extensions/validateLink.test.ts b/public/app/features/plugins/extensions/validateLink.test.ts index d47a6bc0cf9..9187b54a971 100644 --- a/public/app/features/plugins/extensions/validateLink.test.ts +++ b/public/app/features/plugins/extensions/validateLink.test.ts @@ -1,5 +1,3 @@ -import type { AppPluginExtensionLink } from '@grafana/data'; - import { createLinkValidator } from './validateLink'; describe('extension link validator', () => { @@ -11,11 +9,6 @@ describe('extension link validator', () => { }); const context = {}; - const extension: AppPluginExtensionLink = { - title: 'Go to page one', - description: 'Will navigate the user to page one', - path: `/a/${pluginId}/one`, - }; it('should return link configuration if path is valid', () => { const configureWithValidation = validator(() => { @@ -24,7 +17,7 @@ describe('extension link validator', () => { }; }); - const configured = configureWithValidation(extension, context); + const configured = configureWithValidation(context); expect(configured).toEqual({ path: `/a/${pluginId}/other`, }); @@ -37,7 +30,7 @@ describe('extension link validator', () => { }; }); - const configured = configureWithValidation(extension, context); + const configured = configureWithValidation(context); expect(configured).toEqual({ title: 'Go to page two' }); }); @@ -48,7 +41,7 @@ describe('extension link validator', () => { }; }); - const configured = configureWithValidation(extension, context); + const configured = configureWithValidation(context); expect(configured).toBeUndefined(); }); @@ -57,7 +50,7 @@ describe('extension link validator', () => { return undefined; }); - const configured = configureWithValidation(extension, context); + const configured = configureWithValidation(context); expect(configured).toBeUndefined(); }); }); diff --git a/public/app/features/plugins/extensions/validateLink.ts b/public/app/features/plugins/extensions/validateLink.ts index c8c4d214196..ef7626980a9 100644 --- a/public/app/features/plugins/extensions/validateLink.ts +++ b/public/app/features/plugins/extensions/validateLink.ts @@ -14,8 +14,8 @@ export function createLinkValidator(options: Options) { const { pluginId, title, logger } = options; return (configure: ConfigureFunc): ConfigureFunc => { - return function validateLink(link, context) { - const configured = configure(link, context); + return function validateLink(context) { + const configured = configure(context); if (!isString(configured?.path)) { return configured;