fix react import (#1229)

* fix node imports

* prettier

* add missing mockJsDelivr

* log resolveNodeImport error

* fix react import

* more fixes

* more fixes

* fix input; tsc

* more consistent promising?

* more test isolation

docs/lib/react.md

@@ -0,0 +1,52 @@
+# React
+
+```js echo
+import {createRoot} from "react-dom/client";
+
+const root = createRoot(display(document.createElement("DIV")));
+```
+
+The code above creates a root; a place for React content to live. We render some content into the root below.
+
+```js echo
+root.render(jsxs(createContent, {}));
+```
+
+The content is defined as a component, but hand-authored using the JSX runtime. You wouldn’t normally write this code by hand, but Framework doesn’t support JSX yet. We’re working on it.
+
+```js echo
+import {useState} from "react";
+import {Fragment, jsx, jsxs} from "react/jsx-runtime";
+
+function createContent() {
+  const [counter, setCounter] = useState(0);
+  return jsxs(Fragment, {
+    children: [
+      jsx("p", {
+        children: ["Hello, world! ", counter]
+      }),
+      "\n",
+      jsx("p", {
+        children: "This content is rendered by React."
+      }),
+      "\n",
+      jsx("div", {
+        style: {
+          backgroundColor: "indigo",
+          padding: "1rem"
+        },
+        onClick: () => setCounter(counter + 1),
+        children: jsxs("p", {
+          children: [
+            "Try changing the background color to ",
+            jsx("code", {
+              children: "tomato"
+            }),
+            "."
+          ]
+        })
+      })
+    ]
+  });
+}
+```

package.json

@@ -56,7 +56,9 @@
     "@clack/prompts": "^0.7.0",
     "@observablehq/inputs": "^0.10.6",
     "@observablehq/runtime": "^5.9.4",
+    "@rollup/plugin-commonjs": "^25.0.7",
     "@rollup/plugin-node-resolve": "^15.2.3",
+    "@rollup/plugin-virtual": "^3.0.2",
     "acorn": "^8.11.2",
     "acorn-walk": "^8.3.0",
     "ci-info": "^4.0.0",
@@ -118,6 +120,8 @@
     "glob": "^10.3.10",
     "mocha": "^10.2.0",
     "prettier": "^3.0.3 <3.1",
+    "react": "^18.2.0",
+    "react-dom": "^18.2.0",
     "rimraf": "^5.0.5",
     "tempy": "^3.1.0",
     "typescript": "^5.2.2",

src/files.ts

@@ -48,21 +48,17 @@ export function* visitMarkdownFiles(root: string): Generator<string> {
   }
 }
 
