refactor(padToggle): split server/client to keep client bundle clean

JohnMcLear · claude · JohnMcLear · commit c409de55eecc · 2026-05-07T16:03:53.000+01:00
Top-level requires of ep_etherpad-lite/node/* in pad-toggle.js were getting
crawled by esbuild when plugin authors imported the helper from client code,
even though the requires were inside try/catch and the chain only reached
leaf modules. The bundler resolves the package and pulls in everything
along the way. The existing convention (attributes.js client / attributes-
server.js server) handles this cleanly — adopt the same split:

- pad-toggle-server.js — server hooks (loadSettings, clientVars,
  eejsBlock_mySettings, eejsBlock_padSettings) + capability detection via
  PluginCapabilities.
- pad-toggle.js — client hooks (init, handleClientMessage_CLIENT_MESSAGE)
  with no top-level node-only requires.

Plugin authors:
  // server (index.js)
  const {padToggle} = require('ep_plugin_helpers/pad-toggle-server');
  // client (static/js/postAceInit.js)
  const {padToggle} = require('ep_plugin_helpers/pad-toggle');

The top-level `padToggle` getter on ep_plugin_helpers' index.js still
returns the server factory for callers who already use the index entry.

Co-Authored-By: Claude Opus 4.7 (1M context) &lt;noreply@anthropic.com&gt;
diff --git a/index.js b/index.js
@@ -23,7 +23,11 @@ module.exports = {
   // matching native Etherpad behavior. Pad-wide value rides the existing
   // padoptions broadcast/persist rail; degrades gracefully on cores that
   // lack the ep_* passthrough patch (Etherpad < 2.7.4).
-  get padToggle() { return require('./pad-toggle').padToggle; },
+  //
+  // Server side ONLY here. Client code must import the sub-path
+  // 'ep_plugin_helpers/pad-toggle' directly to avoid pulling settings-toggle
+  // and other server-only modules into the client bundle.
+  get padToggle() { return require('./pad-toggle-server').padToggle; },
 
   // Messages — intercept and relay real-time messages
   get messageRelay() { return require('./message-relay').messageRelay; },
diff --git a/pad-toggle-server.js b/pad-toggle-server.js
@@ -0,0 +1,100 @@
+'use strict';
+
+// padToggle (server side) — emits parallel checkboxes in the User Settings
+// (mySettings) and Pad Wide Settings (padSettings) panels, mirroring native
+// Etherpad behavior. Pad-wide values ride the existing padoptions COLLABROOM
+// rail (stored at pad.padOptions[pluginName] = {enabled: bool}) when the core
+// has the ep_* passthrough patch (Etherpad >= 2.7.4); on older cores the
+// pad-wide block silently no-ops and the user-side cookie toggle alone still
+// works.
+//
+// This module is intended for server-side import only. The companion
+// `pad-toggle.js` provides the client-side init/handleClientMessage hooks.
+// They share the same config; plugin authors should use identical
+// `pluginName`, `settingId`, `l10nId`, and `defaultEnabled` on both sides so
+// the checkbox ids and clientVars block line up.
+
+const PLUGIN_NAME_RE = /^ep_[a-z0-9_]+$/;
+
+let padOptionsPluginPassthrough = false;
+try {
+  // The require lands on a leaf module on patched cores (Etherpad >= 2.7.4)
+  // and throws on older cores. Server-only: this file is never bundled for
+  // the browser, so esbuild's static analysis does not run here.
+  // eslint-disable-next-line global-require
+  const caps = require('ep_etherpad-lite/node/utils/PluginCapabilities');
+  padOptionsPluginPassthrough = caps && caps.padOptionsPluginPassthrough === true;
+} catch (_e) { /* older core — leave as false */ }
+
+const validateConfig = (config) => {
+  if (!config || typeof config !== 'object') {
+    throw new Error('padToggle requires a config object');
+  }
+  const {pluginName, settingId, l10nId, defaultEnabled = true} = config;
+  if (!PLUGIN_NAME_RE.test(pluginName || '')) {
+    throw new Error(
+        `padToggle pluginName must match /^ep_[a-z0-9_]+$/, got: ${pluginName}`);
+  }
+  if (!settingId || typeof settingId !== 'string') {
+    throw new Error('padToggle requires settingId (string)');
+  }
+  if (!l10nId || typeof l10nId !== 'string') {
+    throw new Error('padToggle requires l10nId (string) — never hardcode a label');
+  }
+  return {pluginName, settingId, l10nId, defaultEnabled: !!defaultEnabled};
+};
+
+const renderCheckbox = (settingId, l10nId, idPrefix) =>
+  `<p>` +
+    `<input type="checkbox" id="${idPrefix}options-${settingId}">` +
+    `<label for="${idPrefix}options-${settingId}" data-l10n-id="${l10nId}"></label>` +
+  `</p>`;
+
+const padToggleServer = (rawConfig) => {
+  const {pluginName, settingId, l10nId, defaultEnabled} = validateConfig(rawConfig);
+  let cachedDefaultEnabled = defaultEnabled;
+
+  const loadSettings = async (hookName, args) => {
+    const ps = (args && args.settings && args.settings[pluginName]) || {};
+    if (typeof ps.defaultEnabled === 'boolean') cachedDefaultEnabled = ps.defaultEnabled;
+  };
+
+  const clientVars = async (hookName, ctx) => {
+    let initialPadEnabled = cachedDefaultEnabled;
+    try {
+      const padSettings = ctx && ctx.pad && typeof ctx.pad.getPadSettings === 'function'
+        ? ctx.pad.getPadSettings() : null;
+      const stored = padSettings && padSettings[pluginName];
+      if (stored && typeof stored.enabled === 'boolean') initialPadEnabled = stored.enabled;
+    } catch (_e) { /* leave initialPadEnabled at instance default */ }
+
+    return {
+      ep_plugin_helpers: {
+        padToggle: {
+          [pluginName]: {
+            padWideSupported: padOptionsPluginPassthrough,
+            settingId,
+            l10nId,
+            defaultEnabled: cachedDefaultEnabled,
+            initialPadEnabled,
+          },
+        },
+      },
+    };
+  };
+
+  const eejsBlock_mySettings = (hookName, args, cb) => {
+    args.content += renderCheckbox(settingId, l10nId, '');
+    return cb();
+  };
+
+  const eejsBlock_padSettings = (hookName, args, cb) => {
+    if (!padOptionsPluginPassthrough) return cb();
+    args.content += renderCheckbox(settingId, l10nId, 'padsettings-');
+    return cb();
+  };
+
+  return {loadSettings, clientVars, eejsBlock_mySettings, eejsBlock_padSettings};
+};
+
+module.exports = {padToggle: padToggleServer, createPadToggle: padToggleServer};
diff --git a/pad-toggle.js b/pad-toggle.js
@@ -1,30 +1,19 @@
 'use strict';
 
