Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix an issue where hot reload did not work unless the code is under a package with type module #783

Merged
merged 6 commits into from Mar 9, 2024
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/dull-seahorses-push.md
@@ -0,0 +1,5 @@
---
"counterfact": patch
---

fix issue where hot reload did not work unless the code is in a package with type: module"
2 changes: 1 addition & 1 deletion package.json
Expand Up @@ -31,7 +31,7 @@
"test": "yarn node --experimental-vm-modules ./node_modules/jest-cli/bin/jest --testPathIgnorePatterns=black-box",
"test:black-box": "rimraf dist && rimraf out && yarn build && yarn node --experimental-vm-modules ./node_modules/jest-cli/bin/jest black-box --forceExit --coverage=false",
"test:mutants": "stryker run stryker.config.json",
"build": "tsc && copyfiles -f \"src/client/**\" dist/client && copyfiles -f \"src/server/*.d.ts\" dist/server",
"build": "tsc && copyfiles -f \"src/client/**\" dist/client && copyfiles -f \"src/server/*.d.ts\" dist/server && copyfiles -f \"src/server/*.cjs\" dist/server",
"prepack": "yarn build",
"release": "npx changeset publish",
"prepare": "husky install",
Expand Down
45 changes: 24 additions & 21 deletions src/server/module-loader.ts
Expand Up @@ -2,13 +2,16 @@ import { once } from "node:events";
import { type Dirent, existsSync } from "node:fs";
import fs from "node:fs/promises";
import nodePath from "node:path";
import { pathToFileURL } from "node:url";

import { type FSWatcher, watch } from "chokidar";
import createDebug from "debug";

import { type Context, ContextRegistry } from "./context-registry.js";
import { determineModuleKind } from "./determine-module-kind.js";
import type { Module, Registry } from "./registry.js";
import { uncachedImport } from "./uncached-import.js";

const { uncachedRequire } = await import("./uncached-require.cjs");

const debug = createDebug("counterfact:typescript-generator:module-loader");

