Introduce PluginIdentifier type to ensure we don't lookup plugins by …

…object identities
facebook · Mar 8, 2024 · ba4b935 · ba4b935
1 parent f0fdb8c
commit ba4b935
Show file tree

Hide file tree

Showing 5 changed files with 133 additions and 94 deletions.
diff --git a/packages/docusaurus-types/src/plugin.d.ts b/packages/docusaurus-types/src/plugin.d.ts
@@ -163,6 +163,15 @@ export type Plugin<Content = unknown> = {
   }) => ThemeConfig;
 };
 
+/**
+ * Data required to uniquely identify a plugin
+ * The name or instance id alone is not enough
+ */
+export type PluginIdentifier = {
+  readonly name: string;
+  readonly id: string;
+};
+
 export type InitializedPlugin = Plugin & {
   readonly options: Required<PluginOptions>;
   readonly version: PluginVersionInformation;

diff --git a/packages/docusaurus/src/commands/start/start.ts b/packages/docusaurus/src/commands/start/start.ts
@@ -5,18 +5,13 @@
  * LICENSE file in the root directory of this source tree.
  */
 
-import fs from 'fs-extra';
-import _ from 'lodash';
 import logger from '@docusaurus/logger';
 import openBrowser from 'react-dev-utils/openBrowser';
-import {loadSite, reloadSite, reloadSitePlugin} from '../../server/site';
-import {PerfLogger} from '../../utils';
 import {setupSiteFileWatchers} from './watcher';
 import {createWebpackDevServer} from './webpack';
-import {createOpenUrlContext} from './utils';
-import type {LoadContextParams, LoadSiteParams} from '../../server/site';
+import {createReloadableSite} from './utils';
+import type {LoadContextParams} from '../../server/site';
 import type {HostPortOptions} from '../../server/getHostPort';
-import type {LoadedPlugin} from '@docusaurus/types';
 
 export type StartCLIOptions = HostPortOptions &
   Pick<LoadContextParams, 'locale' | 'config'> & {
@@ -26,77 +21,6 @@ export type StartCLIOptions = HostPortOptions &
     minify?: boolean;
   };
 
-async function createLoadSiteParams({
-  siteDirParam,
-  cliOptions,
-}: StartParams): Promise<LoadSiteParams> {
-  const siteDir = await fs.realpath(siteDirParam);
-  return {
-    siteDir,
-    config: cliOptions.config,
-    locale: cliOptions.locale,
-    localizePath: undefined, // Should this be configurable?
-  };
-}
-
-async function createReloadableSite(startParams: StartParams) {
-  const openUrlContext = await createOpenUrlContext(startParams);
-
-  let site = await PerfLogger.async('Loading site', async () => {
-    const params = await createLoadSiteParams(startParams);
-    return loadSite(params);
-  });
-
-  const get = () => site;
-
-  const getOpenUrl = () =>
-    openUrlContext.getOpenUrl({
-      baseUrl: site.props.baseUrl,
-    });
-
-  const printOpenUrlMessage = () => {
-    logger.success`Docusaurus website is running at: url=${getOpenUrl()}`;
-  };
-  printOpenUrlMessage();
-
-  const reloadBase = async () => {
-    try {
-      const oldSite = site;
-      site = await PerfLogger.async('Reloading site', () => reloadSite(site));
-      if (oldSite.props.baseUrl !== site.props.baseUrl) {
-        printOpenUrlMessage();
-      }
-    } catch (e) {
-      logger.error('Site reload failure');
-      console.error(e);
-    }
-  };
-
-  // TODO instead of debouncing we should rather add AbortController support
-  const reload = _.debounce(reloadBase, 500);
-
-  const reloadPlugin = async (plugin: LoadedPlugin) => {
-    try {
-      site = await PerfLogger.async(
-        `Reloading site plugin ${plugin.name}@${plugin.options.id}`,
-        () => reloadSitePlugin(site, plugin),
-      );
-    } catch (e) {
-      logger.error(
-        `Site plugin reload failure - Plugin ${plugin.name}@${plugin.options.id}`,
-      );
-      console.error(e);
-    }
-  };
-
-  return {get, getOpenUrl, reload, reloadPlugin, openUrlContext};
-}
-
-type StartParams = {
-  siteDirParam: string;
-  cliOptions: Partial<StartCLIOptions>;
-};
-
 export async function start(
   siteDirParam: string = '.',
   cliOptions: Partial<StartCLIOptions> = {},
@@ -108,6 +32,7 @@ export async function start(
   process.env.DOCUSAURUS_CURRENT_LOCALE = cliOptions.locale;
 
   const reloadableSite = await createReloadableSite({siteDirParam, cliOptions});
+
   setupSiteFileWatchers(
     {props: reloadableSite.get().props, cliOptions},
     ({plugin}) => {

diff --git a/packages/docusaurus/src/commands/start/utils.ts b/packages/docusaurus/src/commands/start/utils.ts
@@ -5,10 +5,21 @@
  * LICENSE file in the root directory of this source tree.
  */
 
+import fs from 'fs-extra';
+import _ from 'lodash';
 import {prepareUrls} from 'react-dev-utils/WebpackDevServerUtils';
 import {normalizeUrl} from '@docusaurus/utils';
+import logger from '@docusaurus/logger';
 import {getHostPort} from '../../server/getHostPort';
+import {PerfLogger} from '../../utils';
+import {
+  loadSite,
+  type LoadSiteParams,
+  reloadSite,
+  reloadSitePlugin,
+} from '../../server/site';
 import type {StartCLIOptions} from './start';
+import type {LoadedPlugin} from '@docusaurus/types';
 
 export type OpenUrlContext = {
   host: string;
@@ -35,3 +46,81 @@ export async function createOpenUrlContext({
 
   return {host, port, getOpenUrl};
 }
+
+type StartParams = {
+  siteDirParam: string;
+  cliOptions: Partial<StartCLIOptions>;
+};
+
+async function createLoadSiteParams({
+  siteDirParam,
+  cliOptions,
+}: StartParams): Promise<LoadSiteParams> {
+  const siteDir = await fs.realpath(siteDirParam);
+  return {
+    siteDir,
+    config: cliOptions.config,
+    locale: cliOptions.locale,
+    localizePath: undefined, // Should this be configurable?
+  };
+}
+
+// eslint-disable-next-line @typescript-eslint/explicit-module-boundary-types
+export async function createReloadableSite(startParams: StartParams) {
+  const openUrlContext = await createOpenUrlContext(startParams);
+
+  let site = await PerfLogger.async('Loading site', async () => {
+    const params = await createLoadSiteParams(startParams);
+    return loadSite(params);
+  });
+
+  const get = () => site;
+
+  const getOpenUrl = () =>
+    openUrlContext.getOpenUrl({
+      baseUrl: site.props.baseUrl,
+    });
+
+  const printOpenUrlMessage = () => {
+    logger.success`Docusaurus website is running at: url=${getOpenUrl()}`;
+  };
+  printOpenUrlMessage();
+
+  const reloadBase = async () => {
+    try {
+      const oldSite = site;
+      site = await PerfLogger.async('Reloading site', () => reloadSite(site));
+      if (oldSite.props.baseUrl !== site.props.baseUrl) {
+        printOpenUrlMessage();
+      }
+    } catch (e) {
+      logger.error('Site reload failure');
+      console.error(e);
+    }
+  };
+
+  // TODO instead of debouncing we should rather add AbortController support?
+  const reload = _.debounce(reloadBase, 500);
+
+  // TODO this could be subject to plugin reloads race conditions
+  //  In practice, it is not likely the user will hot reload 2 plugins at once
+  //  but we should still support it and probably use a task queuing system
+  const reloadPlugin = async (plugin: LoadedPlugin) => {
+    try {
+      site = await PerfLogger.async(
+        `Reloading site plugin ${plugin.name}@${plugin.options.id}`,
+        () => {
+          const pluginIdentifier = {name: plugin.name, id: plugin.options.id};
+          return reloadSitePlugin(site, pluginIdentifier);
+        },
+      );
+    } catch (e) {
+      logger.error(
+        `Site plugin reload failure - Plugin ${plugin.name}@${plugin.options.id}`,
+      );
+      console.error(e);
+    }
+  };
+
+  return {get, getOpenUrl, reload, reloadPlugin, openUrlContext};
+}
diff --git a/packages/docusaurus/src/server/plugins/plugins.ts b/packages/docusaurus/src/server/plugins/plugins.ts
@@ -8,6 +8,7 @@
 import path from 'path';
 import _ from 'lodash';
 import {docuHash, generate} from '@docusaurus/utils';
+import logger from '@docusaurus/logger';
 import {initPlugins} from './init';
 import {createBootstrapPlugin, createMDXFallbackPlugin} from './synthetic';
 import {localizePluginTranslationFile} from '../translations/translations';
@@ -23,6 +24,7 @@ import type {
   InitializedPlugin,
   PluginRouteContext,
 } from '@docusaurus/types';
+import type {PluginIdentifier} from '@docusaurus/types/src/plugin';
 
 async function translatePlugin({
   plugin,
@@ -270,27 +272,39 @@ export async function loadPlugins(
   });
 }
 
+export function getPluginByIdentifier({
+  plugins,
+  pluginIdentifier,
+}: {
+  pluginIdentifier: PluginIdentifier;
+  plugins: LoadedPlugin[];
+}): LoadedPlugin {
+  const plugin = plugins.find(
+    (p) =>
+      p.name === pluginIdentifier.name && p.options.id === pluginIdentifier.id,
+  );
+  if (!plugin) {
+    throw new Error(
+      logger.interpolate`Plugin not found for identifier ${pluginIdentifier.name}@${pluginIdentifier.id}`,
+    );
+  }
+  return plugin;
+}
+
 export async function reloadPlugin({
-  plugin,
+  pluginIdentifier,
   plugins,
   context,
 }: {
-  plugin: LoadedPlugin;
+  pluginIdentifier: PluginIdentifier;
   plugins: LoadedPlugin[];
   context: LoadContext;
 }): Promise<LoadPluginsResult> {
   return PerfLogger.async('Plugins - reloadPlugin', async () => {
-    const pluginIndex = plugins.findIndex(
-      (p) => p.name === plugin.name && p.options.id === plugin.options.id,
-    );
-    if (pluginIndex === -1) {
-      throw new Error(
-        'Unexpected: this code assumes the plugin to reload is in the list of provided plugins',
-      );
-    }
+    const plugin = getPluginByIdentifier({plugins, pluginIdentifier});
 
     const reloadedPlugin = await executePluginLoadContent({plugin, context});
-    const newPlugins = plugins.with(pluginIndex, reloadedPlugin);
+    const newPlugins = plugins.with(plugins.indexOf(plugin), reloadedPlugin);
 
     // Unfortunately, due to the "AllContent" data we have to re-execute this
     // for all plugins, not just the one to reload...

diff --git a/packages/docusaurus/src/server/site.ts b/packages/docusaurus/src/server/site.ts
@@ -30,9 +30,9 @@ import type {
   DocusaurusConfig,
   GlobalData,
   LoadContext,
-  LoadedPlugin,
   Props,
 } from '@docusaurus/types';
+import type {PluginIdentifier} from '@docusaurus/types/src/plugin';
 
 export type LoadContextParams = {
   /** Usually the CWD; can be overridden with command argument. */
@@ -245,12 +245,14 @@ export async function reloadSite(site: Site): Promise<Site> {
 
 export async function reloadSitePlugin(
   site: Site,
-  plugin: LoadedPlugin,
+  pluginIdentifier: PluginIdentifier,
 ): Promise<Site> {
-  console.log(`reloadSitePlugin ${plugin.name}`);
+  console.log(
+    `reloadSitePlugin ${pluginIdentifier.name}@${pluginIdentifier.id}`,
+  );
 
   const {plugins, routes, globalData} = await reloadPlugin({
-    plugin,
+    pluginIdentifier,
     plugins: site.props.plugins,
     context: site.props,
   });
@@ -267,7 +269,7 @@ export async function reloadSitePlugin(
     params: site.params,
   };
 
-  // TODO optimize, bypass codegen if new site is similar to old site
+  // TODO optimize, bypass useless codegen if new site is similar to old site
   await createSiteFiles({site: newSite, globalData});
 
   return newSite;