Skip to content

Commit

Permalink
Fix a bug where transitively required modules can't change paths betw…
Browse files Browse the repository at this point in the history
…een requirefire instances

requirefire intentionally re-creates the entire require stack such that when used, its as if the returned `require` function is from a totally different process. Two requirefire instances should be totally independent, with different require caches. Crucially, if the files on disk change between the creation of one instance and the next, the second instance should reflect those changes.

Before this change, that wasn't the case in a particular circumstance that burned us in Gadget. Specifically, if a module was required by require fire, and that module required another module from it's node_modules directory, subsequent requirefire instances requiring the entrypoint module will always get the same copy of the node_modules module, regardless of if it changes.

An example:

 - const a = requirefire();
 - a("foo") requires "@gadgetinc/api-client-core" from foo's node_modules, gets one copy of api-client-core
 - we change api-client-core in the node_modules on disk
 - const b = requirefire();
 - b("foo") requires "@gadgetinc/api-client-core" again, should return the new copy, but didn't.

The reason the old copy was returned by new instances of requirefire is that there was another cache inside the node internals that all requirefire instances were sharing by accident: `Module._pathCache`.

The path cache is used by node to cache resolutions from entrypoints to files on disk. Usually in node the path cache is only ever filled and never emptied, and thus node will never pick up new or changed resolutions after the first time they are run. We explicitly want the opposite with requirefire, so we need to change to using one path cache per requirefire instance instead of one shared one.

Super regrettably, this shared global cache is baked in deep to node. I couldn't figure out any way to re-use node's internal logic while still changing the semantics of which path cache is used, so I had to skip node's resolve stuff that uses that cache entirely. Luckly, there's a great userland re-implementation of resolving in webpack that we can use, that has great support for ... everything. As webpack would need!
  • Loading branch information
airhorns committed Sep 20, 2023
1 parent fab3382 commit 2b47171
Show file tree
Hide file tree
Showing 14 changed files with 423 additions and 89 deletions.
4 changes: 4 additions & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -48,5 +48,9 @@
"npm": "^10.1.0",
"prettier": "^2.8.8",
"typescript": "^5.2.2"
},
"dependencies": {
"enhanced-resolve": "^5.15.0",
"is-builtin-module": "^3.2.1"
}
}
156 changes: 155 additions & 1 deletion pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

115 changes: 115 additions & 0 deletions spec/behaviour-matching.spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,115 @@
import execa from "execa";
import fs from "fs-extra";
import os from "os";
import path from "path";
import requirefire from "../src";

describe("requirefire vs require behaviour matching", () => {
describe.each([
["require", require],
["requirefire", requirefire()],
])("%s", (name, _require) => {
test("should require modules", () => {
const a = _require("./fixtures/mod_a");
const b = _require("./fixtures/mod_b");
expect(a.name).toEqual("a");
expect(b.name).toEqual("b");
});

test("the same instance return cached versions of the same module when required twice", () => {
const one = _require("./fixtures/mod_a");
const two = _require("./fixtures/mod_a");
expect(one).toBe(two);
});

test("transitive requires are required through requirefire", () => {
jest.isolateModules(() => {
const outer = _require("./fixtures/outer_transitive");
const inner = _require("./fixtures/inner_transitive");
expect(outer.inner.random).toEqual(inner.random);
});
});

test("transitive requires have require extensions and resolve", () => {
const outer = _require("./fixtures/outer_transitive");
expect(outer.inner.requireKeys).toContain("cache");
expect(outer.inner.requireKeys).toContain("extensions");
expect(outer.inner.requireKeys).toContain("resolve");
});

test("transitive requires can resolve correctly", () => {
const outer = _require("./fixtures/outer_transitive");
expect(outer.inner.outerTransitiveResolve).toEqual(path.resolve(__dirname, "fixtures/outer_transitive.js"));
});

test("non-existent filepath modules throw a module not found error", () => {
try {
_require("/tmp/does-not-exist-requirefire-test.js");
} catch (error: any) {
expect(error).toBeTruthy();
expect(error.code).toEqual("MODULE_NOT_FOUND");
expect(error.message).toContain("Cannot find module '/tmp/does-not-exist-requirefire-test.js'");
return;
}
// unreachable
expect(false).toBe(true);
});

test("non-existent node_modules modules throw a module not found error", () => {
try {
_require("a-magical-package-that-does-everything");
} catch (error: any) {
expect(error).toBeTruthy();
expect(error.code).toEqual("MODULE_NOT_FOUND");
expect(error.message).toContain("Cannot find module 'a-magical-package-that-does-everything'");
return;
}
// unreachable
expect(false).toBe(true);
});

test("mutual (circular) requires can be required", () => {
const a = _require("./fixtures/mutual_a");
const b = _require("./fixtures/mutual_b");
expect(a.getB()).toBe(b);
expect(b.getA()).toBe(a);
});

test("modules without newlines at the end can be required", () => {
_require("./fixtures/no-newline");
});

test("aliased modules that resolve to the same module should resolve to the same module if cached", () => {
const linked = _require("./fixtures/linked_module");
linked.foo = "not foo";
const { linked: outerLinked } = _require("./fixtures/outer_linked_module");

expect(linked).toBe(outerLinked);
});

test("packages with combo esm/cjs exports configured can be requirefired", async () => {
const modDir = fs.mkdtempSync(path.join(os.tmpdir(), "requirefire-"));
await fs.rm(modDir, { recursive: true, force: true });
await fs.mkdir(modDir);

await fs.writeFile(path.join(modDir, "index.js"), `module.exports = require('dualexports')`);
await fs.writeFile(
path.join(modDir, "package.json"),
JSON.stringify({
name: "parent",
version: "0.1.0",
dependencies: {
dualexports: `file:${path.resolve(path.join(__dirname, "fixtures", "dualexports"))}`,
},
})
);

await execa("npm", ["install"], { cwd: modDir });

jest.isolateModules(() => {
const mod = _require(modDir);
expect(mod.foo).toEqual("bar");
});
});
});
});
Loading

0 comments on commit 2b47171

Please sign in to comment.