[fix] preserve isolation of minified require bundles

CHANGELOG.md

@@ -4,6 +4,17 @@
 
 ### Build
 
+- The minified module bundle `dist/sd-expression-engine.min.js` keeps the
+  state of its `require` wrapper (module cache, factories, previous `require`)
+  in its own closure: `@rollup/plugin-terser` defaults to `toplevel: true` for
+  the cjs output and turned the wrapper into global variables, so two minified
+  module bundles in one scope overwrote each other (`Maximum call stack size
+  exceeded`, `r is not a function`). Terser now runs with `toplevel: false`;
+  the plain and standalone files are unchanged.
+  `test/contract/bundle-isolation.test.js` checks the globals, the cache and
+  the `require` chain of the module bundle. `npm run test:bundle-set` loads
+  the module bundles of sd-utils, sd-random, sd-model and this package
+  together in all load orders (`test/bundle-set`).
 - The `dist/` bundles are built with Rollup and npm scripts instead of
   gulp/browserify (`npm run build`). Same files, formats and globals:
   `dist/sd-expression-engine(.min).js` defines the global `require` exposing

README.md

@@ -93,12 +93,26 @@ artifact, chosen with `SD_EXPRESSION_ENGINE_TARGET`:
 
 `SD_EXPRESSION_ENGINE_DIST=<dir>` tests the bundles of another `dist/` directory.
 
+For the `module` targets `test/contract/bundle-isolation.test.js` loads the module bundle as a script into a fresh
+global scope and checks that it adds only `require`, keeps its exports and
+cache when another `require` bundle is loaded after it, passes other names
+along the `require` chain (`MODULE_NOT_FOUND` for unknown ones) and evaluates
+the module on the first `require`.
+`npm run test:bundle-set` (`test/bundle-set`) loads the module bundles of
+sd-utils, sd-random, sd-model and this package, plain and minified, into one
+global scope in all load orders and uses each library; `--sd-utils=`,
+`--sd-random=`, `--sd-model=`, `--sd-expression-engine=` take a `dist/`
+directory, a tarball or `published[@version]` (defaults: the installed
+sd-utils / sd-random, the published sd-model, `./dist`). `npm run test:build`
+runs it on the fresh build (`--sd-model=<.tgz>` for an unreleased sd-model).
+
 ```bash
 npm test                                   # all five targets on ./dist and the sources, then Jasmine
 SD_EXPRESSION_ENGINE_TARGET=module node --test test/contract/   # one target
 node test/contract/run.js --targets=module,source
 node test/contract/run.js --dist=<dir>     # another dist/ directory
 npm run test:published                     # the bundles of [email protected] from npm
+npm run test:bundle-set                    # the four module bundles in one global scope
 npx jasmine --config=test/jasmine.json     # the Jasmine specs (test/specs) in Node
 ```
 

package.json

@@ -9,6 +9,7 @@
     "test": "node test/contract/run.js && jasmine --config=test/jasmine.json",
     "test:published": "node test/contract/run.js --published=0.1.17",
     "test:build": "node scripts/check-build.js",