-/** Yields every file within the given root, recursively. */
+/** Yields every file within the given root, recursively, ignoring .observablehq. */
 export function* visitFiles(root: string): Generator<string> {
   const visited = new Set<number>();
   const queue: string[] = [(root = normalize(root))];
-  try {
-    visited.add(statSync(join(root, ".observablehq")).ino);
-  } catch {
-    // ignore the .observablehq directory, if it exists
-  }
   for (const path of queue) {
     const status = statSync(path);
     if (status.isDirectory()) {
       if (visited.has(status.ino)) continue; // circular symlink
       visited.add(status.ino);
       for (const entry of readdirSync(path)) {
+        if (entry === ".observablehq") continue; // ignore the .observablehq directory
         queue.push(join(path, entry));
       }
     } else {

src/node.ts

@@ -4,23 +4,25 @@ import {createRequire} from "node:module";
 import op from "node:path";
 import {extname, join} from "node:path/posix";
 import {pathToFileURL} from "node:url";
+import commonjs from "@rollup/plugin-commonjs";
 import {nodeResolve} from "@rollup/plugin-node-resolve";
+import virtual from "@rollup/plugin-virtual";
 import {packageDirectory} from "pkg-dir";
 import type {AstNode, OutputChunk, Plugin, ResolveIdResult} from "rollup";
 import {rollup} from "rollup";
 import esbuild from "rollup-plugin-esbuild";
 import {prepareOutput, toOsPath} from "./files.js";
 import type {ImportReference} from "./javascript/imports.js";
 import {isJavaScript, parseImports} from "./javascript/imports.js";
-import {parseNpmSpecifier} from "./npm.js";
-import {isPathImport} from "./path.js";
+import {parseNpmSpecifier, rewriteNpmImports} from "./npm.js";
+import {isPathImport, relativePath} from "./path.js";
 import {faint} from "./tty.js";
 
 export async function resolveNodeImport(root: string, spec: string): Promise<string> {
   return resolveNodeImportInternal(op.join(root, ".observablehq", "cache", "_node"), root, spec);
 }
 
-const bundlePromises = new Map<string, Promise<void>>();
+const bundlePromises = new Map<string, Promise<string>>();
 
 async function resolveNodeImportInternal(cacheRoot: string, packageRoot: string, spec: string): Promise<string> {
   const {name, path = "."} = parseNpmSpecifier(spec);
@@ -31,24 +33,23 @@ async function resolveNodeImportInternal(cacheRoot: string, packageRoot: string,
   const {version} = JSON.parse(await readFile(op.join(packageResolution, "package.json"), "utf-8"));
   const resolution = `${name}@${version}/${extname(path) ? path : path === "." ? "index.js" : `${path}.js`}`;
   const outputPath = op.join(cacheRoot, toOsPath(resolution));
-  if (!existsSync(outputPath)) {
-    let promise = bundlePromises.get(outputPath);
-    if (!promise) {
-      promise = (async () => {
-        process.stdout.write(`${spec} ${faint("→")} ${resolution}\n`);
-        await prepareOutput(outputPath);
-        if (isJavaScript(pathResolution)) {
-          await writeFile(outputPath, await bundle(spec, cacheRoot, packageResolution));
-        } else {
-          await copyFile(pathResolution, outputPath);
-        }
-      })();
-      bundlePromises.set(outputPath, promise);
-      promise.catch(console.error).then(() => bundlePromises.delete(outputPath));
+  const resolutionPath = `/_node/${resolution}`;
+  if (existsSync(outputPath)) return resolutionPath;
+  let promise = bundlePromises.get(outputPath);
+  if (promise) return promise; // coalesce concurrent requests
+  promise = (async () => {
+    console.log(`${spec} ${faint("→")} ${outputPath}`);
+    await prepareOutput(outputPath);
+    if (isJavaScript(pathResolution)) {
+      await writeFile(outputPath, await bundle(resolutionPath, spec, require, cacheRoot, packageResolution), "utf-8");
+    } else {
+      await copyFile(pathResolution, outputPath);
     }
-    await promise;
-  }
-  return `/_node/${resolution}`;
+    return resolutionPath;
+  })();
+  promise.catch(console.error).then(() => bundlePromises.delete(outputPath));
+  bundlePromises.set(outputPath, promise);
+  return promise;
 }
 
 /**
@@ -69,29 +70,59 @@ export function extractNodeSpecifier(path: string): string {
   return path.replace(/^\/_node\//, "");
 }
 
-async function bundle(input: string, cacheRoot: string, packageRoot: string): Promise<string> {
+/**
+ * React (and its dependencies) are distributed as CommonJS modules, and worse,
+ * they’re incompatible with cjs-module-lexer; so when we try to import them as
+ * ES modules we only see a default export. We fix this by creating a shim
+ * module that exports everything that is visible to require. I hope the React
+ * team distributes ES modules soon…
+ *
+ * https://github.com/facebook/react/issues/11503
+ */
+function isBadCommonJs(specifier: string): boolean {
+  const {name} = parseNpmSpecifier(specifier);
+  return name === "react" || name === "react-dom" || name === "react-is" || name === "scheduler";
+}
+
+function shimCommonJs(specifier: string, require: NodeRequire): string {
+  return `export {${Object.keys(require(specifier))}} from ${JSON.stringify(specifier)};\n`;
+}
+
+async function bundle(
+  path: string,
+  input: string,
+  require: NodeRequire,
+  cacheRoot: string,
+  packageRoot: string
+): Promise<string> {
   const bundle = await rollup({
-    input,
+    input: isBadCommonJs(input) ? "-" : input,
     plugins: [
-      nodeResolve({browser: true, rootDir: packageRoot}),
+      ...(isBadCommonJs(input) ? [(virtual as any)({"-": shimCommonJs(input, require)})] : []),
       importResolve(input, cacheRoot, packageRoot),
+      nodeResolve({browser: true, rootDir: packageRoot}),
+      (commonjs as any)({esmExternals: true}),
       esbuild({
         format: "esm",
         platform: "browser",
         target: ["es2022", "chrome96", "firefox96", "safari16", "node18"],
         exclude: [], // don’t exclude node_modules
+        define: {"process.env.NODE_ENV": JSON.stringify("production")},
         minify: true
       })
     ],
+    external(source) {
+      return source.startsWith("/_node/");
+    },
     onwarn(message, warn) {
       if (message.code === "CIRCULAR_DEPENDENCY") return;
       warn(message);
     }
   });
   try {
-    const output = await bundle.generate({format: "es"});
-    const code = output.output.find((o): o is OutputChunk => o.type === "chunk")!.code; // TODO don’t assume one chunk?
-    return code;
+    const output = await bundle.generate({format: "es", exports: "named"});
+    const code = output.output.find((o): o is OutputChunk => o.type === "chunk")!.code;
+    return rewriteNpmImports(code, (i) => (i.startsWith("/_node/") ? relativePath(path, i) : i));
   } finally {
     await bundle.close();
   }

src/npm.ts

@@ -75,29 +75,28 @@ const npmRequests = new Map<string, Promise<string>>();
 /** Note: path must start with "/_npm/". */
 export async function populateNpmCache(root: string, path: string): Promise<string> {
   if (!path.startsWith("/_npm/")) throw new Error(`invalid npm path: ${path}`);
-  const filePath = join(root, ".observablehq", "cache", path);
-  if (existsSync(filePath)) return filePath;
-  let promise = npmRequests.get(filePath);
+  const outputPath = join(root, ".observablehq", "cache", path);
+  if (existsSync(outputPath)) return outputPath;
+  let promise = npmRequests.get(outputPath);
   if (promise) return promise; // coalesce concurrent requests
-  promise = (async function () {
+  promise = (async () => {
     const specifier = extractNpmSpecifier(path);
     const href = `https://cdn.jsdelivr.net/npm/${specifier}`;
-    process.stdout.write(`npm:${specifier} ${faint("→")} `);
+    console.log(`npm:${specifier} ${faint("→")} ${outputPath}`);
     const response = await fetch(href);
     if (!response.ok) throw new Error(`unable to fetch: ${href}`);
-    process.stdout.write(`${filePath}\n`);
-    await mkdir(dirname(filePath), {recursive: true});
+    await mkdir(dirname(outputPath), {recursive: true});
     if (/^application\/javascript(;|$)/i.test(response.headers.get("content-type")!)) {
       const source = await response.text();
       const resolver = await getDependencyResolver(root, path, source);
-      await writeFile(filePath, rewriteNpmImports(source, resolver), "utf-8");
+      await writeFile(outputPath, rewriteNpmImports(source, resolver), "utf-8");
     } else {
-      await writeFile(filePath, Buffer.from(await response.arrayBuffer()));
+      await writeFile(outputPath, Buffer.from(await response.arrayBuffer()));
     }
-    return filePath;
+    return outputPath;
   })();
-  promise.catch(() => {}).then(() => npmRequests.delete(filePath));
-  npmRequests.set(filePath, promise);
+  promise.catch(console.error).then(() => npmRequests.delete(outputPath));
+  npmRequests.set(outputPath, promise);
   return promise;
 }
 

test/files-test.ts

@@ -62,6 +62,9 @@ describe("visitFiles(root)", () => {
     if (os.platform() === "win32") this.skip(); // symlinks are not the same on Windows
     assert.deepStrictEqual(collect(visitFiles("test/input/circular-files")), ["a/a.txt", "b/b.txt"]);
   });
+  it("ignores .observablehq at any level", function () {
+    assert.deepStrictEqual(collect(visitFiles("test/files")), ["visible.txt", "sub/visible.txt"]);
+  });
 });
 
 describe("visitMarkdownFiles(root)", () => {

test/files/.observablehq/hidden.txt

@@ -0,0 +1 @@
+This file shouldn’t be found by visitFiles.

test/files/sub/.observablehq/hidden.txt

@@ -0,0 +1 @@
+This file shouldn’t be found by visitFiles.

test/files/sub/visible.txt

@@ -0,0 +1 @@
+This file should be found by visitFiles.

test/files/visible.txt

@@ -0,0 +1 @@
+This file should be found by visitFiles.

test/node-test.ts

@@ -1,6 +1,6 @@
 import assert from "node:assert";
-import {existsSync} from "node:fs";
 import {rm} from "node:fs/promises";
+import {join} from "node:path/posix";
 import {extractNodeSpecifier, isNodeBuiltin, resolveNodeImport, resolveNodeImports} from "../src/node.js";
 
 describe("isNodeBuiltin(specifier)", () => {
@@ -29,64 +29,63 @@ describe("isNodeBuiltin(specifier)", () => {
   });
 });
 
-describe("resolveNodeImport(root, spec)", () => {
-  const importRoot = "../../input/packages/.observablehq/cache";
-  before(async () => {
-    await rm("docs/.observablehq/cache/_node", {recursive: true, force: true});
-    await rm("test/input/packages/.observablehq/cache", {recursive: true, force: true});
-  });
+describe("resolveNodeImport(root, spec) with top-level node_modules", () => {
+  const testRoot = "test/output/node-import"; // unique to this test
+  before(() => rm(join(testRoot, ".observablehq/cache/_node"), {recursive: true, force: true}));
   it("resolves the version of a direct dependency", async () => {
-    assert.deepStrictEqual(await resolveNodeImport("docs", "d3-array"), "/_node/[email protected]/index.js");
-    assert.deepStrictEqual(await resolveNodeImport("docs", "mime"), "/_node/[email protected]/index.js");
+    assert.deepStrictEqual(await resolveNodeImport(testRoot, "d3-array"), "/_node/[email protected]/index.js");
+    assert.deepStrictEqual(await resolveNodeImport(testRoot, "mime"), "/_node/[email protected]/index.js");
   });
   it("allows entry points", async () => {
-    assert.deepStrictEqual(await resolveNodeImport("docs", "mime/lite"), "/_node/[email protected]/lite.js");
+    assert.deepStrictEqual(await resolveNodeImport(testRoot, "mime/lite"), "/_node/[email protected]/lite.js");
   });
   it("allows non-javascript entry points", async () => {
-    assert.deepStrictEqual(await resolveNodeImport("docs", "glob/package.json"), "/_node/[email protected]/package.json");
+    assert.deepStrictEqual(await resolveNodeImport(testRoot, "glob/package.json"), "/_node/[email protected]/package.json");
   });
   it("does not allow version ranges", async () => {
-    await assert.rejects(() => resolveNodeImport("docs", "mime@4"), /Cannot find module/);
+    await assert.rejects(() => resolveNodeImport(testRoot, "mime@4"), /Cannot find module/);
   });
+});
+
+describe("resolveNodeImport(root, spec) with test node_modules", () => {
+  const testRoot = "test/input/packages"; // unique to this test
+  const importRoot = "../../input/packages/.observablehq/cache";
+  before(() => rm(join(testRoot, ".observablehq/cache/_node"), {recursive: true, force: true}));
   it("bundles a package with a shorthand export", async () => {
-    assert.strictEqual(await resolveNodeImport("test/input/packages", "test-shorthand-export"), "/_node/[email protected]/index.js"); // prettier-ignore
+    assert.strictEqual(await resolveNodeImport(testRoot, "test-shorthand-export"), "/_node/[email protected]/index.js"); // prettier-ignore
     assert.strictEqual((await import(`${importRoot}/_node/[email protected]/index.js`)).name, "test-shorthand-export"); // prettier-ignore
   });
   it("bundles a package with an import conditional export", async () => {
-    assert.strictEqual(await resolveNodeImport("test/input/packages", "test-import-condition"), "/_node/[email protected]/index.js"); // prettier-ignore
+    assert.strictEqual(await resolveNodeImport(testRoot, "test-import-condition"), "/_node/[email protected]/index.js"); // prettier-ignore
     assert.strictEqual((await import(`${importRoot}/_node/[email protected]/index.js`)).name, "test-import-condition:import"); // prettier-ignore
   });
   it("bundles a package with a browser field", async () => {
-    assert.strictEqual(await resolveNodeImport("test/input/packages", "test-browser-field"), "/_node/[email protected]/index.js"); // prettier-ignore
+    assert.strictEqual(await resolveNodeImport(testRoot, "test-browser-field"), "/_node/[email protected]/index.js"); // prettier-ignore
     assert.strictEqual((await import(`${importRoot}/_node/[email protected]/index.js`)).name, "test-browser-field:browser"); // prettier-ignore
   });
   it("bundles a package with a browser map", async () => {
-    assert.strictEqual(await resolveNodeImport("test/input/packages", "test-browser-map"), "/_node/[email protected]/index.js"); // prettier-ignore
+    assert.strictEqual(await resolveNodeImport(testRoot, "test-browser-map"), "/_node/[email protected]/index.js"); // prettier-ignore
     assert.strictEqual((await import(`${importRoot}/_node/[email protected]/index.js`)).name, "test-browser-map:browser"); // prettier-ignore
   });
   it("bundles a package with a browser conditional export", async () => {
-    assert.strictEqual(await resolveNodeImport("test/input/packages", "test-browser-condition"), "/_node/[email protected]/index.js"); // prettier-ignore
+    assert.strictEqual(await resolveNodeImport(testRoot, "test-browser-condition"), "/_node/[email protected]/index.js"); // prettier-ignore
     assert.strictEqual((await import(`${importRoot}/_node/[email protected]/index.js`)).name, "test-browser-condition:browser"); // prettier-ignore
   });
 });
 
 describe("resolveNodeImports(root, path)", () => {
-  before(async () => {
-    if (existsSync("docs/.observablehq/cache/_node")) {
-      await rm("docs/.observablehq/cache/_node", {recursive: true});
-    }
-  });
+  const testRoot = "test/output/node-imports"; // unique to this test
+  before(() => rm(join(testRoot, ".observablehq/cache/_node"), {recursive: true, force: true}));
   it("resolves the imports of a dependency", async () => {
-    assert.deepStrictEqual(await resolveNodeImports("docs", await resolveNodeImport("docs", "d3-array")), [
-      {
-        method: "static",
-        name: "../[email protected]/index.js",
-        type: "local"
-      }
+    assert.deepStrictEqual(await resolveNodeImports(testRoot, await resolveNodeImport(testRoot, "d3-array")), [
+      {method: "static", name: "../[email protected]/index.js", type: "local"}
     ]);
   });
   it("ignores non-JavaScript paths", async () => {
-    assert.deepStrictEqual(await resolveNodeImports("docs", await resolveNodeImport("docs", "glob/package.json")), []);
+    assert.deepStrictEqual(
+      await resolveNodeImports(testRoot, await resolveNodeImport(testRoot, "glob/package.json")),
+      []
+    );
   });
 });