fix(build): debounce webpack watch mode to avoid mid-write reads (#78)

build/test/webpack-sdk-factory.test.cjs

@@ -175,6 +175,126 @@ test('StripBundlePostprocessPlugin: strips webpack\'s bootstrap "use strict" fro
     }
 });
 
+test('resolveWatchAggregateTimeout: falls back to the default when unset', async () => {
+    const { resolveWatchAggregateTimeout } = await import(
+        url.pathToFileURL(path.join(__dirname, '..', 'webpack.sdk.factory.mjs'))
+    );
+    assert.equal(resolveWatchAggregateTimeout(undefined), 500);
+    assert.equal(resolveWatchAggregateTimeout(''), 500);
+});
+
+test('resolveWatchAggregateTimeout: honors an explicit 0 (no debounce) instead of treating it as unset', async () => {
+    const { resolveWatchAggregateTimeout } = await import(
+        url.pathToFileURL(path.join(__dirname, '..', 'webpack.sdk.factory.mjs'))
+    );
+    // `Number(x) || 500` would wrongly collapse "0" back to the default since
+    // 0 is falsy — this is the regression the helper guards against.
+    assert.equal(resolveWatchAggregateTimeout('0'), 0);
+});
+
+test('resolveWatchAggregateTimeout: rejects a non-numeric override instead of silently falling back', async () => {
+    const { resolveWatchAggregateTimeout } = await import(
+        url.pathToFileURL(path.join(__dirname, '..', 'webpack.sdk.factory.mjs'))
+    );
+    assert.throws(() => resolveWatchAggregateTimeout('not-a-number'), /must be a non-negative number/);
+    assert.throws(() => resolveWatchAggregateTimeout('-100'), /must be a non-negative number/);
+});
+
+test('sdkConfig: never throws on a bad WATCH_AGGREGATE_TIMEOUT — one-shot builds must not fail over a watch-only setting', async () => {
+    const { sdkConfig } = await import(
+        url.pathToFileURL(path.join(__dirname, '..', 'webpack.sdk.factory.mjs'))
+    );
+
+    const tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'sdk-watch-options-bad-env-test-'));
+    const prevBuildRoot = process.env.BUILD_ROOT;
+    const prevCacheDir  = process.env.WEBPACK_CACHE_DIR;
+    const prevTimeout   = process.env.WATCH_AGGREGATE_TIMEOUT;
+    const prevWarn      = console.warn;
+    process.env.BUILD_ROOT              = tmpRoot;
+    process.env.WEBPACK_CACHE_DIR       = path.join(tmpRoot, '.webpack-cache');
+    process.env.WATCH_AGGREGATE_TIMEOUT = 'not-a-number';
+    let warned = false;
+    console.warn = () => { warned = true; };
+
+    try {
+        // sdkConfig() is called for plain `npm run build` too, where
+        // watchOptions is inert — a stray/typo'd env var must never crash
+        // that path (see resolveWatchAggregateTimeout's own throwing tests
+        // above for the input-validation contract itself).
+        const [minConfig, allConfig] = sdkConfig('word');
+        for (const config of [minConfig, allConfig]) {
+            assert.equal(config.watchOptions.aggregateTimeout, 500, `${config.name}: should fall back to the default on invalid input`);
+        }
+        assert.equal(warned, true, 'expected a console.warn about the invalid value');
+    } finally {
+        console.warn = prevWarn;
+        if (prevBuildRoot === undefined) delete process.env.BUILD_ROOT; else process.env.BUILD_ROOT = prevBuildRoot;
+        if (prevCacheDir  === undefined) delete process.env.WEBPACK_CACHE_DIR; else process.env.WEBPACK_CACHE_DIR = prevCacheDir;
+        if (prevTimeout   === undefined) delete process.env.WATCH_AGGREGATE_TIMEOUT; else process.env.WATCH_AGGREGATE_TIMEOUT = prevTimeout;
+        fs.rmSync(tmpRoot, { recursive: true, force: true });
+    }
+});
+
+test('sdkConfig: sets a watchOptions.aggregateTimeout above webpack\'s 20ms default on every chunk config', async () => {
+    const { sdkConfig } = await import(
+        url.pathToFileURL(path.join(__dirname, '..', 'webpack.sdk.factory.mjs'))
+    );
+
+    const tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'sdk-watch-options-test-'));
+    const prevBuildRoot = process.env.BUILD_ROOT;
+    const prevCacheDir  = process.env.WEBPACK_CACHE_DIR;
+    process.env.BUILD_ROOT        = tmpRoot;
+    process.env.WEBPACK_CACHE_DIR = path.join(tmpRoot, '.webpack-cache');
+
+    try {
+        const [minConfig, allConfig] = sdkConfig('word');
+
+        // webpack 5's own default (see node_modules/webpack/lib/Watching.js) is
+        // 20ms — a race window too short to survive a multi-event editor save
+        // (see issue #78). Both chunk configs must opt into a longer debounce;
+        // watchOptions is a no-op for one-shot (non --watch) builds, so setting
+        // it unconditionally is safe for production/CI builds too.
+        for (const config of [minConfig, allConfig]) {
+            assert.ok(config.watchOptions, `${config.name}: missing watchOptions`);
+            assert.equal(config.watchOptions.aggregateTimeout, 500, `${config.name}: default aggregateTimeout should be 500`);
+            assert.ok(
+                config.watchOptions.aggregateTimeout > 20,
+                `${config.name}: aggregateTimeout (${config.watchOptions.aggregateTimeout}) must exceed webpack's 20ms default`
+            );
+        }
+    } finally {
+        if (prevBuildRoot === undefined) delete process.env.BUILD_ROOT; else process.env.BUILD_ROOT = prevBuildRoot;
+        if (prevCacheDir  === undefined) delete process.env.WEBPACK_CACHE_DIR; else process.env.WEBPACK_CACHE_DIR = prevCacheDir;
+        fs.rmSync(tmpRoot, { recursive: true, force: true });
+    }
+});
+
+test('sdkConfig: WATCH_AGGREGATE_TIMEOUT overrides the default watch debounce', async () => {
+    const { sdkConfig } = await import(
+        url.pathToFileURL(path.join(__dirname, '..', 'webpack.sdk.factory.mjs'))
+    );
+
+    const tmpRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'sdk-watch-options-override-test-'));
+    const prevBuildRoot = process.env.BUILD_ROOT;
+    const prevCacheDir  = process.env.WEBPACK_CACHE_DIR;
+    const prevTimeout   = process.env.WATCH_AGGREGATE_TIMEOUT;
+    process.env.BUILD_ROOT             = tmpRoot;
+    process.env.WEBPACK_CACHE_DIR      = path.join(tmpRoot, '.webpack-cache');
+    process.env.WATCH_AGGREGATE_TIMEOUT = '1200';
+
+    try {
+        const [minConfig, allConfig] = sdkConfig('word');
+        for (const config of [minConfig, allConfig]) {
+            assert.equal(config.watchOptions.aggregateTimeout, 1200, `${config.name}: WATCH_AGGREGATE_TIMEOUT override not applied`);
+        }
+    } finally {
+        if (prevBuildRoot === undefined) delete process.env.BUILD_ROOT; else process.env.BUILD_ROOT = prevBuildRoot;
+        if (prevCacheDir  === undefined) delete process.env.WEBPACK_CACHE_DIR; else process.env.WEBPACK_CACHE_DIR = prevCacheDir;
+        if (prevTimeout   === undefined) delete process.env.WATCH_AGGREGATE_TIMEOUT; else process.env.WATCH_AGGREGATE_TIMEOUT = prevTimeout;
+        fs.rmSync(tmpRoot, { recursive: true, force: true });
+    }
+});
+
 test('StripBundlePostprocessPlugin: strips the @@license-banner@@ sentinel after Terser has used it to keep the banner', async () => {
     const { StripBundlePostprocessPlugin } = await import(
         url.pathToFileURL(path.join(__dirname, '..', 'webpack.sdk.factory.mjs'))

build/webpack.sdk.factory.mjs

@@ -29,6 +29,9 @@
  *   SDK_SOURCE_MAPS   '1' to emit source maps for a production build too
  *                      (development builds always get them regardless)
  *   WEBPACK_CACHE_DIR override filesystem cache location; defaults to build/.webpack-cache
+ *   WATCH_AGGREGATE_TIMEOUT
+ *                      override watch-mode debounce (ms); defaults to 500,
+ *                      only takes effect under `--watch`
  */
 
 import webpack    from 'webpack';
@@ -133,6 +136,20 @@ export class StripBundlePostprocessPlugin {
     }
 }
 
+// `Number(x) || 500` would silently treat WATCH_AGGREGATE_TIMEOUT=0 (a valid,
+// if unusual, "no debounce" choice) the same as an unset/typo'd value, and a
+// non-numeric typo would silently fall back to the default instead of
+// surfacing the mistake. Exported for the unit test below.
+export function resolveWatchAggregateTimeout(rawValue, fallback = 500) {
+    if (rawValue === undefined || rawValue === '') return fallback;
+
+    const parsed = Number(rawValue);
+    if (!Number.isFinite(parsed) || parsed < 0) {
+        throw new Error(`WATCH_AGGREGATE_TIMEOUT must be a non-negative number, got: ${JSON.stringify(rawValue)}`);
+    }
+    return parsed;
+}
+
 /**
  * @param {string} moduleName  'word' | 'cell' | 'slide' | 'visio'
  * @returns {object[]}  Two webpack compiler configs: [sdk-all-min, sdk-all]
@@ -167,6 +184,22 @@ export function sdkConfig(moduleName) {
     const appCopyright = process.env.APP_COPYRIGHT || defaultAppCopyright();
     const publisherUrl = process.env.PUBLISHER_URL || DEFAULT_PUBLISHER_URL;
 
+    // watchOptions is a documented no-op for one-shot builds (see chunkConfig()
+    // below), but sdkConfig() has no way to know at this point whether the
+    // caller is about to run under `--watch` or not — so a bad
+    // WATCH_AGGREGATE_TIMEOUT must never hard-fail a plain `npm run build`
+    // over a setting that build doesn't even use. Warn and fall back instead
+    // of propagating resolveWatchAggregateTimeout()'s throw (which stays a
+    // throw for its own unit tests, where the bad-input contract is exactly
+    // what's being verified).
+    let watchAggregateTimeout;
+    try {
+        watchAggregateTimeout = resolveWatchAggregateTimeout(process.env.WATCH_AGGREGATE_TIMEOUT);
+    } catch (err) {
+        console.warn(`sdkConfig: ${err.message} — falling back to the default (500ms)`);
+        watchAggregateTimeout = 500;
+    }
+
     // Sentinel deliberately kept in (stripSentinel: false, the default) — Terser's
     // format.comments regex below matches on it, and StripBundlePostprocessPlugin
     // strips it from the asset only after that Terser pass runs.
@@ -381,6 +414,31 @@ export function sdkConfig(moduleName) {
             },
 
             devtool: emitSourceMaps ? 'source-map' : false,
+
+            // Only takes effect under `--watch` (npm run watch:*); ignored for
+            // one-shot builds. webpack 5's default aggregateTimeout is 20ms
+            // (see Watching.js) — too short for an editor's save to fully land
+            // on disk before the loader re-reads it. VS Code (and other
+            // editors) can flush a single save as more than one MODIFY event,
+            // especially over a 9p/WSL2 filesystem boundary; the first event
+            // triggers a rebuild while sdk-concat-loader is re-concatenating
+            // ~400+ source files, and it can catch this file's write
+            // mid-flight, truncated. That produces the intermittent tiny
+            // sdk-all.js / parse-error rebuild described in issue #78, always
+            // followed by a second, correct rebuild once the write settles.
+            //
+            // This only debounces the *trigger* — sdk-concat.cjs still does a
+            // plain readFile with no size/mtime stability check once the
+            // timeout fires, so it reduces the race window (20ms -> 500ms of
+            // required quiescence) rather than closing it structurally. A
+            // write that's still landing after 500ms of silence (slow disk,
+            // network mount) could still be read mid-flight. Overridable via
+            // WATCH_AGGREGATE_TIMEOUT, matching this file's other env-tunable
+            // knobs, in case 500ms proves insufficient on a given machine/CI
+            // watcher.
+            watchOptions: {
+                aggregateTimeout: watchAggregateTimeout,
+            },
         };
     }