+    "test:bundle-set": "node test/bundle-set/run.js",
     "release": "release-it --verbose"
   },
   "repository": {

rollup.config.mjs

@@ -54,7 +54,9 @@ function outputs(file, output) {
     const common = Object.assign({exports: 'auto'}, output);
     return [
         Object.assign({file: file + '.js', sourcemap: 'inline'}, common),
-        Object.assign({file: file + '.min.js', sourcemap: true, plugins: [terser()]}, common)
+        // toplevel: false, as for the other formats: @rollup/plugin-terser turns it on for cjs,
+        // which moves the variables of the require wrapper into the global scope
+        Object.assign({file: file + '.min.js', sourcemap: true, plugins: [terser({toplevel: false})]}, common)
     ];
 }
 

scripts/check-build.js

@@ -12,7 +12,9 @@
 //  5. the contract tests against the four fresh bundles (dist/standalone and
 //     dist/sd-expression-engine.js after dist/sd-utils.js and dist/sd-random.js,
 //     plain and minified) and the sources bundled with browserify (target
-//     `source`), and the Jasmine specs
+//     `source`), the Jasmine specs, and test/bundle-set: the module bundles of
+//     sd-utils, sd-random, sd-model and the fresh ones in one global scope
+//     (installed sd-utils / sd-random, sd-model from --sd-model or the registry)
 //  6. `npm pack`: the tarball contains the entry points, sources and dist; it is
 //     installed into an empty consumer that bundles require('sd-expression-engine')
 //     with browserify + babelify (the package's browserify.transform field, like
@@ -24,11 +26,12 @@
 //     outside dist/ changes
 //
 // Usage: node scripts/check-build.js [--keep] [--skip-consumer]
-//                                    [--sd-utils=<.tgz>] [--sd-random=<.tgz>]
+//                                    [--sd-utils=<.tgz>] [--sd-random=<.tgz>] [--sd-model=<.tgz>]
 //   --sd-utils / --sd-random  install these tarballs in the copy and in the
 //               consumer instead of the locked / published versions (integration
 //               check of unreleased packages); package.json and the lockfile of
 //               the working tree are not changed
+//   --sd-model  the sd-model tarball for test/bundle-set (default: the published one)
 'use strict';
 
 const fs = require('fs');
@@ -234,6 +237,19 @@ function sourceMapProblem(file, minified, projectDir) {
     return null;
 }
 
+function runBundleSet(dir) {
+    const sdModel = option('--sd-model') ? ['--sd-model=' + path.resolve(option('--sd-model'))] : [];
+    const r = run(process.execPath, [path.join('test', 'bundle-set', 'run.js'), '--test-reporter=tap'].concat(sdModel), {cwd: dir});
+    const count = name => (r.log.match(new RegExp('^# ' + name + ' (\\d+)', 'm')) || [])[1];
+    const summary = 'tests ' + count('tests') + ', pass ' + count('pass') + ', fail ' + count('fail');
+    if (r.status === 0 && count('fail') === '0' && Number(count('pass')) > 0) {
+        ok('bundle set (module bundles of sd-utils, sd-random, sd-model and the fresh ones in one scope): ' + summary);
+    } else {
+        console.error(r.log.split('\n').filter(l => /^(sd-|not ok)|unexpected|gave|instead|error/.test(l)).slice(0, 30).join('\n'));
+        fail('bundle set: ' + summary);
+    }
+}
+
 function runTests(dir, target, env) {
     // TAP: the default reporter of Node 24 prints no summary lines to parse
     const files = fs.readdirSync(path.join(dir, 'test', 'contract')).filter(f => f.endsWith('.test.js')).map(f => path.join('test', 'contract', f));
@@ -328,6 +344,7 @@ function main() {
             runTests(copy, target);
         }
         runJasmine(copy);
+        runBundleSet(copy);
 
         // 6 (before 7, which breaks the sources of its own copy)
         if (!flag('--skip-consumer')) {

test/bundle-set/bundle-set.test.js

@@ -0,0 +1,212 @@
+// The module bundles of sd-utils, sd-random, sd-model and sd-expression-engine
+// (dist/<name>.js and dist/<name>.min.js) loaded as classic scripts into one
+// global scope, as <script> tags or importScripts() in the sd-computations job
+// worker load them. Run with `npm run test:bundle-set` (test/bundle-set/run.js
+// chooses the dist directories).
+//
+// Every load order of the four minified files, and every plain/minified mix in
+// two orders: after each file the libraries whose dependencies are loaded are
+// required and used; at the end every library resolves to the same exports
+// through the latest `require` and through the `require` each file defined, the
+// earlier exports still work, names go to the `require` defined before the
+// bundles and to a `require` bundle loaded after them, unknown names throw
+// MODULE_NOT_FOUND, and the global scope gained only `require` (and
+// regeneratorRuntime once the engine is used, see AFTER_USE).
+'use strict';
+
+const test = require('node:test');
+const assert = require('node:assert/strict');
+const fs = require('fs');
+const path = require('path');
+const vm = require('vm');
+
+const LIBS = [
+    {
+        name: 'sd-utils',
+        deps: [],
+        use: 'return JSON.stringify([exports.Utils.isString("a"), exports.Utils.isValidVersionString("1.2.0"), typeof exports.log.getLevel()]);',
+        expected: '[true,true,"number"]'
+    },
+    {
+        name: 'sd-random',
+        deps: [],
+        use: 'return JSON.stringify([exports.Uniform(2, 4)]);',
+        expected: '[3]'
+    },
+    {
+        // serialize() uses Utils of sd-utils
+        name: 'sd-model',
+        deps: ['sd-utils'],
+        use: 'var d = exports.domain, m = new exports.DataModel(); m.addNode(new d.DecisionNode(new d.Point(0, 0))); ' +
+            'return JSON.stringify([JSON.parse(m.serialize(true)).trees.length, m.getRoots().length]);',
+        expected: '[1,1]'
+    },
+    {
+        // Uniform from sd-random, hasAssignmentExpression uses Utils of sd-utils
+        name: 'sd-expression-engine',
+        deps: ['sd-utils', 'sd-random'],
+        use: 'var E = exports.ExpressionEngine; return JSON.stringify([E.eval("1/4 + 1/2", false, {}), ' +
+            'E.eval("Uniform(2, 4)", false, {}), E.hasAssignmentExpression("p = 1")]);',
+        expected: '[0.75,3,true]'
+    }
+];
+// globals besides `require`: Math.js 9.5.0 loads @babel/runtime/regenerator,
+// which sets regeneratorRuntime (sd-expression-engine 0.1.17 does too)
+const AFTER_USE = {'sd-expression-engine': ['regeneratorRuntime']};
+
+const dirs = {};
+for (const lib of LIBS) {
+    const dir = process.env['SD_SET_' + lib.name.toUpperCase().replace(/-/g, '_')];
+    if (dir) {
+        dirs[lib.name] = path.resolve(dir);
+    }
+}
+const missing = LIBS.filter(lib => !dirs[lib.name]).map(lib => lib.name);
+const skip = missing.length ? 'no dist directory for ' + missing.join(', ') + ' (use test/bundle-set/run.js)' : false;
+
+const scripts = new Map();
+function script(name, min) {
+    const file = path.join(dirs[name], name + (min ? '.min' : '') + '.js');
+    if (!scripts.has(file)) {
+        scripts.set(file, new vm.Script(fs.readFileSync(file, 'utf8'), {filename: file}));
+    }
+    return scripts.get(file);
+}
+
+// a `require` defined before the bundles, as browserify defines it
+const BEFORE = `require = function (id, jumped) {
+    __t.calls.push([id, jumped]);
+    if (id === 'iso-before') { return __t.before; }
+    var error = new Error("Cannot find module '" + id + "'");
+    error.code = 'MODULE_NOT_FOUND';
+    throw error;
+};`;
+// a `require` bundle loaded after them
+const AFTER = `require = (function (previous) {
+    function newRequire(id, jumped) {
+        if (id === 'iso-after') { return __t.after; }
+        var current = typeof require === 'function' && require;
+        if (!jumped && current && current !== newRequire) { return current(id, true); }
+        if (previous) { return previous(id, true); }
+        var error = new Error("Cannot find module '" + id + "'");
+        error.code = 'MODULE_NOT_FOUND';
+        throw error;
+    }
+    return newRequire;
+})(typeof require === 'function' && require);`;
+
+const use = (lib, ref) => '(function (exports) {' + lib.use + '})(' + ref + ')';
+
+// Loads the libraries in `order` (minified where min[name]); returns what was observed.
+function scenario(order, min) {
+    const ctx = vm.createContext({});
+    const run = (src) => vm.runInContext(src, ctx);
+    run('Math.random = function () { return 0.5; }; var __t = {calls: [], before: {}, after: {}, got: {}, requireAt: {}};');
+    const names = () => Object.getOwnPropertyNames(run('this'));
+    const initial = names();
+    const added = () => names().filter(n => !initial.includes(n)).sort();
+    run(BEFORE);
+    const problems = [];
+    const allowed = ['require'];
+    const loaded = [];
+    const used = [];
+    for (const name of order) {
+        script(name, min[name]).runInContext(ctx);
+        loaded.push(name);
+        run('__t.requireAt[' + JSON.stringify(name) + '] = require;');
+        for (const lib of LIBS) {
+            if (!used.includes(lib.name) && loaded.includes(lib.name) && lib.deps.every(d => loaded.includes(d))) {
+                let result;
+                try {
+                    result = run('__t.got[' + JSON.stringify(lib.name) + '] = require(' + JSON.stringify(lib.name) + '); ' +
+                        use(lib, '__t.got[' + JSON.stringify(lib.name) + ']'));
+                } catch (e) {
+                    result = 'error: ' + e.message;
+                }
+                if (result !== lib.expected) {
+                    problems.push('after loading ' + name + ': ' + lib.name + ' gave ' + result);
+                }
+                used.push(lib.name);
+                allowed.push(...(AFTER_USE[lib.name] || []));
+            }
+        }
+        const extra = added().filter(n => !allowed.includes(n));
+        if (extra.length) {
+            problems.push('after loading ' + name + ': ' + extra.length + ' unexpected global(s)');
+        }
+    }
+    const check = (what, src) => {
+        let ok;
+        try {
+            ok = run(src);
+        } catch (e) {
+            ok = 'error: ' + e.message;
+        }
+        if (ok !== true) {
+            problems.push(what + (ok === false ? '' : ' (' + ok + ')'));
+        }
+    };
+    const requires = ['require'].concat(order.map(n => '__t.requireAt[' + JSON.stringify(n) + ']'));
+    for (const lib of LIBS) {
+        const got = '__t.got[' + JSON.stringify(lib.name) + ']';
+        check(lib.name + ' resolves to the first exports through every require',
+            requires.map(r => r + '(' + JSON.stringify(lib.name) + ') === ' + got).join(' && '));
+        check(lib.name + ' still works', use(lib, got) + ' === ' + JSON.stringify(lib.expected));
+    }
+    check('iso-before through every require', requires.map(r => r + '("iso-before") === __t.before').join(' && '));
+    check('the require defined before is called with jumped', '__t.calls.every(function (c) { return c[1] === true; }) && __t.calls.length > 0');
+    run(AFTER);
+    check('iso-after through every require of the bundles', requires.slice(1).map(r => r + '("iso-after") === __t.after').join(' && '));
+    for (const lib of LIBS) {
+        check(lib.name + ' through the later require', 'require(' + JSON.stringify(lib.name) + ') === __t.got[' + JSON.stringify(lib.name) + ']');
+    }
+    check('MODULE_NOT_FOUND through every require', requires.map(r => '(function () { try { ' + r + '("iso-missing"); return false; } ' +
+        'catch (e) { return e.code === "MODULE_NOT_FOUND" && e.message === "Cannot find module \'iso-missing\'"; } })()').join(' && '));
+    const expectedGlobals = ['require'].concat(AFTER_USE['sd-expression-engine']).sort();
+    if (JSON.stringify(added()) !== JSON.stringify(expectedGlobals)) {
+        problems.push('globals at the end: ' + added().length + ' instead of ' + expectedGlobals.join(', '));
+    }
+    return problems;
+}
+
+function permutations(list) {
+    if (list.length <= 1) {
+        return [list];
+    }
+    return list.flatMap((x, i) => permutations(list.slice(0, i).concat(list.slice(i + 1))).map(p => [x].concat(p)));
+}
+
+const NAMES = LIBS.map(lib => lib.name);
+const label = (order, min) => order.map(n => n + (min[n] ? '.min' : '')).join(' ');
+
+test('loading all four files adds only `require` to the global scope', {skip}, () => {
+    for (const minified of [false, true]) {
+        const ctx = vm.createContext({});
+        const before = Object.getOwnPropertyNames(vm.runInContext('this', ctx));
+        for (const name of NAMES) {
+            script(name, minified).runInContext(ctx);
+        }
+        const added = Object.getOwnPropertyNames(vm.runInContext('this', ctx)).filter(n => !before.includes(n));
+        assert.deepEqual(added, ['require'], minified ? 'minified' : 'plain');
+    }
+});
+
+test('the 24 load orders of the minified files', {skip}, () => {
+    const all = Object.fromEntries(NAMES.map(n => [n, true]));
+    const failures = permutations(NAMES).map(order => [label(order, all), scenario(order, all)]).filter(([, p]) => p.length);
+    assert.deepEqual(failures, []);
+});
+
+test('the 16 plain/minified mixes, in the dependency order and reversed', {skip}, () => {
+    const failures = [];
+    for (let mask = 0; mask < 16; mask++) {
+        const min = Object.fromEntries(NAMES.map((n, i) => [n, Boolean(mask & (1 << i))]));
+        for (const order of [NAMES, NAMES.slice().reverse()]) {
+            const problems = scenario(order, min);
+            if (problems.length) {
+                failures.push([label(order, min), problems]);
+            }
+        }
+    }
+    assert.deepEqual(failures, []);
+});