Expand Down Expand Up @@ -43,6 +46,12 @@ export class ModuleLoader extends EventTarget {

private readonly contextRegistry: ContextRegistry;

private uncachedImport: (moduleName: string) => Promise<unknown> =
// eslint-disable-next-line @typescript-eslint/require-await
async function (moduleName: string) {
throw new Error(`uncachedImport not set up; importing ${moduleName}`);
};

public constructor(
basePath: string,
registry: Registry,
Expand Down Expand Up @@ -82,16 +91,10 @@ export class ModuleLoader extends EventTarget {
this.dispatchEvent(new Event("remove"));
}

const fileUrl = `${pathToFileURL(
pathName,
).toString()}?cacheBust=${Date.now()}`;

debug("importing module: %s", fileUrl);

// eslint-disable-next-line import/no-dynamic-require, no-unsanitized/method
import(fileUrl)
debug("importing module: %s", pathName);
this.uncachedImport(pathName)
// eslint-disable-next-line promise/prefer-await-to-then
.then((endpoint: ContextModule | Module) => {
.then((endpoint) => {
this.dispatchEvent(new Event(eventName));

if (pathName.includes("_.context")) {
Expand All @@ -112,7 +115,7 @@ export class ModuleLoader extends EventTarget {
})
// eslint-disable-next-line promise/prefer-await-to-then
.catch((error: unknown) => {
reportLoadError(error, fileUrl);
reportLoadError(error, pathName);
});
},
);
Expand All @@ -124,6 +127,11 @@ export class ModuleLoader extends EventTarget {
}

public async load(directory = ""): Promise<void> {
const moduleKind = await determineModuleKind(this.basePath);

this.uncachedImport =
moduleKind === "module" ? uncachedImport : uncachedRequire;

if (
!existsSync(nodePath.join(this.basePath, directory).replaceAll("\\", "/"))
) {
Expand Down Expand Up @@ -161,21 +169,16 @@ export class ModuleLoader extends EventTarget {
await Promise.all(imports);
}

// eslint-disable-next-line max-statements
private async loadEndpoint(
fullPath: string,
directory: string,
file: Dirent,
) {
const fileUrl = `${pathToFileURL(
fullPath,
).toString()}?cacheBust=${Date.now()}`;

try {
// eslint-disable-next-line import/no-dynamic-require, no-unsanitized/method, @typescript-eslint/consistent-type-assertions
const endpoint: ContextModule | Module = (await import(fileUrl)) as
| ContextModule
| Module;
// eslint-disable-next-line @typescript-eslint/consistent-type-assertions
const endpoint: ContextModule | Module = (await this.uncachedImport(
fullPath,
)) as ContextModule | Module;

if (file.name.includes("_.context")) {
if (isContextModule(endpoint)) {
Expand Down Expand Up @@ -207,7 +210,7 @@ export class ModuleLoader extends EventTarget {
return;
}

process.stdout.write(`\nError loading ${fileUrl}:\n${String(error)}\n`);
process.stdout.write(`\nError loading ${fullPath}:\n${String(error)}\n`);
}
}
}
10 changes: 10 additions & 0 deletions src/server/uncached-import.ts
@@ -0,0 +1,10 @@
import { pathToFileURL } from "node:url";

export async function uncachedImport(pathName: string) {
const fileUrl = `${pathToFileURL(
pathName,
).toString()}?cacheBust=${Date.now()}`;

// eslint-disable-next-line @typescript-eslint/no-unsafe-return, import/no-dynamic-require, no-unsanitized/method
return await import(fileUrl);
}
10 changes: 10 additions & 0 deletions src/server/uncached-require.cjs
@@ -0,0 +1,10 @@
"use strict";

module.exports = {
uncachedRequire: function uncachedRequire(moduleName) {
delete require.cache[require.resolve(moduleName)];

// eslint-disable-next-line n/global-require, security/detect-non-literal-require, import/no-dynamic-require
return Promise.resolve(require(moduleName));
},
};
36 changes: 22 additions & 14 deletions test/server/module-loader.test.ts
Expand Up @@ -8,20 +8,22 @@ import { withTemporaryFiles } from "../lib/with-temporary-files.js";
describe("a module loader", () => {
it("finds a file and adds it to the registry", async () => {
const files: { [fileName: string]: string } = {
"a/b/c.mjs": `
"a/b/c.js": `
export function GET() {
return {
body: "GET from a/b/c"
};
}
`,
"hello.mjs": `

"hello.js": `
export function GET() {
return {
body: "hello"
};
}
`,
"package.json": '{ "type": "module" }',
};

await withTemporaryFiles(files, async (basePath: string) => {
Expand All @@ -39,7 +41,7 @@ describe("a module loader", () => {

it("updates the registry when a file is added", async () => {
await withTemporaryFiles(
{},
{ "package.json": '{ "type": "module" }' },
async (
basePath: string,
{ add }: { add: (path: string, content: string) => void },
Expand All @@ -53,7 +55,7 @@ describe("a module loader", () => {
expect(registry.exists("GET", "/late/addition")).toBe(false);

add(
"late/addition.mjs",
"late/addition.js",
'export function GET() { return { body: "I\'m here now!" }; }',
);
await once(loader, "add");
Expand All @@ -68,8 +70,9 @@ describe("a module loader", () => {
it("updates the registry when a file is deleted", async () => {
await withTemporaryFiles(
{
"delete-me.mjs":
'export function GET() { return { body: "Goodbye" }; }',
"delete-me.js": 'export function GET() { return { body: "Goodbye" }; }',

"package.json": '{ "type": "module" }',
},
async (
basePath: string,
Expand All @@ -83,7 +86,7 @@ describe("a module loader", () => {

expect(registry.exists("GET", "/delete-me")).toBe(true);

remove("delete-me.mjs");
remove("delete-me.js");
await once(loader, "remove");

expect(registry.exists("GET", "/delete-me")).toBe(false);
Expand All @@ -97,7 +100,8 @@ describe("a module loader", () => {
const contents = 'export function GET() { return { body: "hello" }; }';

const files: { [key: string]: string } = {
"module.mjs": contents,
"module.js": contents,
"package.json": '{"type": "module"}',
"README.md": contents,
};

Expand Down Expand Up @@ -132,8 +136,10 @@ describe("a module loader", () => {
it.skip("updates the registry when a file is changed", async () => {
await withTemporaryFiles(
{
"change.mjs":
"change.js":
'export function GET(): { body } { return { body: "before change" }; }',

"package.json": '{ "type": "module" }',
},
async (
basePath: string,
Expand All @@ -144,7 +150,7 @@ describe("a module loader", () => {

await loader.watch();
add(
"change.mjs",
"change.js",
'export function GET() { return { body: "after change" }; }',
);
await once(loader, "change");
Expand All @@ -166,8 +172,9 @@ describe("a module loader", () => {

it("finds a context and adds it to the context registry", async () => {
const files: { [key: string]: string } = {
"_.context.mjs": 'export class Context { name = "main"};',
"hello/_.context.mjs": 'export class Context { name = "hello"};',
"_.context.js": 'export class Context { name = "main"};',
"hello/_.context.js": 'export class Context { name = "hello"};',
"package.json": '{ "type": "module" }',
};

await withTemporaryFiles(files, async (basePath: string) => {
Expand All @@ -191,8 +198,9 @@ describe("a module loader", () => {

it("provides the parent context if the locale _.context.ts doesn't export a default", async () => {
const files: { [key: string]: string } = {
"_.context.mjs": "export class Context { value = 0 }",
"hello/_.context.mjs": "export class Context { value = 100 }",
"_.context.js": "export class Context { value = 0 }",
"hello/_.context.js": "export class Context { value = 100 }",
"package.json": '{ "type": "module" }',
};

await withTemporaryFiles(files, async (basePath: string) => {
Expand Down