-// padToggle — emit parallel checkboxes in the User Settings (mySettings) and
-// Pad Wide Settings (padSettings) panels, mirroring how native settings like
-// stickychat / lineNumbers / disablechat work. The pad-wide value rides the
-// existing padoptions COLLABROOM rail (the helper's value lives at
-// pad.padOptions[pluginName] = {enabled: bool}), so broadcast, persistence,
-// creator-only-write, and enforceSettings semantics all come for free —
-// provided Etherpad core ships the ep_* passthrough patch (since 2.7.4).
+// padToggle (client side) — wires up the parallel User Settings / Pad Wide
+// Settings checkboxes that pad-toggle-server.js renders. Reads the helper's
+// clientVars block (capability flag + initial pad-wide value), persists the
+// per-user choice in padcookie, and forwards pad-wide changes through the
+// native pad.changePadOption() flow so they ride the existing padoptions
+// COLLABROOM broadcast.
 //
-// On older cores (no PluginCapabilities module), the pad-wide block is a
-// no-op and the user-side cookie toggle still works. Plugins built on this
-// helper continue to function everywhere; only the pad-wide column is gone.
+// This file deliberately has no top-level requires that touch server-only
+// modules — esbuild bundles it into the browser pad bundle, and any
+// node-only path would break the client build.
 
 const PLUGIN_NAME_RE = /^ep_[a-z0-9_]+$/;
 
