feat: Plugin loader should prioritize new plugin format, when availab…

…le (#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.

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();
+  });
+});

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<PluginManifest> {
   }
 }
 
+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 {

packages/plugin/src/PluginTypes.test.ts

@@ -4,15 +4,25 @@ import {
   isDashboardPlugin,
   isTablePlugin,
   isThemePlugin,
+  isWidgetPlugin,
+  Plugin,
+  isPlugin,
 } from './PluginTypes';
 
 const pluginTypeToTypeGuardMap = [
   [PluginType.DASHBOARD_PLUGIN, isDashboardPlugin],
   [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);
+  });
+});

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)
+  );
+}