🐛 Bug Report — Circular `require()` not supported

[mrbbot]

Hey! 👋 I'm sure you're already aware of this, but want to make sure it doesn't get lost. workerd's require() doesn't currently support cycles. This is something that came up when building a Vitest pool for Workers, as one of Vitest's dependencies chai, includes these. We're working around this by bundling chai with esbuild before returning it from the module fallback service, but ideally we wouldn't need to. For a simple example of something that doesn't work, but does in Node:

using Workerd = import "/workerd/workerd.capnp";

const config :Workerd.Config = (
  services = [
    ( name = "main", worker = .worker ),
  ],
  sockets = [
    ( name = "http", address = "*:8080", http = (), service = "main" ),
  ]
);

const worker :Workerd.Worker = (
  modules = [
    ( name = "index.mjs",
      esModule =
        `import a from "./a.cjs";
        `export default {
        `  async fetch(request, env, ctx) {
        `    return new Response(String(a.three()));
        `  }
        `}
    ),
    ( name = "a.cjs",
      commonJsModule =
        `const b = require("./b.cjs");
        `exports.one = () => 1;
        `exports.three = () => 1 + b.two();
    ),
    ( name = "b.cjs",
      commonJsModule =
        `const a = require("./a.cjs");
        `exports.two = () => a.one() + a.one();
    )
  ],
  compatibilityDate = "2023-02-28",
);

[kentonv]

Wait, how could cyclic require() possibly work, considering the required module is supposed to be evaluated before require() returns?

[mrbbot]

I don't think CommonJS modules have to be fully evaluated. The module context is stored in a require cache, and that's what gets returned. Something like...

import path from "node:path";

// Would normally be loaded from disk and wrapped with function
function aModule(module, exports, require) {
  const b = require("./b.cjs");
  exports.one = () => 1;
  exports.three = () => 1 + b.two();
}
function bModule(module, exports, require) {
  const a = require("./a.cjs");
  exports.two = () => a.one() + a.one();
}
const moduleFiles = new Map([
  ["/a.cjs", aModule],
  ["/b.cjs", bModule],
]);

const cache = new Map();
function createRequire(refererDir) {
  return function (specifier) {
    const target = path.resolve(refererDir, specifier);

    // Return existing instance, even if not fully evaluated yet
    const cached = cache.get(target);
    if (cached !== undefined) return cached.exports;

    const moduleFile = moduleFiles.get(target);
    if (moduleFile === undefined) throw new Error(`${target} not found`);

    const module = { exports: {} };
    cache.set(target, module);

    const require = createRequire(path.dirname(target));
    moduleFile(module, module.exports, require);

    return module.exports;
  }
}

const rootRequire = createRequire("/");
const a = rootRequire("./a.cjs");
console.log(a.three());

[jasnell]

I don't think CommonJS modules have to be fully evaluated...

That's correct, tho if we had been able to do it again from the beginning we probably would have locked this down more as there can be some weird side effects of allowing cycles. Cycles need to be carefully handled and a lot of the time folks get it wrong. But, there are times when it's just not avoidable.

james@DESKTOP-38UI0E9:~/tmp$ cat a.js
const b = require('./b.js')
exports.xyz = 1;
console.log(exports.xyz)
james@DESKTOP-38UI0E9:~/tmp$ cat b.js
const a = require('./a.js')
exports.zzz = 123
console.log(a);;
james@DESKTOP-38UI0E9:~/tmp$ node a
{}
1
(node:734718) Warning: Accessing non-existent property 'Symbol(nodejs.util.inspect.custom)' of module exports inside circular dependency
(Use `node --trace-warnings ...` to show where the warning was created)
(node:734718) Warning: Accessing non-existent property 'constructor' of module exports inside circular dependency
(node:734718) Warning: Accessing non-existent property 'Symbol(Symbol.toStringTag)' of module exports inside circular dependency
james@DESKTOP-38UI0E9:~/tmp$ node b
1
{ xyz: 1 }
james@DESKTOP-38UI0E9:~/tmp$