-let padOptionsPluginPassthrough = false;
-try {
-  // Server-only. Wrapped because the Settings module pulls in node deps
-  // (fs, path) that don't exist in the esbuild-bundled client. The require
-  // also fails on Etherpad versions before the passthrough patch shipped.
-  // eslint-disable-next-line global-require
-  const caps = require('ep_etherpad-lite/node/utils/PluginCapabilities');
-  padOptionsPluginPassthrough = caps && caps.padOptionsPluginPassthrough === true;
-} catch (_e) { /* older core or client bundle — leave as false */ }
-
-const padToggle = (config) => {
+const validateConfig = (config) => {
   if (!config || typeof config !== 'object') {
     throw new Error('padToggle requires a config object');
   }
@@ -39,64 +28,19 @@ const padToggle = (config) => {
   if (!l10nId || typeof l10nId !== 'string') {
     throw new Error('padToggle requires l10nId (string) — never hardcode a label');
   }
+  return {pluginName, settingId, l10nId, defaultEnabled: !!defaultEnabled};
+};
 
+const padToggleClient = (rawConfig) => {
+  const {pluginName, settingId, defaultEnabled} = validateConfig(rawConfig);
   const userCheckboxId = `options-${settingId}`;
   const padCheckboxId = `padsettings-options-${settingId}`;
-  let cachedDefaultEnabled = !!defaultEnabled;
-
-  // ---------- Server hooks ----------
-
-  const loadSettings = async (hookName, args) => {
-    const ps = (args && args.settings && args.settings[pluginName]) || {};
-    if (typeof ps.defaultEnabled === 'boolean') cachedDefaultEnabled = ps.defaultEnabled;
-  };
-
-  const clientVars = async (hookName, ctx) => {
-    let initialPadEnabled = cachedDefaultEnabled;
-    try {
-      const padSettings = ctx && ctx.pad && typeof ctx.pad.getPadSettings === 'function'
-        ? ctx.pad.getPadSettings() : null;
-      const stored = padSettings && padSettings[pluginName];
-      if (stored && typeof stored.enabled === 'boolean') initialPadEnabled = stored.enabled;
-    } catch (_e) { /* leave initialPadEnabled at instance default */ }
-
-    const helperBlock = {
-      [pluginName]: {
-        padWideSupported: padOptionsPluginPassthrough,
-        settingId,
-        l10nId,
-        defaultEnabled: cachedDefaultEnabled,
-        initialPadEnabled,
-      },
-    };
-    return {ep_plugin_helpers: {padToggle: helperBlock}};
-  };
-
-  const renderCheckbox = (idPrefix) =>
-    `<p>` +
-      `<input type="checkbox" id="${idPrefix}options-${settingId}">` +
-      `<label for="${idPrefix}options-${settingId}" data-l10n-id="${l10nId}"></label>` +
-    `</p>`;
-
-  const eejsBlock_mySettings = (hookName, args, cb) => {
-    args.content += renderCheckbox('');
-    return cb();
-  };
-
-  const eejsBlock_padSettings = (hookName, args, cb) => {
-    if (!padOptionsPluginPassthrough) return cb();
-    args.content += renderCheckbox('padsettings-');
-    return cb();
-  };
-
-  // ---------- Client-side state (closed over by init/handleClientMessage) ----------
 
   let onChangeCallback = () => {};
   let lastEffective = null;
 
   const getPad = () => {
     if (typeof window === 'undefined') return null;
-    // Try the AMD pad module first (preferred), then the global.
     try {
       // eslint-disable-next-line global-require
       const m = require('ep_etherpad-lite/static/js/pad');
@@ -148,12 +92,12 @@ const padToggle = (config) => {
   const getEffective = () => {
     if (isEnforced()) {
       const padVal = readPadValue();
-      return padVal != null ? padVal : cachedDefaultEnabled;
+      return padVal != null ? padVal : defaultEnabled;
     }
     const userVal = readUserValue();
     if (userVal != null) return userVal;
     const padVal = readPadValue();
-    return padVal != null ? padVal : cachedDefaultEnabled;
+    return padVal != null ? padVal : defaultEnabled;
   };
 
   const refreshUI = () => {
@@ -181,9 +125,6 @@ const padToggle = (config) => {
     const $u = window.$(`#${userCheckboxId}`);
     const $p = window.$(`#${padCheckboxId}`);
 
-    // User-side checkbox: cookie-backed, mirrors the effective value when not
-    // enforced. Disabled visually + functionally when the pad creator has
-    // turned on enforceSettings.
     if ($u.length) {
       $u.prop('checked', getEffective());
       $u.prop('disabled', isEnforced());
@@ -198,9 +139,6 @@ const padToggle = (config) => {
       });
     }
 
-    // Pad-wide checkbox: only present when the rendering hook ran (i.e. the
-    // server has the passthrough patch). changePadOption broadcasts and the
-    // local applyPadSettings updates pad.padOptions[pluginName] in-place.
     if ($p.length && pad && typeof pad.changePadOption === 'function') {
       const initial = readPadValue();
       if (initial != null) $p.prop('checked', initial);
@@ -210,9 +148,6 @@ const padToggle = (config) => {
         refreshUI();
       });
     } else if (!isSupportedClient()) {
-      // Surface the degraded state once per pad load so the operator notices
-      // when their core lacks the passthrough patch but plugin authors expect
-      // pad-wide behavior.
       if (typeof console !== 'undefined' && !init._warned) {
         console.warn(
             `[ep_plugin_helpers.padToggle ${pluginName}] pad-wide settings ` +
@@ -231,24 +166,17 @@ const padToggle = (config) => {
     };
   };
 
-  // Etherpad dispatches handleClientMessage_<type> for every incoming
-  // COLLABROOM message. For pad-wide changes, the outer type is
-  // CLIENT_MESSAGE and the inner payload.type is padoptions. Plugins
-  // re-export this hook so the helper can refresh local state when another
-  // user toggles the pad-wide value.
+  // Plugin re-exports this so the helper sees pad-wide broadcasts and
+  // refreshes local state when another user toggles the pad-wide checkbox.
+  // Etherpad dispatches handleClientMessage_<type> for every COLLABROOM
+  // message; for pad-wide changes the outer type is CLIENT_MESSAGE and the
+  // inner payload.type is padoptions.
   const handleClientMessage_CLIENT_MESSAGE = (hookName, ctx) => {
     if (!ctx || !ctx.payload) return;
     if (ctx.payload.type === 'padoptions') refreshUI();
   };
 
-  return {
-    loadSettings,
-    clientVars,
-    eejsBlock_mySettings,
-    eejsBlock_padSettings,
-    init,
-    handleClientMessage_CLIENT_MESSAGE,
-  };
+  return {init, handleClientMessage_CLIENT_MESSAGE};
 };
 
-module.exports = {padToggle, createPadToggle: padToggle};
+module.exports = {padToggle: padToggleClient, createPadToggle: padToggleClient};
diff --git a/test/pad-toggle.js b/test/pad-toggle.js
@@ -1,7 +1,7 @@
 'use strict';
 
 const assert = require('assert');
-const {padToggle} = require('../pad-toggle');
+const {padToggle} = require('../pad-toggle-server');
 
 const baseConfig = () => ({
   pluginName: 'ep_test',
@@ -27,16 +27,22 @@ describe('padToggle', () => {
           /l10nId/);
     });
 
-    it('returns the full hook surface for valid config', () => {
+    it('returns the full server hook surface for valid config', () => {
       const t = padToggle(baseConfig());
       for (const k of [
         'loadSettings', 'clientVars',
         'eejsBlock_mySettings', 'eejsBlock_padSettings',
-        'init', 'handleClientMessage_CLIENT_MESSAGE',
       ]) {
         assert.strictEqual(typeof t[k], 'function', `missing hook: ${k}`);
       }
     });
+
+    it('client sub-path exposes init + handleClientMessage_CLIENT_MESSAGE', () => {
+      const {padToggle: clientFactory} = require('../pad-toggle');
+      const t = clientFactory(baseConfig());
+      assert.strictEqual(typeof t.init, 'function');
+      assert.strictEqual(typeof t.handleClientMessage_CLIENT_MESSAGE, 'function');
+    });
   });
 
   describe('eejsBlock_mySettings', () => {
@@ -123,9 +129,9 @@ describe('padToggle', () => {
   });
 
   describe('backwards-compat alias', () => {
-    it('createPadToggle still resolves', () => {
-      const {createPadToggle} = require('../pad-toggle');
-      assert.strictEqual(typeof createPadToggle, 'function');
+    it('createPadToggle resolves on both sub-paths', () => {
+      assert.strictEqual(typeof require('../pad-toggle-server').createPadToggle, 'function');
+      assert.strictEqual(typeof require('../pad-toggle').createPadToggle, 'function');
     });
   });
 });
