Separate interfaces for installed, bundled and downloadable plugins

Summary: I've re-designed interfaces describing plugins as I found that mental overhead working with them became too expensive because of slightly flawed design. However this cascaded changes in many files so you can see how extensively these interfaces used in our codebase. Before this change we had one interface PluginDetails which described three different entities: 1) plugins installed on the disk 2) plugins bundled into Flipper 3) plugins available on Marketplace. It's hard to use this "general" PluginDetails interface because of this as you always need to think about all three use cases everywhere. After this change we have 3 separate interfaces: InstalledPluginDetails, BundledPluginDetails and DownloadablePluginDetails and things became much type-safer now. Reviewed By: mweststrate Differential Revision: D25530383 fbshipit-source-id: b93593916a980c04e36dc6ffa168797645a0ff9c
2020-12-15 09:28:58 -08:00
parent efb82e80b5
commit 5383017299
27 changed files with 327 additions and 216 deletions
--- a/desktop/plugin-lib/src/PluginDetails.ts
+++ b/desktop/plugin-lib/src/PluginDetails.ts
@@ -8,15 +8,12 @@
 */

 export interface PluginDetails {
-  dir: string;
  name: string;
  specVersion: number;
  version: string;
  source: string;
  main: string;
  id: string;
-  isDefault: boolean;
-  entry: string;
  gatekeeper?: string;
  title: string;
  icon?: string;
@@ -32,7 +29,36 @@ export interface PluginDetails {
  flipperSDKVersion?: string;
 }

-export interface DownloadablePluginDetails extends PluginDetails {
+export interface ConcretePluginDetails extends PluginDetails {
+  // Determines whether the plugin is a part of the Flipper JS bundle.
+  isBundled: boolean;
+  // Determines whether the plugin is physically available for activation in Flipper.
+  isActivatable: boolean;
+}
+
+// Describes plugin which is a part of the Flipper JS bundle.
+export interface BundledPluginDetails extends ConcretePluginDetails {
+  isBundled: true;
+  isActivatable: true;
+}
+
+// Describes plugin installed on the disk.
+export interface InstalledPluginDetails extends ConcretePluginDetails {
+  isBundled: false;
+  isActivatable: true;
+  dir: string;
+  entry: string;
+}
+
+// Describes plugin physically available for activation in Flipper.
+export type ActivatablePluginDetails =
+  | BundledPluginDetails
+  | InstalledPluginDetails;
+
+// Describes plugin available for downloading. Until downloaded to the disk it is not available for activation in Flipper.
+export interface DownloadablePluginDetails extends ConcretePluginDetails {
+  isActivatable: false;
+  isBundled: false;
  downloadUrl: string;
  lastUpdated: Date;
 }
--- a/desktop/plugin-lib/src/tests/getPluginDetails.node.ts
+++ b/desktop/plugin-lib/src/tests/getPluginDetails.node.ts
@@ -9,7 +9,7 @@

 import fs from 'fs-extra';
 import path from 'path';
-import {getPluginDetailsFromDir} from '../getPluginDetails';
+import {getInstalledPluginDetails} from '../getPluginDetails';
 import {pluginInstallationDir} from '../pluginPaths';
 import {normalizePath} from 'flipper-test-utils';

@@ -31,7 +31,7 @@ test('getPluginDetailsV1', async () => {
  };
  jest.mock('fs-extra', () => jest.fn());
  fs.readJson = jest.fn().mockImplementation(() => pluginV1);
-  const details = await getPluginDetailsFromDir(pluginPath);
+  const details = await getInstalledPluginDetails(pluginPath);
  details.dir = normalizePath(details.dir);
  details.entry = normalizePath(details.entry);
  expect(details).toMatchInlineSnapshot(`
@@ -45,7 +45,8 @@ test('getPluginDetailsV1', async () => {
      "gatekeeper": "GK_flipper_plugin_test",
      "icon": undefined,
      "id": "flipper-plugin-test",
-      "isDefault": false,
+      "isActivatable": true,
+      "isBundled": false,
      "main": "dist/bundle.js",
      "name": "flipper-plugin-test",
      "source": "src/index.tsx",
@@ -69,7 +70,7 @@ test('getPluginDetailsV2', async () => {
  };
  jest.mock('fs-extra', () => jest.fn());
  fs.readJson = jest.fn().mockImplementation(() => pluginV2);
-  const details = await getPluginDetailsFromDir(pluginPath);
+  const details = await getInstalledPluginDetails(pluginPath);
  details.dir = normalizePath(details.dir);
  details.entry = normalizePath(details.entry);
  expect(details).toMatchInlineSnapshot(`
@@ -83,7 +84,8 @@ test('getPluginDetailsV2', async () => {
      "gatekeeper": "GK_flipper_plugin_test",
      "icon": undefined,
      "id": "flipper-plugin-test",
-      "isDefault": false,
+      "isActivatable": true,
+      "isBundled": false,
      "main": "dist/bundle.js",
      "name": "flipper-plugin-test",
      "source": "src/index.tsx",
@@ -107,7 +109,7 @@ test('id used as title if the latter omited', async () => {
  };
  jest.mock('fs-extra', () => jest.fn());
  fs.readJson = jest.fn().mockImplementation(() => pluginV2);
-  const details = await getPluginDetailsFromDir(pluginPath);
+  const details = await getInstalledPluginDetails(pluginPath);
  details.dir = normalizePath(details.dir);
  details.entry = normalizePath(details.entry);
  expect(details).toMatchInlineSnapshot(`
@@ -121,7 +123,8 @@ test('id used as title if the latter omited', async () => {
      "gatekeeper": "GK_flipper_plugin_test",
      "icon": undefined,
      "id": "test",
-      "isDefault": false,
+      "isActivatable": true,
+      "isBundled": false,
      "main": "dist/bundle.js",
      "name": "flipper-plugin-test",
      "source": "src/index.tsx",
@@ -144,7 +147,7 @@ test('name without "flipper-plugin-" prefix is used as title if the latter omite
  };
  jest.mock('fs-extra', () => jest.fn());
  fs.readJson = jest.fn().mockImplementation(() => pluginV2);
-  const details = await getPluginDetailsFromDir(pluginPath);
+  const details = await getInstalledPluginDetails(pluginPath);
  details.dir = normalizePath(details.dir);
  details.entry = normalizePath(details.entry);
  expect(details).toMatchInlineSnapshot(`
@@ -158,7 +161,8 @@ test('name without "flipper-plugin-" prefix is used as title if the latter omite
      "gatekeeper": "GK_flipper_plugin_test",
      "icon": undefined,
      "id": "flipper-plugin-test",
-      "isDefault": false,
+      "isActivatable": true,
+      "isBundled": false,
      "main": "dist/bundle.js",
      "name": "flipper-plugin-test",
      "source": "src/index.tsx",
@@ -184,7 +188,7 @@ test('flipper-plugin-version is parsed', async () => {
  };
  jest.mock('fs-extra', () => jest.fn());
  fs.readJson = jest.fn().mockImplementation(() => pluginV2);
-  const details = await getPluginDetailsFromDir(pluginPath);
+  const details = await getInstalledPluginDetails(pluginPath);
  details.dir = normalizePath(details.dir);
  details.entry = normalizePath(details.entry);
  expect(details).toMatchInlineSnapshot(`
@@ -198,7 +202,8 @@ test('flipper-plugin-version is parsed', async () => {
      "gatekeeper": "GK_flipper_plugin_test",
      "icon": undefined,
      "id": "flipper-plugin-test",
-      "isDefault": false,
+      "isActivatable": true,
+      "isBundled": false,
      "main": "dist/bundle.js",
      "name": "flipper-plugin-test",
      "source": "src/index.tsx",
--- a/desktop/plugin-lib/src/tests/getUpdatablePlugins.node.ts
+++ b/desktop/plugin-lib/src/tests/getUpdatablePlugins.node.ts
@@ -18,7 +18,7 @@ import {
 import {getInstalledPlugins} from '../pluginInstaller';
 import {mocked} from 'ts-jest/utils';
 import type {Package} from 'npm-api';
-import PluginDetails from '../PluginDetails';
+import {InstalledPluginDetails} from '../PluginDetails';

 jest.mock('npm-api', () => {
  return jest.fn().mockImplementation(() => {
@@ -54,7 +54,7 @@ jest.mock('npm-api', () => {
  });
 });

-const installedPlugins: PluginDetails[] = [
+const installedPlugins: InstalledPluginDetails[] = [
  {
    name: 'flipper-plugin-hello',
    entry: './test/index.js',
@@ -66,7 +66,8 @@ const installedPlugins: PluginDetails[] = [
    id: 'Hello',
    title: 'Hello',
    description: 'World?',
-    isDefault: false,
+    isBundled: false,
+    isActivatable: true,
  },
  {
    name: 'flipper-plugin-world',
@@ -79,7 +80,8 @@ const installedPlugins: PluginDetails[] = [
    id: 'World',
    title: 'World',
    description: 'Hello?',
-    isDefault: false,
+    isBundled: false,
+    isActivatable: true,
  },
 ];

--- a/desktop/plugin-lib/src/getPluginDetails.ts
+++ b/desktop/plugin-lib/src/getPluginDetails.ts
@@ -9,10 +9,14 @@

 import fs from 'fs-extra';
 import path from 'path';
-import {PluginDetails} from './PluginDetails';
-import {getPluginVersionInstallationDir, pluginCacheDir} from './pluginPaths';
+import {
+  DownloadablePluginDetails,
+  InstalledPluginDetails,
+  PluginDetails,
+} from './PluginDetails';
+import {pluginCacheDir} from './pluginPaths';

-export async function getPluginDetails(pluginDir: string, packageJson: any) {
+export function getPluginDetails(packageJson: any): PluginDetails {
  const specVersion =
    packageJson.$schema &&
    packageJson.$schema ===
@@ -21,60 +25,61 @@ export async function getPluginDetails(pluginDir: string, packageJson: any) {
      : 1;
  switch (specVersion) {
    case 1:
-      return await getPluginDetailsV1(pluginDir, packageJson);
+      return getPluginDetailsV1(packageJson);
    case 2:
-      return await getPluginDetailsV2(pluginDir, packageJson);
+      return getPluginDetailsV2(packageJson);
    default:
      throw new Error(`Unknown plugin format version: ${specVersion}`);
  }
 }

-export async function getPluginDetailsFromDir(
-  pluginDir: string,
-): Promise<PluginDetails> {
-  const packageJson = await fs.readJson(path.join(pluginDir, 'package.json'));
-  return await getPluginDetails(pluginDir, packageJson);
+export async function getInstalledPluginDetails(
+  dir: string,
+  packageJson?: any,
+): Promise<InstalledPluginDetails> {
+  packageJson =
+    packageJson ?? (await fs.readJson(path.join(dir, 'package.json')));
+  const pluginDetails = getPluginDetails(packageJson);
+  const entry =
+    pluginDetails.specVersion === 1
+      ? path.resolve(
+          pluginCacheDir,
+          `${packageJson.name}@${packageJson.version || '0.0.0'}.js`,
+        )
+      : path.resolve(dir, packageJson.main);
+  return {
+    ...pluginDetails,
+    isBundled: false,
+    isActivatable: true,
+    dir,
+    entry,
+  };
 }

-export async function getPluginDetailsFromPackageJson(packageJson: any) {
-  const pluginDir = getPluginVersionInstallationDir(
-    packageJson.name,
-    packageJson.version,
-  );
-  return await getPluginDetails(pluginDir, packageJson);
-}
-
-export async function getDownloadablePluginDetails(
+export function getDownloadablePluginDetails(
  packageJson: any,
  downloadUrl: string,
  lastUpdated: Date,
-) {
-  const details = await getPluginDetailsFromPackageJson(packageJson);
+): DownloadablePluginDetails {
+  const details = getPluginDetails(packageJson);
  return {
    ...details,
+    isBundled: false,
+    isActivatable: false,
    downloadUrl,
    lastUpdated,
  };
 }

 // Plugins packaged using V1 are distributed as sources and compiled in run-time.
-async function getPluginDetailsV1(
-  pluginDir: string,
-  packageJson: any,
-): Promise<PluginDetails> {
+function getPluginDetailsV1(packageJson: any): PluginDetails {
  return {
    specVersion: 1,
-    dir: pluginDir,
    name: packageJson.name,
    version: packageJson.version,
    main: 'dist/bundle.js',
-    entry: path.join(
-      pluginCacheDir,
-      `${packageJson.name}@${packageJson.version || '0.0.0'}.js`,
-    ),
    source: packageJson.main,
    id: packageJson.name,
-    isDefault: false,
    gatekeeper: packageJson.gatekeeper,
    icon: packageJson.icon,
    title: packageJson.title || packageJson.name,
@@ -86,19 +91,13 @@ async function getPluginDetailsV1(
 }

 // Plugins packaged using V2 are pre-bundled, so compilation in run-time is not required for them.
-async function getPluginDetailsV2(
-  pluginDir: string,
-  packageJson: any,
-): Promise<PluginDetails> {
+function getPluginDetailsV2(packageJson: any): PluginDetails {
  return {
    specVersion: 2,
-    dir: pluginDir,
    name: packageJson.name,
    version: packageJson.version,
    main: packageJson.main,
-    entry: path.resolve(pluginDir, packageJson.main),
    source: packageJson.flipperBundlerEntry,
-    isDefault: false,
    id: packageJson.id || packageJson.name,
    gatekeeper: packageJson.gatekeeper,
    icon: packageJson.icon,
@@ -111,9 +110,10 @@ async function getPluginDetailsV2(
  };
 }

-function getTitleFromName(name: string) {
+function getTitleFromName(name: string): string {
  const prefix = 'flipper-plugin-';
  if (name.startsWith(prefix)) {
    return name.substr(prefix.length);
  }
+  return name;
 }
--- a/desktop/plugin-lib/src/getSourcePlugins.ts
+++ b/desktop/plugin-lib/src/getSourcePlugins.ts
@@ -11,16 +11,17 @@ import path from 'path';
 import fs from 'fs-extra';
 import expandTilde from 'expand-tilde';
 import {getPluginSourceFolders} from './pluginPaths';
-import {PluginDetails, getPluginDetails} from 'flipper-plugin-lib';
 import pmap from 'p-map';
 import pfilter from 'p-filter';
 import {satisfies} from 'semver';
+import {getInstalledPluginDetails} from './getPluginDetails';
+import {InstalledPluginDetails} from './PluginDetails';

 const flipperVersion = require('../package.json').version;

-export async function getSourcePlugins(): Promise<PluginDetails[]> {
+export async function getSourcePlugins(): Promise<InstalledPluginDetails[]> {
  const pluginFolders = await getPluginSourceFolders();
-  const entryPoints: {[key: string]: PluginDetails} = {};
+  const entryPoints: {[key: string]: InstalledPluginDetails} = {};
  const additionalPlugins = await pmap(pluginFolders, (path) =>
    entryPointForPluginFolder(path),
  );
@@ -47,7 +48,7 @@ export async function getSourcePlugins(): Promise<PluginDetails[]> {
 }
 async function entryPointForPluginFolder(
  pluginsDir: string,
-): Promise<{[key: string]: PluginDetails}> {
+): Promise<{[key: string]: InstalledPluginDetails}> {
  pluginsDir = expandTilde(pluginsDir);
  if (!fs.existsSync(pluginsDir)) {
    return {};
@@ -96,7 +97,7 @@ async function entryPointForPluginFolder(
    .then((packages) =>
      pmap(packages, async ({manifest, dir}) => {
        try {
-          const details = await getPluginDetails(dir, manifest);
+          const details = await getInstalledPluginDetails(dir, manifest);
          if (
            details.flipperSDKVersion &&
            !satisfies(flipperVersion, details.flipperSDKVersion)
@@ -117,7 +118,7 @@ async function entryPointForPluginFolder(
    )
    .then((plugins) => plugins.filter(notNull))
    .then((plugins) =>
-      plugins.reduce<{[key: string]: PluginDetails}>((acc, cv) => {
+      plugins.reduce<{[key: string]: InstalledPluginDetails}>((acc, cv) => {
        acc[cv!.name] = cv!;
        return acc;
      }, {}),
--- a/desktop/plugin-lib/src/getUpdatablePlugins.ts
+++ b/desktop/plugin-lib/src/getUpdatablePlugins.ts
@@ -7,15 +7,12 @@
 * @format
 */

-import PluginDetails from './PluginDetails';
+import {InstalledPluginDetails} from './PluginDetails';
 import {getInstalledPlugins} from './pluginInstaller';
 import semver from 'semver';
 import {getNpmHostedPlugins, NpmPackageDescriptor} from './getNpmHostedPlugins';
 import NpmApi from 'npm-api';
-import {
-  getPluginDetails,
-  getPluginDetailsFromPackageJson,
-} from './getPluginDetails';
+import {getInstalledPluginDetails, getPluginDetails} from './getPluginDetails';
 import {getPluginVersionInstallationDir} from './pluginPaths';
 import pmap from 'p-map';
 import {notNull} from './typeUtils';
@@ -31,7 +28,7 @@ export type UpdatablePlugin = {
  updateStatus: UpdateResult;
 };

-export type UpdatablePluginDetails = PluginDetails & UpdatablePlugin;
+export type UpdatablePluginDetails = InstalledPluginDetails & UpdatablePlugin;

 export async function getUpdatablePlugins(
  query?: string,
@@ -51,7 +48,7 @@ export async function getUpdatablePlugins(
            semver.lt(installedPlugin.version, npmPackageDescriptor.version)
          ) {
            const pkg = await npmApi.repo(npmPackageDescriptor.name).package();
-            const npmPluginDetails = await getPluginDetails(
+            const npmPluginDetails = await getInstalledPluginDetails(
              getPluginVersionInstallationDir(
                npmPackageDescriptor.name,
                npmPackageDescriptor.version,
@@ -91,7 +88,7 @@ export async function getUpdatablePlugins(
    async (notInstalledPlugin) => {
      try {
        const pkg = await npmApi.repo(notInstalledPlugin.name).package();
-        const npmPluginDetails = await getPluginDetailsFromPackageJson(pkg);
+        const npmPluginDetails = getPluginDetails(pkg);
        if (npmPluginDetails.specVersion === 1) {
          return null;
        }
--- a/desktop/plugin-lib/src/pluginInstaller.ts
+++ b/desktop/plugin-lib/src/pluginInstaller.ts
@@ -15,8 +15,8 @@ import decompress from 'decompress';
 import decompressTargz from 'decompress-targz';
 import decompressUnzip from 'decompress-unzip';
 import tmp from 'tmp';
-import PluginDetails from './PluginDetails';
-import {getPluginDetailsFromDir} from './getPluginDetails';
+import {InstalledPluginDetails} from './PluginDetails';
+import {getInstalledPluginDetails} from './getPluginDetails';
 import {
  getPluginVersionInstallationDir,
  getPluginDirNameFromPackageName,
@@ -37,8 +37,8 @@ function providePluginManagerNoDependencies(): PM {

 async function installPluginFromTempDir(
  sourceDir: string,
-): Promise<PluginDetails> {
-  const pluginDetails = await getPluginDetailsFromDir(sourceDir);
+): Promise<InstalledPluginDetails> {
+  const pluginDetails = await getInstalledPluginDetails(sourceDir);
  const {name, version} = pluginDetails;
  const backupDir = path.join(await getTmpDir(), `${name}-${version}`);
  const destinationDir = getPluginVersionInstallationDir(name, version);
@@ -63,7 +63,7 @@ async function installPluginFromTempDir(
    }
    throw err;
  }
-  return await getPluginDetailsFromDir(destinationDir);
+  return await getInstalledPluginDetails(destinationDir);
 }

 async function getPluginRootDir(dir: string) {
@@ -87,12 +87,12 @@ async function getPluginRootDir(dir: string) {
 export async function getInstalledPlugin(
  name: string,
  version: string,
-): Promise<PluginDetails | null> {
+): Promise<InstalledPluginDetails | null> {
  const dir = getPluginVersionInstallationDir(name, version);
  if (!(await fs.pathExists(dir))) {
    return null;
  }
-  return await getPluginDetailsFromDir(dir);
+  return await getInstalledPluginDetails(dir);
 }

 export async function installPluginFromNpm(name: string) {
@@ -114,7 +114,7 @@ export async function installPluginFromNpm(name: string) {

 export async function installPluginFromFile(
  packagePath: string,
-): Promise<PluginDetails> {
+): Promise<InstalledPluginDetails> {
  const tmpDir = await getTmpDir();
  try {
    const files = await decompress(packagePath, tmpDir, {
@@ -140,14 +140,14 @@ export async function removePlugins(
  await pmap(names, (name) => removePlugin(name));
 }

-export async function getInstalledPlugins(): Promise<PluginDetails[]> {
+export async function getInstalledPlugins(): Promise<InstalledPluginDetails[]> {
  const versionDirs = await getInstalledPluginVersionDirs();
  return pmap(
    versionDirs
      .filter(([_, versionDirs]) => versionDirs.length > 0)
      .map(([_, versionDirs]) => versionDirs[0]),
    (latestVersionDir) =>
-      getPluginDetailsFromDir(latestVersionDir).catch((err) => {
+      getInstalledPluginDetails(latestVersionDir).catch((err) => {
        console.error(`Failed to load plugin from ${latestVersionDir}`, err);
        return null;
      }),
@@ -187,7 +187,7 @@ export async function moveInstalledPluginsFromLegacyDir() {
      )
      .then((dirs) =>
        pmap(dirs, (dir) =>
-          getPluginDetailsFromDir(dir).catch(async (err) => {
+          getInstalledPluginDetails(dir).catch(async (err) => {
            console.error(
              `Failed to load plugin from ${dir} on moving legacy plugins. Removing it.`,
              err,