From c6ef5b37efbbea6cd8b8a8fd3597b99827d59284 Mon Sep 17 00:00:00 2001 From: Vlad Babich Date: Thu, 29 Feb 2024 14:34:24 -0700 Subject: [PATCH] feat: Plugin loader should prioritize new plugin format, when available (#1846) We should be able to use both new and legacy plugin format in the same module, and plugin loader should prioritize the new format. --- .../app-utils/src/plugins/PluginUtils.test.ts | 129 ++++++++++++++++++ .../app-utils/src/plugins/PluginUtils.tsx | 38 ++++-- packages/plugin/src/PluginTypes.test.ts | 21 +++ packages/plugin/src/PluginTypes.ts | 10 ++ 4 files changed, 190 insertions(+), 8 deletions(-) create mode 100644 packages/app-utils/src/plugins/PluginUtils.test.ts diff --git a/packages/app-utils/src/plugins/PluginUtils.test.ts b/packages/app-utils/src/plugins/PluginUtils.test.ts new file mode 100644 index 0000000000..f21a2ebeb6 --- /dev/null +++ b/packages/app-utils/src/plugins/PluginUtils.test.ts @@ -0,0 +1,129 @@ +import { LegacyPlugin, Plugin, PluginType } from '@deephaven/plugin'; +import { getPluginModuleValue } from './PluginUtils'; + +describe('getPluginModuleValue', () => { + const legacyPlugins: [type: string, moduleValue: LegacyPlugin][] = [ + [ + 'dashboard', + { + DashboardPlugin: () => null, + }, + ], + [ + 'auth', + { + AuthPlugin: { + Component: () => null, + isAvailable: () => true, + }, + }, + ], + [ + 'table', + { + TablePlugin: () => null, + }, + ], + ]; + + const newPlugins: [type: string, moduleValue: Plugin][] = Object.keys( + PluginType + ).map(type => [type, { name: `${type}`, type: PluginType[type] }]); + + const newPluginsWithNamedExports: [ + type: string, + moduleValue: { default: Plugin; [key: string]: unknown }, + ][] = Object.keys(PluginType).map(type => [ + type, + { + default: { name: `${type}Plugin`, type: PluginType[type] }, + NamedExport: 'NamedExportValue', + }, + ]); + + const combinedPlugins: [ + type: string, + moduleValue: { + default: Plugin; + } & LegacyPlugin, + ][] = [ + [ + 'dashboard', + { + default: { + name: 'combinedFormat1', + type: PluginType.DASHBOARD_PLUGIN, + }, + DashboardPlugin: () => null, + }, + ], + [ + 'auth', + { + default: { + name: 'combinedFormat2', + type: PluginType.AUTH_PLUGIN, + }, + AuthPlugin: { + Component: () => null, + isAvailable: () => true, + }, + }, + ], + [ + 'table', + { + default: { + name: 'combinedFormat3', + type: PluginType.TABLE_PLUGIN, + }, + TablePlugin: () => null, + }, + ], + [ + // Should be able to combine different plugin types + 'multiple', + { + default: { + name: 'widgetPlugin', + type: PluginType.WIDGET_PLUGIN, + }, + DashboardPlugin: () => null, + }, + ], + ]; + + it.each(legacyPlugins)( + 'supports legacy %s plugin format', + (type, legacyPlugin) => { + const moduleValue = getPluginModuleValue(legacyPlugin); + expect(moduleValue).toBe(legacyPlugin); + } + ); + + it.each(newPlugins)('supports new %s format', (type, plugin) => { + const moduleValue = getPluginModuleValue(plugin); + expect(moduleValue).toBe(plugin); + }); + + it.each(newPluginsWithNamedExports)( + 'supports new %s format with named exports', + (type, plugin) => { + const moduleValue = getPluginModuleValue(plugin); + expect(moduleValue).toBe(plugin.default); + } + ); + + it.each(combinedPlugins)( + 'prioritizes new %s plugin if the module contains both legacy and new format', + (type, plugin) => { + const moduleValue = getPluginModuleValue(plugin); + expect(moduleValue).toBe(plugin.default); + } + ); + + it('returns null if the module value is not a plugin', () => { + const moduleValue = getPluginModuleValue({} as Plugin); + expect(moduleValue).toBeNull(); + }); +}); diff --git a/packages/app-utils/src/plugins/PluginUtils.tsx b/packages/app-utils/src/plugins/PluginUtils.tsx index 75003fc18f..f8b7cef818 100644 --- a/packages/app-utils/src/plugins/PluginUtils.tsx +++ b/packages/app-utils/src/plugins/PluginUtils.tsx @@ -10,6 +10,8 @@ import { PluginType, isLegacyAuthPlugin, isLegacyPlugin, + PluginModule, + isPlugin, } from '@deephaven/plugin'; import loadRemoteModule from './loadRemoteModule'; @@ -52,6 +54,33 @@ export async function loadJson(jsonUrl: string): Promise { } } +function hasDefaultExport(value: unknown): value is { default: Plugin } { + return ( + typeof value === 'object' && + value != null && + typeof (value as { default?: unknown }).default === 'object' + ); +} + +export function getPluginModuleValue( + value: LegacyPlugin | Plugin | { default: Plugin } +): PluginModule | null { + // TypeScript builds CJS default exports differently depending on + // whether there are also named exports. If the default is the only + // export, it will be the value. If there are also named exports, + // it will be assigned to the `default` property on the value. + if (isPlugin(value)) { + return value; + } + if (hasDefaultExport(value) && isPlugin(value.default)) { + return value.default; + } + if (isLegacyPlugin(value)) { + return value; + } + return null; +} + /** * Load all plugin modules available based on the manifest file at the provided base URL * @param modulePluginsUrl The base URL of the module plugins to load @@ -83,14 +112,7 @@ export async function loadModulePlugins( const module = pluginModules[i]; const { name } = manifest.plugins[i]; if (module.status === 'fulfilled') { - const moduleValue = isLegacyPlugin(module.value) - ? module.value - : // TypeScript builds CJS default exports differently depending on - // whether there are also named exports. If the default is the only - // export, it will be the value. If there are also named exports, - // it will be assigned to the `default` property on the value. - module.value.default ?? module.value; - + const moduleValue = getPluginModuleValue(module.value); if (moduleValue == null) { log.error(`Plugin '${name}' is missing an exported value.`); } else { diff --git a/packages/plugin/src/PluginTypes.test.ts b/packages/plugin/src/PluginTypes.test.ts index de75414961..4593ae1555 100644 --- a/packages/plugin/src/PluginTypes.test.ts +++ b/packages/plugin/src/PluginTypes.test.ts @@ -4,6 +4,9 @@ import { isDashboardPlugin, isTablePlugin, isThemePlugin, + isWidgetPlugin, + Plugin, + isPlugin, } from './PluginTypes'; const pluginTypeToTypeGuardMap = [ @@ -11,8 +14,15 @@ const pluginTypeToTypeGuardMap = [ [PluginType.AUTH_PLUGIN, isAuthPlugin], [PluginType.TABLE_PLUGIN, isTablePlugin], [PluginType.THEME_PLUGIN, isThemePlugin], + [PluginType.WIDGET_PLUGIN, isWidgetPlugin], ] as const; +const pluginTypeToPluginMap: [type: string, moduleValue: Plugin][] = + Object.keys(PluginType).map(type => [ + type, + { name: `${type}`, type: PluginType[type] }, + ]); + describe.each(pluginTypeToTypeGuardMap)( 'plugin type guard: %s', (expectedPluginType, typeGuard) => { @@ -26,3 +36,14 @@ describe.each(pluginTypeToTypeGuardMap)( ); } ); + +describe('isPlugin', () => { + it.each(pluginTypeToPluginMap)('returns true for %s type', (type, plugin) => { + expect(isPlugin(plugin)).toBe(true); + }); + + it('returns false for non-plugin types', () => { + expect(isPlugin({ name: 'test', type: 'other' })).toBe(false); + expect(isPlugin({})).toBe(false); + }); +}); diff --git a/packages/plugin/src/PluginTypes.ts b/packages/plugin/src/PluginTypes.ts index ca2b463c22..e729636103 100644 --- a/packages/plugin/src/PluginTypes.ts +++ b/packages/plugin/src/PluginTypes.ts @@ -226,3 +226,13 @@ export interface ThemePlugin extends Plugin { export function isThemePlugin(plugin: PluginModule): plugin is ThemePlugin { return 'type' in plugin && plugin.type === PluginType.THEME_PLUGIN; } + +export function isPlugin(plugin: unknown): plugin is Plugin { + return ( + isDashboardPlugin(plugin as PluginModule) || + isAuthPlugin(plugin as PluginModule) || + isTablePlugin(plugin as PluginModule) || + isThemePlugin(plugin as PluginModule) || + isWidgetPlugin(plugin as PluginModule) + ); +}