From 3e9a5ef64542519194eb1e39a3352832c099dc04 Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Wed, 27 Mar 2024 12:12:02 -0700 Subject: [PATCH 01/22] checkpoint import from node_modules --- docs/node.md | 5 ++++ src/node.ts | 62 ++++++++++++++++++++++++++++++++++++++++++++++++ src/preview.ts | 2 ++ src/resolvers.ts | 35 +++++++++++++++++---------- 4 files changed, 92 insertions(+), 12 deletions(-) create mode 100644 docs/node.md create mode 100644 src/node.ts diff --git a/docs/node.md b/docs/node.md new file mode 100644 index 000000000..e264b9f15 --- /dev/null +++ b/docs/node.md @@ -0,0 +1,5 @@ +# Hello node + +```js echo +import * as d3 from "d3-array"; +``` diff --git a/src/node.ts b/src/node.ts new file mode 100644 index 000000000..0e6183350 --- /dev/null +++ b/src/node.ts @@ -0,0 +1,62 @@ +import {existsSync} from "node:fs"; +import {copyFile, readFile} from "node:fs/promises"; +import {createRequire} from "node:module"; +import {dirname, join, relative} from "node:path/posix"; +import {pathToFileURL} from "node:url"; +import {prepareOutput} from "./files.js"; +import {parseNpmSpecifier} from "./npm.js"; +import {faint} from "./tty.js"; + +export async function resolveNodeImport(root: string, spec: string): Promise { + const specifier = parseNpmSpecifier(spec); + const require = createRequire(pathToFileURL(join(root, "/"))); + const pathResolution = require.resolve(spec); + let packageResolution = pathResolution; + do { + const p = dirname(packageResolution); + if (p === packageResolution) throw new Error(`unable to resolve package.json: ${spec}`); + packageResolution = p; + } while (!existsSync(join(packageResolution, "package.json"))); + const {version} = JSON.parse(await readFile(join(packageResolution, "package.json"), "utf-8")); + const relativePath = relative(packageResolution, pathResolution); + const resolution = `${specifier.name}@${version}/${relativePath}`; + const outputPath = join(root, ".observablehq", "cache", "_node", resolution); + if (!existsSync(outputPath)) { + // TODO bundle + process.stdout.write(`${spec} ${faint("→")} `); + await prepareOutput(outputPath); + await copyFile(pathResolution, outputPath); + process.stdout.write(`${resolution}\n`); + } + return `/_node/${resolution}`; +} + +/** Note: path must start with "/_node/". */ +export async function populateNodeCache(root: string, path: string): Promise { + if (!path.startsWith("/_node/")) throw new Error(`invalid node path: ${path}`); + const filePath = join(root, ".observablehq", "cache", path); + if (existsSync(filePath)) return filePath; + throw new Error("not yet implemented"); + // let promise = npmRequests.get(path); + // if (promise) return promise; // coalesce concurrent requests + // promise = (async function () { + // const specifier = extractNpmSpecifier(path); + // const href = `https://cdn.jsdelivr.net/npm/${specifier}`; + // process.stdout.write(`npm:${specifier} ${faint("→")} `); + // 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}); + // 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"); + // } else { + // await writeFile(filePath, Buffer.from(await response.arrayBuffer())); + // } + // return filePath; + // })(); + // promise.catch(() => {}).then(() => npmRequests.delete(path)); + // npmRequests.set(path, promise); + // return promise; +} diff --git a/src/preview.ts b/src/preview.ts index 2d2c84af1..35b3619c4 100644 --- a/src/preview.ts +++ b/src/preview.ts @@ -124,6 +124,8 @@ export class PreviewServer { } else if (pathname.startsWith("/_observablehq/") && pathname.endsWith(".css")) { const path = getClientPath(pathname.slice("/_observablehq/".length)); end(req, res, await bundleStyles({path}), "text/css"); + } else if (pathname.startsWith("/_node/")) { + send(req, pathname, {root: join(root, ".observablehq", "cache")}).pipe(res); } else if (pathname.startsWith("/_npm/")) { await populateNpmCache(root, pathname); send(req, pathname, {root: join(root, ".observablehq", "cache")}).pipe(res); diff --git a/src/resolvers.ts b/src/resolvers.ts index a34ebe945..a942714d8 100644 --- a/src/resolvers.ts +++ b/src/resolvers.ts @@ -10,6 +10,7 @@ import {getImplicitStylesheets} from "./libraries.js"; import type {MarkdownPage} from "./markdown.js"; import {extractNpmSpecifier, populateNpmCache, resolveNpmImport, resolveNpmImports} from "./npm.js"; import {isAssetPath, isPathImport, relativePath, resolveLocalPath, resolvePath} from "./path.js"; +import {resolveNodeImport} from "./node.js"; export interface Resolvers { path: string; @@ -169,31 +170,40 @@ export async function getResolvers( globalImports.add(i); } - // Resolve npm: imports. + // Resolve npm: and node imports. for (const i of globalImports) { - if (i.startsWith("npm:") && !builtins.has(i)) { + if (i.startsWith("observablehq:") || builtins.has(i)) { + continue; + } else if (i.startsWith("npm:")) { resolutions.set(i, await resolveNpmImport(root, i.slice("npm:".length))); + } else { + resolutions.set(i, await resolveNodeImport(root, i)); } } // Follow transitive imports of npm imports. This has the side-effect of - // populating the npm cache. - for (const value of resolutions.values()) { - for (const i of await resolveNpmImports(root, value)) { - if (i.type === "local") { - const path = resolvePath(value, i.name); - const specifier = `npm:${extractNpmSpecifier(path)}`; - globalImports.add(specifier); - resolutions.set(specifier, path); + // populating the npm cache. TODO Transitive node imports, too? + for (const [key, value] of resolutions) { + if (key.startsWith("npm:")) { + for (const i of await resolveNpmImports(root, value)) { + if (i.type === "local") { + const path = resolvePath(value, i.name); + const specifier = `npm:${extractNpmSpecifier(path)}`; + globalImports.add(specifier); + resolutions.set(specifier, path); + } } } } // Resolve transitive static npm: imports. + // TODO Transitive static node imports, too? const npmStaticResolutions = new Set(); for (const i of staticImports) { - const r = resolutions.get(i); - if (r) npmStaticResolutions.add(r); + if (i.startsWith("npm:")) { + const r = resolutions.get(i); + if (r) npmStaticResolutions.add(r); + } } for (const value of npmStaticResolutions) { for (const i of await resolveNpmImports(root, value)) { @@ -207,6 +217,7 @@ export async function getResolvers( } // Add implicit stylesheets. + // TODO Node imports? for (const specifier of getImplicitStylesheets(staticImports)) { stylesheets.add(specifier); if (specifier.startsWith("npm:")) { From c17ec119870a327da949b80b1eeb3d36a699368d Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Wed, 27 Mar 2024 12:31:37 -0700 Subject: [PATCH 02/22] it works! --- docs/node.md | 2 ++ src/node.ts | 74 +++++++++++++++++++++++++++++----------------------- 2 files changed, 44 insertions(+), 32 deletions(-) diff --git a/docs/node.md b/docs/node.md index e264b9f15..da3f599d8 100644 --- a/docs/node.md +++ b/docs/node.md @@ -2,4 +2,6 @@ ```js echo import * as d3 from "d3-array"; + +display(d3); ``` diff --git a/src/node.ts b/src/node.ts index 0e6183350..eaf67c7d5 100644 --- a/src/node.ts +++ b/src/node.ts @@ -1,15 +1,22 @@ import {existsSync} from "node:fs"; -import {copyFile, readFile} from "node:fs/promises"; +import {readFile, writeFile} from "node:fs/promises"; import {createRequire} from "node:module"; import {dirname, join, relative} from "node:path/posix"; import {pathToFileURL} from "node:url"; +import type {AstNode, OutputChunk, Plugin, ResolveIdResult} from "rollup"; +import {rollup} from "rollup"; import {prepareOutput} from "./files.js"; import {parseNpmSpecifier} from "./npm.js"; +import {isPathImport} from "./path.js"; import {faint} from "./tty.js"; export async function resolveNodeImport(root: string, spec: string): Promise { + return resolveNodeImportInternal(root, root, spec); +} + +async function resolveNodeImportInternal(root: string, packageRoot: string, spec: string): Promise { const specifier = parseNpmSpecifier(spec); - const require = createRequire(pathToFileURL(join(root, "/"))); + const require = createRequire(pathToFileURL(join(packageRoot, "/"))); const pathResolution = require.resolve(spec); let packageResolution = pathResolution; do { @@ -22,41 +29,44 @@ export async function resolveNodeImport(root: string, spec: string): Promise { - if (!path.startsWith("/_node/")) throw new Error(`invalid node path: ${path}`); - const filePath = join(root, ".observablehq", "cache", path); - if (existsSync(filePath)) return filePath; - throw new Error("not yet implemented"); - // let promise = npmRequests.get(path); - // if (promise) return promise; // coalesce concurrent requests - // promise = (async function () { - // const specifier = extractNpmSpecifier(path); - // const href = `https://cdn.jsdelivr.net/npm/${specifier}`; - // process.stdout.write(`npm:${specifier} ${faint("→")} `); - // 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}); - // 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"); - // } else { - // await writeFile(filePath, Buffer.from(await response.arrayBuffer())); - // } - // return filePath; - // })(); - // promise.catch(() => {}).then(() => npmRequests.delete(path)); - // npmRequests.set(path, promise); - // return promise; +async function bundle(input: string, root: string, packageRoot: string): Promise { + const bundle = await rollup({ + input, + plugins: [ + importResolve(root, packageRoot) + // TODO minify with esbuild during build + ], + 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; + } finally { + await bundle.close(); + } +} + +function importResolve(root: string, packageRoot: string): Plugin { + async function resolve(specifier: string | AstNode): Promise { + if (typeof specifier !== "string") throw new Error(`unexpected specifier: ${specifier}`); + if (isPathImport(specifier)) return null; + return {id: await resolveNodeImportInternal(root, packageRoot, specifier), external: true}; + } + return { + name: "resolve-import", + resolveId: resolve, + resolveDynamicImport: resolve + }; } From 8095c6895dff51558839f9a46e550bf60a57d3c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Philippe=20Rivi=C3=A8re?= Date: Wed, 27 Mar 2024 20:37:32 +0100 Subject: [PATCH 03/22] minify --- src/node.ts | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/node.ts b/src/node.ts index eaf67c7d5..806aa4b43 100644 --- a/src/node.ts +++ b/src/node.ts @@ -5,6 +5,7 @@ import {dirname, join, relative} from "node:path/posix"; import {pathToFileURL} from "node:url"; import type {AstNode, OutputChunk, Plugin, ResolveIdResult} from "rollup"; import {rollup} from "rollup"; +import esbuild from "rollup-plugin-esbuild"; import {prepareOutput} from "./files.js"; import {parseNpmSpecifier} from "./npm.js"; import {isPathImport} from "./path.js"; @@ -41,8 +42,12 @@ async function bundle(input: string, root: string, packageRoot: string): Promise const bundle = await rollup({ input, plugins: [ - importResolve(root, packageRoot) - // TODO minify with esbuild during build + importResolve(root, packageRoot), + esbuild({ + target: ["es2022", "chrome96", "firefox96", "safari16", "node18"], + exclude: [], // don’t exclude node_modules + minify: true + }) ], onwarn(message, warn) { if (message.code === "CIRCULAR_DEPENDENCY") return; From 34144a236b5335f94ea36083db127caeea3b5a38 Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Wed, 27 Mar 2024 14:06:48 -0700 Subject: [PATCH 04/22] only resolve bare imports --- src/resolvers.ts | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/src/resolvers.ts b/src/resolvers.ts index a942714d8..dbb46f13f 100644 --- a/src/resolvers.ts +++ b/src/resolvers.ts @@ -8,9 +8,9 @@ import {getImplicitDependencies, getImplicitDownloads} from "./libraries.js"; import {getImplicitFileImports, getImplicitInputImports} from "./libraries.js"; import {getImplicitStylesheets} from "./libraries.js"; import type {MarkdownPage} from "./markdown.js"; +import {resolveNodeImport} from "./node.js"; import {extractNpmSpecifier, populateNpmCache, resolveNpmImport, resolveNpmImports} from "./npm.js"; import {isAssetPath, isPathImport, relativePath, resolveLocalPath, resolvePath} from "./path.js"; -import {resolveNodeImport} from "./node.js"; export interface Resolvers { path: string; @@ -170,13 +170,11 @@ export async function getResolvers( globalImports.add(i); } - // Resolve npm: and node imports. + // Resolve npm: and bare imports. for (const i of globalImports) { - if (i.startsWith("observablehq:") || builtins.has(i)) { - continue; - } else if (i.startsWith("npm:")) { + if (i.startsWith("npm:") && !builtins.has(i)) { resolutions.set(i, await resolveNpmImport(root, i.slice("npm:".length))); - } else { + } else if (!/^\w+:/.test(i)) { resolutions.set(i, await resolveNodeImport(root, i)); } } From e776e61c5d3df759b712f31be3713124d1fd51f9 Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Wed, 27 Mar 2024 14:17:57 -0700 Subject: [PATCH 05/22] coalesce imports; ignore errors --- src/node.ts | 17 +++++++++++++---- src/resolvers.ts | 6 +++++- 2 files changed, 18 insertions(+), 5 deletions(-) diff --git a/src/node.ts b/src/node.ts index 806aa4b43..85660b3ad 100644 --- a/src/node.ts +++ b/src/node.ts @@ -15,6 +15,8 @@ export async function resolveNodeImport(root: string, spec: string): Promise>(); + async function resolveNodeImportInternal(root: string, packageRoot: string, spec: string): Promise { const specifier = parseNpmSpecifier(spec); const require = createRequire(pathToFileURL(join(packageRoot, "/"))); @@ -30,10 +32,17 @@ async function resolveNodeImportInternal(root: string, packageRoot: string, spec const resolution = `${specifier.name}@${version}/${relativePath}`; const outputPath = join(root, ".observablehq", "cache", "_node", resolution); if (!existsSync(outputPath)) { - process.stdout.write(`${spec} ${faint("→")} `); - await prepareOutput(outputPath); - await writeFile(outputPath, await bundle(pathResolution, root, packageResolution)); - process.stdout.write(`${resolution}\n`); + let promise = bundlePromises.get(outputPath); + if (!promise) { + promise = (async () => { + process.stdout.write(`${spec} ${faint("→")} ${resolution}\n`); + await prepareOutput(outputPath); + await writeFile(outputPath, await bundle(pathResolution, root, packageResolution)); + })(); + bundlePromises.set(outputPath, promise); + promise.catch(() => {}).then(() => bundlePromises.delete(outputPath)); + } + await promise; } return `/_node/${resolution}`; } diff --git a/src/resolvers.ts b/src/resolvers.ts index dbb46f13f..1c655aa69 100644 --- a/src/resolvers.ts +++ b/src/resolvers.ts @@ -175,7 +175,11 @@ export async function getResolvers( if (i.startsWith("npm:") && !builtins.has(i)) { resolutions.set(i, await resolveNpmImport(root, i.slice("npm:".length))); } else if (!/^\w+:/.test(i)) { - resolutions.set(i, await resolveNodeImport(root, i)); + try { + resolutions.set(i, await resolveNodeImport(root, i)); + } catch { + // ignore resolution error; allow the import to be resolved at runtime + } } } From ed6bb45ede1de1c8a5e9f3100c8e02000c31cc44 Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Wed, 27 Mar 2024 14:26:38 -0700 Subject: [PATCH 06/22] preload transitive dependencies --- src/node.ts | 30 +++++++++++++++++++++++++++ src/resolvers.ts | 53 ++++++++++++++++++++++++++++++++---------------- 2 files changed, 66 insertions(+), 17 deletions(-) diff --git a/src/node.ts b/src/node.ts index 85660b3ad..cadc12051 100644 --- a/src/node.ts +++ b/src/node.ts @@ -7,6 +7,9 @@ import type {AstNode, OutputChunk, Plugin, ResolveIdResult} from "rollup"; import {rollup} from "rollup"; import esbuild from "rollup-plugin-esbuild"; import {prepareOutput} from "./files.js"; +import type {ImportReference} from "./javascript/imports.js"; +import {findImports} from "./javascript/imports.js"; +import {parseProgram} from "./javascript/parse.js"; import {parseNpmSpecifier} from "./npm.js"; import {isPathImport} from "./path.js"; import {faint} from "./tty.js"; @@ -47,6 +50,33 @@ async function resolveNodeImportInternal(root: string, packageRoot: string, spec return `/_node/${resolution}`; } +/** + * Resolves the direct dependencies of the specified node import path, such as + * "/_node/d3-array@3.2.4/src/index.js", returning a set of node import paths. + */ +export async function resolveNodeImports(root: string, path: string): Promise { + if (!path.startsWith("/_node/")) throw new Error(`invalid node path: ${path}`); + try { + const filePath = join(root, ".observablehq", "cache", path); + if (!/\.(m|c)?js$/i.test(path)) return []; // not JavaScript; TODO traverse CSS, too + const source = await readFile(filePath, "utf-8"); + const body = parseProgram(source); + return findImports(body, path, source); + } catch (error: any) { + console.warn(`unable to fetch or parse ${path}: ${error.message}`); + return []; + } +} + +/** + * Given a local npm path such as "/_node/d3-array@3.2.4/src/index.js", returns + * the corresponding npm specifier such as "d3-array@3.2.4/src/index.js". + */ +export function extractNodeSpecifier(path: string): string { + if (!path.startsWith("/_node/")) throw new Error(`invalid node path: ${path}`); + return path.replace(/^\/_node\//, ""); +} + async function bundle(input: string, root: string, packageRoot: string): Promise { const bundle = await rollup({ input, diff --git a/src/resolvers.ts b/src/resolvers.ts index 1c655aa69..db4b77340 100644 --- a/src/resolvers.ts +++ b/src/resolvers.ts @@ -8,7 +8,7 @@ import {getImplicitDependencies, getImplicitDownloads} from "./libraries.js"; import {getImplicitFileImports, getImplicitInputImports} from "./libraries.js"; import {getImplicitStylesheets} from "./libraries.js"; import type {MarkdownPage} from "./markdown.js"; -import {resolveNodeImport} from "./node.js"; +import {extractNodeSpecifier, resolveNodeImport, resolveNodeImports} from "./node.js"; import {extractNpmSpecifier, populateNpmCache, resolveNpmImport, resolveNpmImports} from "./npm.js"; import {isAssetPath, isPathImport, relativePath, resolveLocalPath, resolvePath} from "./path.js"; @@ -178,13 +178,14 @@ export async function getResolvers( try { resolutions.set(i, await resolveNodeImport(root, i)); } catch { - // ignore resolution error; allow the import to be resolved at runtime + // ignore error; allow the import to be resolved at runtime } } } - // Follow transitive imports of npm imports. This has the side-effect of - // populating the npm cache. TODO Transitive node imports, too? + // Follow transitive imports of npm and bare imports. This has the side-effect + // of populating the npm cache; the node import cache is already transitively + // populated above. for (const [key, value] of resolutions) { if (key.startsWith("npm:")) { for (const i of await resolveNpmImports(root, value)) { @@ -195,31 +196,49 @@ export async function getResolvers( resolutions.set(specifier, path); } } + } else if (!/^\w+:/.test(key)) { + for (const i of await resolveNodeImports(root, value)) { + if (i.type === "local") { + const path = resolvePath(value, i.name); + const specifier = extractNodeSpecifier(path); + globalImports.add(specifier); + resolutions.set(specifier, path); + } + } } } - // Resolve transitive static npm: imports. - // TODO Transitive static node imports, too? - const npmStaticResolutions = new Set(); + // Resolve transitive static npm: and bare imports. + const staticResolutions = new Map(); for (const i of staticImports) { - if (i.startsWith("npm:")) { + if (i.startsWith("npm:") || !/^\w+:/.test(i)) { const r = resolutions.get(i); - if (r) npmStaticResolutions.add(r); + if (r) staticResolutions.set(i, r); } } - for (const value of npmStaticResolutions) { - for (const i of await resolveNpmImports(root, value)) { - if (i.type === "local" && i.method === "static") { - const path = resolvePath(value, i.name); - const specifier = `npm:${extractNpmSpecifier(path)}`; - staticImports.add(specifier); - npmStaticResolutions.add(path); + for (const [key, value] of staticResolutions) { + if (key.startsWith("npm:")) { + for (const i of await resolveNpmImports(root, value)) { + if (i.type === "local" && i.method === "static") { + const path = resolvePath(value, i.name); + const specifier = `npm:${extractNpmSpecifier(path)}`; + staticImports.add(specifier); + staticResolutions.set(specifier, path); + } + } + } else if (!/^\w+:/.test(key)) { + for (const i of await resolveNodeImports(root, value)) { + if (i.type === "local" && i.method === "static") { + const path = resolvePath(value, i.name); + const specifier = extractNodeSpecifier(path); + staticImports.add(specifier); + staticResolutions.set(specifier, path); + } } } } // Add implicit stylesheets. - // TODO Node imports? for (const specifier of getImplicitStylesheets(staticImports)) { stylesheets.add(specifier); if (specifier.startsWith("npm:")) { From 02f5ad3bb4851f7e631bade70a1a9b15cb645b77 Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Wed, 27 Mar 2024 17:19:48 -0700 Subject: [PATCH 07/22] DRY parseImports --- src/javascript/imports.ts | 22 ++++++++++++++++++++++ src/node.ts | 14 ++------------ src/npm.ts | 21 ++------------------- 3 files changed, 26 insertions(+), 31 deletions(-) diff --git a/src/javascript/imports.ts b/src/javascript/imports.ts index 961f94b3f..364666ed8 100644 --- a/src/javascript/imports.ts +++ b/src/javascript/imports.ts @@ -1,8 +1,10 @@ +import {readFile} from "node:fs/promises"; import type {Node} from "acorn"; import type {CallExpression} from "acorn"; import type {ExportAllDeclaration, ExportNamedDeclaration, ImportDeclaration, ImportExpression} from "acorn"; import {simple} from "acorn-walk"; import {isPathImport, relativePath, resolveLocalPath} from "../path.js"; +import {parseProgram} from "./parse.js"; import {getStringLiteralValue, isStringLiteral} from "./source.js"; import {syntaxError} from "./syntaxError.js"; @@ -109,3 +111,23 @@ export function isImportMetaResolve(node: CallExpression): boolean { node.arguments.length > 0 ); } + +const parseImportsCache = new Map>(); + +export async function parseImports(path: string): Promise { + if (!/\.(m|c)?js$/i.test(path)) return []; // not JavaScript; TODO traverse CSS, too + let promise = parseImportsCache.get(path); + if (promise) return promise; + promise = (async function () { + try { + const source = await readFile(path, "utf-8"); + const body = parseProgram(source); + return findImports(body, path, source); + } catch (error: any) { + console.warn(`unable to fetch or parse ${path}: ${error.message}`); + return []; + } + })(); + parseImportsCache.set(path, promise); + return promise; +} diff --git a/src/node.ts b/src/node.ts index cadc12051..ea431e6aa 100644 --- a/src/node.ts +++ b/src/node.ts @@ -8,8 +8,7 @@ import {rollup} from "rollup"; import esbuild from "rollup-plugin-esbuild"; import {prepareOutput} from "./files.js"; import type {ImportReference} from "./javascript/imports.js"; -import {findImports} from "./javascript/imports.js"; -import {parseProgram} from "./javascript/parse.js"; +import {parseImports} from "./javascript/imports.js"; import {parseNpmSpecifier} from "./npm.js"; import {isPathImport} from "./path.js"; import {faint} from "./tty.js"; @@ -56,16 +55,7 @@ async function resolveNodeImportInternal(root: string, packageRoot: string, spec */ export async function resolveNodeImports(root: string, path: string): Promise { if (!path.startsWith("/_node/")) throw new Error(`invalid node path: ${path}`); - try { - const filePath = join(root, ".observablehq", "cache", path); - if (!/\.(m|c)?js$/i.test(path)) return []; // not JavaScript; TODO traverse CSS, too - const source = await readFile(filePath, "utf-8"); - const body = parseProgram(source); - return findImports(body, path, source); - } catch (error: any) { - console.warn(`unable to fetch or parse ${path}: ${error.message}`); - return []; - } + return parseImports(join(root, ".observablehq", "cache", path)); } /** diff --git a/src/npm.ts b/src/npm.ts index 45cb91abb..85455f426 100644 --- a/src/npm.ts +++ b/src/npm.ts @@ -6,7 +6,7 @@ import {simple} from "acorn-walk"; import {rsort, satisfies} from "semver"; import {isEnoent} from "./error.js"; import type {ExportNode, ImportNode, ImportReference} from "./javascript/imports.js"; -import {findImports, isImportMetaResolve} from "./javascript/imports.js"; +import {isImportMetaResolve, parseImports} from "./javascript/imports.js"; import {parseProgram} from "./javascript/parse.js"; import type {StringLiteral} from "./javascript/source.js"; import {getStringLiteralValue, isStringLiteral} from "./javascript/source.js"; @@ -252,30 +252,13 @@ export async function resolveNpmImport(root: string, specifier: string): Promise return `/_npm/${name}@${await resolveNpmVersion(root, {name, range})}/${path.replace(/\+esm$/, "_esm.js")}`; } -const npmImportsCache = new Map>(); - /** * Resolves the direct dependencies of the specified npm path, such as * "/_npm/d3@7.8.5/_esm.js", returning the corresponding set of npm paths. */ export async function resolveNpmImports(root: string, path: string): Promise { if (!path.startsWith("/_npm/")) throw new Error(`invalid npm path: ${path}`); - let promise = npmImportsCache.get(path); - if (promise) return promise; - promise = (async function () { - try { - const filePath = await populateNpmCache(root, path); - if (!/\.(m|c)?js$/i.test(path)) return []; // not JavaScript; TODO traverse CSS, too - const source = await readFile(filePath, "utf-8"); - const body = parseProgram(source); - return findImports(body, path, source); - } catch (error: any) { - console.warn(`unable to fetch or parse ${path}: ${error.message}`); - return []; - } - })(); - npmImportsCache.set(path, promise); - return promise; + return parseImports(await populateNpmCache(root, path)); } /** From 589be7a1ed3d3b309ebcac3fedf8ee7839f63781 Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Wed, 27 Mar 2024 17:50:57 -0700 Subject: [PATCH 08/22] parseImports tests --- docs/node.md | 7 ----- src/javascript/imports.ts | 30 ++++++++++++------ src/node.ts | 12 +++++--- src/npm.ts | 3 +- test/node-test.ts | 51 +++++++++++++++++++++++++++++++ test/{javascript => }/npm-test.ts | 8 ++--- 6 files changed, 86 insertions(+), 25 deletions(-) delete mode 100644 docs/node.md create mode 100644 test/node-test.ts rename test/{javascript => }/npm-test.ts (97%) diff --git a/docs/node.md b/docs/node.md deleted file mode 100644 index da3f599d8..000000000 --- a/docs/node.md +++ /dev/null @@ -1,7 +0,0 @@ -# Hello node - -```js echo -import * as d3 from "d3-array"; - -display(d3); -``` diff --git a/src/javascript/imports.ts b/src/javascript/imports.ts index 364666ed8..8080015fb 100644 --- a/src/javascript/imports.ts +++ b/src/javascript/imports.ts @@ -1,4 +1,5 @@ import {readFile} from "node:fs/promises"; +import {join} from "node:path/posix"; import type {Node} from "acorn"; import type {CallExpression} from "acorn"; import type {ExportAllDeclaration, ExportNamedDeclaration, ImportDeclaration, ImportExpression} from "acorn"; @@ -61,6 +62,7 @@ export function hasImportDeclaration(body: Node): boolean { */ export function findImports(body: Node, path: string, input: string): ImportReference[] { const imports: ImportReference[] = []; + const keys = new Set(); simple(body, { ImportDeclaration: findImport, @@ -70,6 +72,11 @@ export function findImports(body: Node, path: string, input: string): ImportRefe CallExpression: findImportMetaResolve }); + function addImport(ref: ImportReference) { + const key = `${ref.type}:${ref.method}:${ref.name}`; + if (!keys.has(key)) keys.add(key), imports.push(ref); + } + function findImport(node: ImportNode | ExportNode) { const source = node.source; if (!source || !isStringLiteral(source)) return; @@ -78,9 +85,9 @@ export function findImports(body: Node, path: string, input: string): ImportRefe if (isPathImport(name)) { const localPath = resolveLocalPath(path, name); if (!localPath) throw syntaxError(`non-local import: ${name}`, node, input); // prettier-ignore - imports.push({name: relativePath(path, localPath), type: "local", method}); + addImport({name: relativePath(path, localPath), type: "local", method}); } else { - imports.push({name, type: "global", method}); + addImport({name, type: "global", method}); } } @@ -91,9 +98,9 @@ export function findImports(body: Node, path: string, input: string): ImportRefe if (isPathImport(name)) { const localPath = resolveLocalPath(path, name); if (!localPath) throw syntaxError(`non-local import: ${name}`, node, input); // prettier-ignore - imports.push({name: relativePath(path, localPath), type: "local", method: "dynamic"}); + addImport({name: relativePath(path, localPath), type: "local", method: "dynamic"}); } else { - imports.push({name, type: "global", method: "dynamic"}); + addImport({name, type: "global", method: "dynamic"}); } } @@ -112,15 +119,20 @@ export function isImportMetaResolve(node: CallExpression): boolean { ); } +export function isJavaScript(path: string): boolean { + return /\.(m|c)?js$/i.test(path); +} + const parseImportsCache = new Map>(); -export async function parseImports(path: string): Promise { - if (!/\.(m|c)?js$/i.test(path)) return []; // not JavaScript; TODO traverse CSS, too - let promise = parseImportsCache.get(path); +export async function parseImports(root: string, path: string): Promise { + if (!isJavaScript(path)) return []; // TODO traverse CSS, too + const filePath = join(root, path); + let promise = parseImportsCache.get(filePath); if (promise) return promise; promise = (async function () { try { - const source = await readFile(path, "utf-8"); + const source = await readFile(filePath, "utf-8"); const body = parseProgram(source); return findImports(body, path, source); } catch (error: any) { @@ -128,6 +140,6 @@ export async function parseImports(path: string): Promise { return []; } })(); - parseImportsCache.set(path, promise); + parseImportsCache.set(filePath, promise); return promise; } diff --git a/src/node.ts b/src/node.ts index ea431e6aa..24d43ef08 100644 --- a/src/node.ts +++ b/src/node.ts @@ -1,5 +1,5 @@ import {existsSync} from "node:fs"; -import {readFile, writeFile} from "node:fs/promises"; +import {copyFile, readFile, writeFile} from "node:fs/promises"; import {createRequire} from "node:module"; import {dirname, join, relative} from "node:path/posix"; import {pathToFileURL} from "node:url"; @@ -8,7 +8,7 @@ import {rollup} from "rollup"; import esbuild from "rollup-plugin-esbuild"; import {prepareOutput} from "./files.js"; import type {ImportReference} from "./javascript/imports.js"; -import {parseImports} from "./javascript/imports.js"; +import {isJavaScript, parseImports} from "./javascript/imports.js"; import {parseNpmSpecifier} from "./npm.js"; import {isPathImport} from "./path.js"; import {faint} from "./tty.js"; @@ -39,7 +39,11 @@ async function resolveNodeImportInternal(root: string, packageRoot: string, spec promise = (async () => { process.stdout.write(`${spec} ${faint("→")} ${resolution}\n`); await prepareOutput(outputPath); - await writeFile(outputPath, await bundle(pathResolution, root, packageResolution)); + if (isJavaScript(pathResolution)) { + await writeFile(outputPath, await bundle(pathResolution, root, packageResolution)); + } else { + await copyFile(pathResolution, outputPath); + } })(); bundlePromises.set(outputPath, promise); promise.catch(() => {}).then(() => bundlePromises.delete(outputPath)); @@ -55,7 +59,7 @@ async function resolveNodeImportInternal(root: string, packageRoot: string, spec */ export async function resolveNodeImports(root: string, path: string): Promise { if (!path.startsWith("/_node/")) throw new Error(`invalid node path: ${path}`); - return parseImports(join(root, ".observablehq", "cache", path)); + return parseImports(join(root, ".observablehq", "cache"), path); } /** diff --git a/src/npm.ts b/src/npm.ts index 85455f426..7ded8c60d 100644 --- a/src/npm.ts +++ b/src/npm.ts @@ -258,7 +258,8 @@ export async function resolveNpmImport(root: string, specifier: string): Promise */ export async function resolveNpmImports(root: string, path: string): Promise { if (!path.startsWith("/_npm/")) throw new Error(`invalid npm path: ${path}`); - return parseImports(await populateNpmCache(root, path)); + await populateNpmCache(root, path); + return parseImports(join(root, ".observablehq", "cache"), path); } /** diff --git a/test/node-test.ts b/test/node-test.ts new file mode 100644 index 000000000..f0e70b1ea --- /dev/null +++ b/test/node-test.ts @@ -0,0 +1,51 @@ +import assert from "node:assert"; +import {existsSync} from "node:fs"; +import {rm} from "node:fs/promises"; +import {extractNodeSpecifier, resolveNodeImport, resolveNodeImports} from "../src/node.js"; + +describe("resolveNodeImport(root, spec)", () => { + before(async () => { + if (existsSync("docs/.observablehq/cache/_node")) { + await rm("docs/.observablehq/cache/_node", {recursive: true}); + } + }); + it("resolves the version of a direct dependency", async () => { + assert.deepStrictEqual(await resolveNodeImport("docs", "d3-array"), "/_node/d3-array@3.2.4/src/index.js"); + assert.deepStrictEqual(await resolveNodeImport("docs", "mime"), "/_node/mime@4.0.1/dist/src/index.js"); + }); + it("allows entry points", async () => { + assert.deepStrictEqual(await resolveNodeImport("docs", "mime/lite"), "/_node/mime@4.0.1/dist/src/index_lite.js"); + }); + it("allows non-javascript entry points", async () => { + assert.deepStrictEqual(await resolveNodeImport("docs", "glob/package.json"), "/_node/glob@10.3.10/package.json"); + }); + it("does not allow version ranges", async () => { + await assert.rejects(() => resolveNodeImport("docs", "mime@4"), /Cannot find module/); + }); +}); + +describe("resolveNodeImports(root, path)", () => { + before(async () => { + if (existsSync("docs/.observablehq/cache/_node")) { + await rm("docs/.observablehq/cache/_node", {recursive: true}); + } + }); + it("resolves the imports of a dependency", async () => { + assert.deepStrictEqual(await resolveNodeImports("docs", await resolveNodeImport("docs", "d3-array")), [ + { + method: "static", + name: "../../internmap@2.0.3/src/index.js", + type: "local" + } + ]); + }); + it("ignores non-JavaScript paths", async () => { + assert.deepStrictEqual(await resolveNodeImports("docs", await resolveNodeImport("docs", "glob/package.json")), []); + }); +}); + +describe("extractNodeSpecifier(path)", () => { + it("returns the node specifier from the given path", () => { + assert.strictEqual(extractNodeSpecifier("/_node/d3-array@3.2.4/src/index.js"), "d3-array@3.2.4/src/index.js"); + }); +}); diff --git a/test/javascript/npm-test.ts b/test/npm-test.ts similarity index 97% rename from test/javascript/npm-test.ts rename to test/npm-test.ts index 53326ebcf..ec874b007 100644 --- a/test/javascript/npm-test.ts +++ b/test/npm-test.ts @@ -1,8 +1,8 @@ import assert from "node:assert"; -import {extractNpmSpecifier, getDependencyResolver, rewriteNpmImports} from "../../src/npm.js"; -import {fromJsDelivrPath} from "../../src/npm.js"; -import {relativePath} from "../../src/path.js"; -import {mockJsDelivr} from "../mocks/jsdelivr.js"; +import {extractNpmSpecifier, getDependencyResolver, rewriteNpmImports} from "../src/npm.js"; +import {fromJsDelivrPath} from "../src/npm.js"; +import {relativePath} from "../src/path.js"; +import {mockJsDelivr} from "./mocks/jsdelivr.js"; describe("getDependencyResolver(root, path, input)", () => { mockJsDelivr(); From 779e8fa8c9fdefcd7315ac98010bb2316bb58246 Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Wed, 27 Mar 2024 20:27:41 -0700 Subject: [PATCH 09/22] fix windows? --- src/node.ts | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/src/node.ts b/src/node.ts index 24d43ef08..4ed14cd87 100644 --- a/src/node.ts +++ b/src/node.ts @@ -1,12 +1,13 @@ import {existsSync} from "node:fs"; import {copyFile, readFile, writeFile} from "node:fs/promises"; import {createRequire} from "node:module"; -import {dirname, join, relative} from "node:path/posix"; +import op from "node:path"; +import {join} from "node:path/posix"; import {pathToFileURL} from "node:url"; import type {AstNode, OutputChunk, Plugin, ResolveIdResult} from "rollup"; import {rollup} from "rollup"; import esbuild from "rollup-plugin-esbuild"; -import {prepareOutput} from "./files.js"; +import {fromOsPath, prepareOutput} from "./files.js"; import type {ImportReference} from "./javascript/imports.js"; import {isJavaScript, parseImports} from "./javascript/imports.js"; import {parseNpmSpecifier} from "./npm.js"; @@ -21,18 +22,17 @@ const bundlePromises = new Map>(); async function resolveNodeImportInternal(root: string, packageRoot: string, spec: string): Promise { const specifier = parseNpmSpecifier(spec); - const require = createRequire(pathToFileURL(join(packageRoot, "/"))); + const require = createRequire(pathToFileURL(op.join(packageRoot, "/"))); const pathResolution = require.resolve(spec); let packageResolution = pathResolution; do { - const p = dirname(packageResolution); + const p = op.dirname(packageResolution); if (p === packageResolution) throw new Error(`unable to resolve package.json: ${spec}`); packageResolution = p; - } while (!existsSync(join(packageResolution, "package.json"))); - const {version} = JSON.parse(await readFile(join(packageResolution, "package.json"), "utf-8")); - const relativePath = relative(packageResolution, pathResolution); - const resolution = `${specifier.name}@${version}/${relativePath}`; - const outputPath = join(root, ".observablehq", "cache", "_node", resolution); + } while (!existsSync(op.join(packageResolution, "package.json"))); + const {version} = JSON.parse(await readFile(op.join(packageResolution, "package.json"), "utf-8")); + const resolution = `${specifier.name}@${version}/${fromOsPath(op.relative(packageResolution, pathResolution))}`; + const outputPath = op.join(root, ".observablehq", "cache", "_node", resolution); if (!existsSync(outputPath)) { let promise = bundlePromises.get(outputPath); if (!promise) { From 9c696c97f980afcbc66efff4e71a9c5809ae67ee Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Wed, 27 Mar 2024 20:34:16 -0700 Subject: [PATCH 10/22] add logging --- src/node.ts | 14 ++++++++++++-- test/node-test.ts | 6 +++--- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/node.ts b/src/node.ts index 4ed14cd87..f740c33cb 100644 --- a/src/node.ts +++ b/src/node.ts @@ -7,7 +7,7 @@ import {pathToFileURL} from "node:url"; import type {AstNode, OutputChunk, Plugin, ResolveIdResult} from "rollup"; import {rollup} from "rollup"; import esbuild from "rollup-plugin-esbuild"; -import {fromOsPath, prepareOutput} from "./files.js"; +import {fromOsPath, prepareOutput, toOsPath} from "./files.js"; import type {ImportReference} from "./javascript/imports.js"; import {isJavaScript, parseImports} from "./javascript/imports.js"; import {parseNpmSpecifier} from "./npm.js"; @@ -32,7 +32,16 @@ async function resolveNodeImportInternal(root: string, packageRoot: string, spec } while (!existsSync(op.join(packageResolution, "package.json"))); const {version} = JSON.parse(await readFile(op.join(packageResolution, "package.json"), "utf-8")); const resolution = `${specifier.name}@${version}/${fromOsPath(op.relative(packageResolution, pathResolution))}`; - const outputPath = op.join(root, ".observablehq", "cache", "_node", resolution); + const outputPath = op.join(root, ".observablehq", "cache", "_node", toOsPath(resolution)); + console.warn("resolveNodeImportInternal", { + root, + packageRoot, + spec, + packageResolution, + pathResolution, + resolution, + outputPath + }); if (!existsSync(outputPath)) { let promise = bundlePromises.get(outputPath); if (!promise) { @@ -100,6 +109,7 @@ function importResolve(root: string, packageRoot: string): Plugin { async function resolve(specifier: string | AstNode): Promise { if (typeof specifier !== "string") throw new Error(`unexpected specifier: ${specifier}`); if (isPathImport(specifier)) return null; + console.warn("importResolve", {root, packageRoot, specifier}); return {id: await resolveNodeImportInternal(root, packageRoot, specifier), external: true}; } return { diff --git a/test/node-test.ts b/test/node-test.ts index f0e70b1ea..24bbe575e 100644 --- a/test/node-test.ts +++ b/test/node-test.ts @@ -3,7 +3,7 @@ import {existsSync} from "node:fs"; import {rm} from "node:fs/promises"; import {extractNodeSpecifier, resolveNodeImport, resolveNodeImports} from "../src/node.js"; -describe("resolveNodeImport(root, spec)", () => { +describe.only("resolveNodeImport(root, spec)", () => { before(async () => { if (existsSync("docs/.observablehq/cache/_node")) { await rm("docs/.observablehq/cache/_node", {recursive: true}); @@ -24,7 +24,7 @@ describe("resolveNodeImport(root, spec)", () => { }); }); -describe("resolveNodeImports(root, path)", () => { +describe.only("resolveNodeImports(root, path)", () => { before(async () => { if (existsSync("docs/.observablehq/cache/_node")) { await rm("docs/.observablehq/cache/_node", {recursive: true}); @@ -44,7 +44,7 @@ describe("resolveNodeImports(root, path)", () => { }); }); -describe("extractNodeSpecifier(path)", () => { +describe.only("extractNodeSpecifier(path)", () => { it("returns the node specifier from the given path", () => { assert.strictEqual(extractNodeSpecifier("/_node/d3-array@3.2.4/src/index.js"), "d3-array@3.2.4/src/index.js"); }); From c6bce08bccee8790e7db440f5c11a6e1d43acfff Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Wed, 27 Mar 2024 20:38:17 -0700 Subject: [PATCH 11/22] test only windows --- .github/workflows/test.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index cde590bdf..8f34355a2 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -10,8 +10,8 @@ jobs: test: strategy: matrix: - version: [20, 21] - os: [ubuntu-latest, windows-latest] + version: [20] + os: [windows-latest] fail-fast: false runs-on: ${{ matrix.os }} steps: @@ -21,7 +21,7 @@ jobs: node-version: ${{ matrix.version }} cache: yarn - run: yarn --frozen-lockfile - - run: yarn test:coverage + - run: yarn test:mocha:serial - run: yarn test:tsc - run: | echo ::add-matcher::.github/eslint.json From 9bc2ba1bb69226d46308e2e17b95280690dbd91d Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Wed, 27 Mar 2024 20:44:43 -0700 Subject: [PATCH 12/22] =?UTF-8?q?don=E2=80=99t=20resolve=20input?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- src/node.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/node.ts b/src/node.ts index f740c33cb..ff45cf8b8 100644 --- a/src/node.ts +++ b/src/node.ts @@ -84,7 +84,7 @@ async function bundle(input: string, root: string, packageRoot: string): Promise const bundle = await rollup({ input, plugins: [ - importResolve(root, packageRoot), + importResolve(input, root, packageRoot), esbuild({ target: ["es2022", "chrome96", "firefox96", "safari16", "node18"], exclude: [], // don’t exclude node_modules @@ -105,10 +105,10 @@ async function bundle(input: string, root: string, packageRoot: string): Promise } } -function importResolve(root: string, packageRoot: string): Plugin { +function importResolve(input: string, root: string, packageRoot: string): Plugin { async function resolve(specifier: string | AstNode): Promise { if (typeof specifier !== "string") throw new Error(`unexpected specifier: ${specifier}`); - if (isPathImport(specifier)) return null; + if (isPathImport(specifier) || specifier === input) return null; console.warn("importResolve", {root, packageRoot, specifier}); return {id: await resolveNodeImportInternal(root, packageRoot, specifier), external: true}; } From 50c12551275dccebe51f0c7bceb3068cc506ad84 Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Wed, 27 Mar 2024 20:50:52 -0700 Subject: [PATCH 13/22] restore tests --- .github/workflows/test.yml | 6 +++--- src/node.ts | 18 +++++------------- test/node-test.ts | 6 +++--- 3 files changed, 11 insertions(+), 19 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 8f34355a2..cde590bdf 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -10,8 +10,8 @@ jobs: test: strategy: matrix: - version: [20] - os: [windows-latest] + version: [20, 21] + os: [ubuntu-latest, windows-latest] fail-fast: false runs-on: ${{ matrix.os }} steps: @@ -21,7 +21,7 @@ jobs: node-version: ${{ matrix.version }} cache: yarn - run: yarn --frozen-lockfile - - run: yarn test:mocha:serial + - run: yarn test:coverage - run: yarn test:tsc - run: | echo ::add-matcher::.github/eslint.json diff --git a/src/node.ts b/src/node.ts index ff45cf8b8..20d24673e 100644 --- a/src/node.ts +++ b/src/node.ts @@ -33,15 +33,6 @@ async function resolveNodeImportInternal(root: string, packageRoot: string, spec const {version} = JSON.parse(await readFile(op.join(packageResolution, "package.json"), "utf-8")); const resolution = `${specifier.name}@${version}/${fromOsPath(op.relative(packageResolution, pathResolution))}`; const outputPath = op.join(root, ".observablehq", "cache", "_node", toOsPath(resolution)); - console.warn("resolveNodeImportInternal", { - root, - packageRoot, - spec, - packageResolution, - pathResolution, - resolution, - outputPath - }); if (!existsSync(outputPath)) { let promise = bundlePromises.get(outputPath); if (!promise) { @@ -107,10 +98,11 @@ async function bundle(input: string, root: string, packageRoot: string): Promise function importResolve(input: string, root: string, packageRoot: string): Plugin { async function resolve(specifier: string | AstNode): Promise { - if (typeof specifier !== "string") throw new Error(`unexpected specifier: ${specifier}`); - if (isPathImport(specifier) || specifier === input) return null; - console.warn("importResolve", {root, packageRoot, specifier}); - return {id: await resolveNodeImportInternal(root, packageRoot, specifier), external: true}; + return typeof specifier !== "string" || isPathImport(specifier) || specifier === input + ? null // relative import + : /^\w+:/.test(specifier) + ? {id: specifier, external: true} // https: import, e.g. + : {id: await resolveNodeImportInternal(root, packageRoot, specifier), external: true}; // bare import } return { name: "resolve-import", diff --git a/test/node-test.ts b/test/node-test.ts index 24bbe575e..f0e70b1ea 100644 --- a/test/node-test.ts +++ b/test/node-test.ts @@ -3,7 +3,7 @@ import {existsSync} from "node:fs"; import {rm} from "node:fs/promises"; import {extractNodeSpecifier, resolveNodeImport, resolveNodeImports} from "../src/node.js"; -describe.only("resolveNodeImport(root, spec)", () => { +describe("resolveNodeImport(root, spec)", () => { before(async () => { if (existsSync("docs/.observablehq/cache/_node")) { await rm("docs/.observablehq/cache/_node", {recursive: true}); @@ -24,7 +24,7 @@ describe.only("resolveNodeImport(root, spec)", () => { }); }); -describe.only("resolveNodeImports(root, path)", () => { +describe("resolveNodeImports(root, path)", () => { before(async () => { if (existsSync("docs/.observablehq/cache/_node")) { await rm("docs/.observablehq/cache/_node", {recursive: true}); @@ -44,7 +44,7 @@ describe.only("resolveNodeImports(root, path)", () => { }); }); -describe.only("extractNodeSpecifier(path)", () => { +describe("extractNodeSpecifier(path)", () => { it("returns the node specifier from the given path", () => { assert.strictEqual(extractNodeSpecifier("/_node/d3-array@3.2.4/src/index.js"), "d3-array@3.2.4/src/index.js"); }); From 10bded4c6be5beaadecc239273ebcd294e94e895 Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Wed, 27 Mar 2024 20:53:31 -0700 Subject: [PATCH 14/22] build _node --- src/build.ts | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/build.ts b/src/build.ts index de974f52f..e05b972fc 100644 --- a/src/build.ts +++ b/src/build.ts @@ -20,6 +20,7 @@ import {bundleStyles, rollupClient} from "./rollup.js"; import {searchIndex} from "./search.js"; import {Telemetry} from "./telemetry.js"; import {faint, yellow} from "./tty.js"; +import {extractNodeSpecifier} from "./node.js"; export interface BuildOptions { config: Config; @@ -175,10 +176,14 @@ export async function build( // these, too, but it would involve rewriting the files since populateNpmCache // doesn’t let you pass in a resolver. for (const path of globalImports) { - if (!path.startsWith("/_npm/")) continue; // skip _observablehq - effects.output.write(`${faint("copy")} npm:${extractNpmSpecifier(path)} ${faint("→")} `); - const sourcePath = await populateNpmCache(root, path); // TODO effects - await effects.copyFile(sourcePath, path); + if (path.startsWith("/_npm/")) { + effects.output.write(`${faint("copy")} npm:${extractNpmSpecifier(path)} ${faint("→")} `); + const sourcePath = await populateNpmCache(root, path); // TODO effects + await effects.copyFile(sourcePath, path); + } else if (path.startsWith("/_node/")) { + effects.output.write(`${faint("copy")} ${extractNodeSpecifier(path)} ${faint("→")} `); + await effects.copyFile(join(root, ".observablehq", "cache", path), path); + } } // Copy over imported local modules, overriding import resolution so that From 396e16d35f50b0e5902a79868b357b6d95ba3585 Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Wed, 27 Mar 2024 20:54:44 -0700 Subject: [PATCH 15/22] fix import order --- src/build.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/build.ts b/src/build.ts index e05b972fc..5b94604f9 100644 --- a/src/build.ts +++ b/src/build.ts @@ -10,6 +10,7 @@ import {transpileModule} from "./javascript/transpile.js"; import type {Logger, Writer} from "./logger.js"; import type {MarkdownPage} from "./markdown.js"; import {parseMarkdown} from "./markdown.js"; +import {extractNodeSpecifier} from "./node.js"; import {extractNpmSpecifier, populateNpmCache, resolveNpmImport} from "./npm.js"; import {isPathImport, relativePath, resolvePath} from "./path.js"; import {renderPage} from "./render.js"; @@ -20,7 +21,6 @@ import {bundleStyles, rollupClient} from "./rollup.js"; import {searchIndex} from "./search.js"; import {Telemetry} from "./telemetry.js"; import {faint, yellow} from "./tty.js"; -import {extractNodeSpecifier} from "./node.js"; export interface BuildOptions { config: Config; From 6173878bb441a01d8af52365fee3668f78b30e82 Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Wed, 27 Mar 2024 21:15:32 -0700 Subject: [PATCH 16/22] 30s timeout --- package.json | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/package.json b/package.json index d9e23fec9..4f9cae908 100644 --- a/package.json +++ b/package.json @@ -27,8 +27,8 @@ "test": "concurrently npm:test:mocha npm:test:tsc npm:test:lint npm:test:prettier", "test:coverage": "c8 --check-coverage --lines 80 --per-file yarn test:mocha", "test:build": "rimraf test/build && node build.js --sourcemap --outdir=test/build \"{src,test}/**/*.{ts,js,css}\" --ignore \"test/input/**\" --ignore \"test/output/**\" --ignore \"test/preview/dashboard/**\" --ignore \"**/*.d.ts\" && cp -r templates test/build", - "test:mocha": "yarn test:build && rimraf --glob test/.observablehq/cache test/input/build/*/.observablehq/cache && cross-env OBSERVABLE_TELEMETRY_DISABLE=1 TZ=America/Los_Angeles mocha --timeout 5000 -p \"test/build/test/**/*-test.js\"", - "test:mocha:serial": "yarn test:build && rimraf --glob test/.observablehq/cache test/input/build/*/.observablehq/cache && cross-env OBSERVABLE_TELEMETRY_DISABLE=1 TZ=America/Los_Angeles mocha --timeout 5000 \"test/build/test/**/*-test.js\"", + "test:mocha": "yarn test:build && rimraf --glob test/.observablehq/cache test/input/build/*/.observablehq/cache && cross-env OBSERVABLE_TELEMETRY_DISABLE=1 TZ=America/Los_Angeles mocha --timeout 30000 -p \"test/build/test/**/*-test.js\"", + "test:mocha:serial": "yarn test:build && rimraf --glob test/.observablehq/cache test/input/build/*/.observablehq/cache && cross-env OBSERVABLE_TELEMETRY_DISABLE=1 TZ=America/Los_Angeles mocha --timeout 30000 \"test/build/test/**/*-test.js\"", "test:lint": "eslint src test --max-warnings=0", "test:prettier": "prettier --check src test", "test:tsc": "tsc --noEmit", From 50a3e7d11a680fcb2fc24cd8b8de5a1f0809e177 Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Thu, 28 Mar 2024 08:02:14 -0700 Subject: [PATCH 17/22] adopt @rollup/plugin-node-resolve --- src/node.ts | 22 ++++++++++++---------- test/node-test.ts | 10 +++++----- 2 files changed, 17 insertions(+), 15 deletions(-) diff --git a/src/node.ts b/src/node.ts index 20d24673e..5a87cedab 100644 --- a/src/node.ts +++ b/src/node.ts @@ -2,12 +2,13 @@ import {existsSync} from "node:fs"; import {copyFile, readFile, writeFile} from "node:fs/promises"; import {createRequire} from "node:module"; import op from "node:path"; -import {join} from "node:path/posix"; +import {extname, join} from "node:path/posix"; import {pathToFileURL} from "node:url"; +import {nodeResolve} from "@rollup/plugin-node-resolve"; import type {AstNode, OutputChunk, Plugin, ResolveIdResult} from "rollup"; import {rollup} from "rollup"; import esbuild from "rollup-plugin-esbuild"; -import {fromOsPath, prepareOutput, toOsPath} from "./files.js"; +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"; @@ -15,13 +16,13 @@ import {isPathImport} from "./path.js"; import {faint} from "./tty.js"; export async function resolveNodeImport(root: string, spec: string): Promise { - return resolveNodeImportInternal(root, root, spec); + return resolveNodeImportInternal(op.join(root, ".observablehq", "cache", "_node"), root, spec); } const bundlePromises = new Map>(); -async function resolveNodeImportInternal(root: string, packageRoot: string, spec: string): Promise { - const specifier = parseNpmSpecifier(spec); +async function resolveNodeImportInternal(cacheRoot: string, packageRoot: string, spec: string): Promise { + const {name, path = "."} = parseNpmSpecifier(spec); const require = createRequire(pathToFileURL(op.join(packageRoot, "/"))); const pathResolution = require.resolve(spec); let packageResolution = pathResolution; @@ -31,8 +32,8 @@ async function resolveNodeImportInternal(root: string, packageRoot: string, spec packageResolution = p; } while (!existsSync(op.join(packageResolution, "package.json"))); const {version} = JSON.parse(await readFile(op.join(packageResolution, "package.json"), "utf-8")); - const resolution = `${specifier.name}@${version}/${fromOsPath(op.relative(packageResolution, pathResolution))}`; - const outputPath = op.join(root, ".observablehq", "cache", "_node", toOsPath(resolution)); + 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) { @@ -40,7 +41,7 @@ async function resolveNodeImportInternal(root: string, packageRoot: string, spec process.stdout.write(`${spec} ${faint("→")} ${resolution}\n`); await prepareOutput(outputPath); if (isJavaScript(pathResolution)) { - await writeFile(outputPath, await bundle(pathResolution, root, packageResolution)); + await writeFile(outputPath, await bundle(spec, cacheRoot, packageResolution)); } else { await copyFile(pathResolution, outputPath); } @@ -71,11 +72,12 @@ export function extractNodeSpecifier(path: string): string { return path.replace(/^\/_node\//, ""); } -async function bundle(input: string, root: string, packageRoot: string): Promise { +async function bundle(input: string, cacheRoot: string, packageRoot: string): Promise { const bundle = await rollup({ input, plugins: [ - importResolve(input, root, packageRoot), + nodeResolve({browser: true, rootDir: packageRoot}), + importResolve(input, cacheRoot, packageRoot), esbuild({ target: ["es2022", "chrome96", "firefox96", "safari16", "node18"], exclude: [], // don’t exclude node_modules diff --git a/test/node-test.ts b/test/node-test.ts index f0e70b1ea..75bdc2efe 100644 --- a/test/node-test.ts +++ b/test/node-test.ts @@ -10,11 +10,11 @@ describe("resolveNodeImport(root, spec)", () => { } }); it("resolves the version of a direct dependency", async () => { - assert.deepStrictEqual(await resolveNodeImport("docs", "d3-array"), "/_node/d3-array@3.2.4/src/index.js"); - assert.deepStrictEqual(await resolveNodeImport("docs", "mime"), "/_node/mime@4.0.1/dist/src/index.js"); + assert.deepStrictEqual(await resolveNodeImport("docs", "d3-array"), "/_node/d3-array@3.2.4/index.js"); + assert.deepStrictEqual(await resolveNodeImport("docs", "mime"), "/_node/mime@4.0.1/index.js"); }); it("allows entry points", async () => { - assert.deepStrictEqual(await resolveNodeImport("docs", "mime/lite"), "/_node/mime@4.0.1/dist/src/index_lite.js"); + assert.deepStrictEqual(await resolveNodeImport("docs", "mime/lite"), "/_node/mime@4.0.1/lite.js"); }); it("allows non-javascript entry points", async () => { assert.deepStrictEqual(await resolveNodeImport("docs", "glob/package.json"), "/_node/glob@10.3.10/package.json"); @@ -34,7 +34,7 @@ describe("resolveNodeImports(root, path)", () => { assert.deepStrictEqual(await resolveNodeImports("docs", await resolveNodeImport("docs", "d3-array")), [ { method: "static", - name: "../../internmap@2.0.3/src/index.js", + name: "../internmap@2.0.3/index.js", type: "local" } ]); @@ -46,6 +46,6 @@ describe("resolveNodeImports(root, path)", () => { describe("extractNodeSpecifier(path)", () => { it("returns the node specifier from the given path", () => { - assert.strictEqual(extractNodeSpecifier("/_node/d3-array@3.2.4/src/index.js"), "d3-array@3.2.4/src/index.js"); + assert.strictEqual(extractNodeSpecifier("/_node/d3-array@3.2.4/index.js"), "d3-array@3.2.4/index.js"); }); }); From 41925cf26dced21ab7987372896afc085c0c0c84 Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Thu, 28 Mar 2024 08:45:06 -0700 Subject: [PATCH 18/22] more tests --- .github/workflows/test.yml | 6 ++-- src/node.ts | 7 +++-- .../test-browser-condition/browser.js | 1 + .../test-browser-condition/default.js | 1 + .../test-browser-condition/package.json | 9 ++++++ .../test-browser-field/browser.js | 1 + .../node_modules/test-browser-field/main.js | 1 + .../test-browser-field/package.json | 7 +++++ .../node_modules/test-browser-map/browser.js | 1 + .../node_modules/test-browser-map/main.js | 1 + .../test-browser-map/package.json | 9 ++++++ .../test-import-condition/default.cjs | 1 + .../test-import-condition/import.mjs | 1 + .../test-import-condition/package.json | 8 ++++++ .../test-shorthand-export/index.js | 1 + .../test-shorthand-export/package.json | 6 ++++ test/input/packages/package.json | 9 ++++++ test/node-test.ts | 28 ++++++++++++++++--- 18 files changed, 89 insertions(+), 9 deletions(-) create mode 100644 test/input/packages/node_modules/test-browser-condition/browser.js create mode 100644 test/input/packages/node_modules/test-browser-condition/default.js create mode 100644 test/input/packages/node_modules/test-browser-condition/package.json create mode 100644 test/input/packages/node_modules/test-browser-field/browser.js create mode 100644 test/input/packages/node_modules/test-browser-field/main.js create mode 100644 test/input/packages/node_modules/test-browser-field/package.json create mode 100644 test/input/packages/node_modules/test-browser-map/browser.js create mode 100644 test/input/packages/node_modules/test-browser-map/main.js create mode 100644 test/input/packages/node_modules/test-browser-map/package.json create mode 100644 test/input/packages/node_modules/test-import-condition/default.cjs create mode 100644 test/input/packages/node_modules/test-import-condition/import.mjs create mode 100644 test/input/packages/node_modules/test-import-condition/package.json create mode 100644 test/input/packages/node_modules/test-shorthand-export/index.js create mode 100644 test/input/packages/node_modules/test-shorthand-export/package.json create mode 100644 test/input/packages/package.json diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index cde590bdf..8f34355a2 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -10,8 +10,8 @@ jobs: test: strategy: matrix: - version: [20, 21] - os: [ubuntu-latest, windows-latest] + version: [20] + os: [windows-latest] fail-fast: false runs-on: ${{ matrix.os }} steps: @@ -21,7 +21,7 @@ jobs: node-version: ${{ matrix.version }} cache: yarn - run: yarn --frozen-lockfile - - run: yarn test:coverage + - run: yarn test:mocha:serial - run: yarn test:tsc - run: | echo ::add-matcher::.github/eslint.json diff --git a/src/node.ts b/src/node.ts index 5a87cedab..d3969235c 100644 --- a/src/node.ts +++ b/src/node.ts @@ -22,6 +22,7 @@ export async function resolveNodeImport(root: string, spec: string): Promise>(); async function resolveNodeImportInternal(cacheRoot: string, packageRoot: string, spec: string): Promise { + console.log("resolveNodeImportInternal", {cacheRoot, packageRoot, spec}); const {name, path = "."} = parseNpmSpecifier(spec); const require = createRequire(pathToFileURL(op.join(packageRoot, "/"))); const pathResolution = require.resolve(spec); @@ -73,6 +74,7 @@ export function extractNodeSpecifier(path: string): string { } async function bundle(input: string, cacheRoot: string, packageRoot: string): Promise { + console.log("bundle", {input, cacheRoot, packageRoot}); const bundle = await rollup({ input, plugins: [ @@ -98,13 +100,14 @@ async function bundle(input: string, cacheRoot: string, packageRoot: string): Pr } } -function importResolve(input: string, root: string, packageRoot: string): Plugin { +function importResolve(input: string, cacheRoot: string, packageRoot: string): Plugin { async function resolve(specifier: string | AstNode): Promise { + console.log("importResolve", {input, specifier, cacheRoot, packageRoot}); return typeof specifier !== "string" || isPathImport(specifier) || specifier === input ? null // relative import : /^\w+:/.test(specifier) ? {id: specifier, external: true} // https: import, e.g. - : {id: await resolveNodeImportInternal(root, packageRoot, specifier), external: true}; // bare import + : {id: await resolveNodeImportInternal(cacheRoot, packageRoot, specifier), external: true}; // bare import } return { name: "resolve-import", diff --git a/test/input/packages/node_modules/test-browser-condition/browser.js b/test/input/packages/node_modules/test-browser-condition/browser.js new file mode 100644 index 000000000..c508c66ce --- /dev/null +++ b/test/input/packages/node_modules/test-browser-condition/browser.js @@ -0,0 +1 @@ +export const name = "test-browser-condition:browser"; diff --git a/test/input/packages/node_modules/test-browser-condition/default.js b/test/input/packages/node_modules/test-browser-condition/default.js new file mode 100644 index 000000000..54a69edfb --- /dev/null +++ b/test/input/packages/node_modules/test-browser-condition/default.js @@ -0,0 +1 @@ +export const name = "test-browser-condition:default"; diff --git a/test/input/packages/node_modules/test-browser-condition/package.json b/test/input/packages/node_modules/test-browser-condition/package.json new file mode 100644 index 000000000..4b4bc9858 --- /dev/null +++ b/test/input/packages/node_modules/test-browser-condition/package.json @@ -0,0 +1,9 @@ +{ + "type": "module", + "name": "test-browser-condition", + "version": "1.0.0", + "exports": { + "browser": "./browser.js", + "default": "./default.js" + } +} diff --git a/test/input/packages/node_modules/test-browser-field/browser.js b/test/input/packages/node_modules/test-browser-field/browser.js new file mode 100644 index 000000000..1f133ea4c --- /dev/null +++ b/test/input/packages/node_modules/test-browser-field/browser.js @@ -0,0 +1 @@ +export const name = "test-browser-field:browser"; diff --git a/test/input/packages/node_modules/test-browser-field/main.js b/test/input/packages/node_modules/test-browser-field/main.js new file mode 100644 index 000000000..9eb75e987 --- /dev/null +++ b/test/input/packages/node_modules/test-browser-field/main.js @@ -0,0 +1 @@ +export const name = "test-browser-field:main"; diff --git a/test/input/packages/node_modules/test-browser-field/package.json b/test/input/packages/node_modules/test-browser-field/package.json new file mode 100644 index 000000000..62cf3d080 --- /dev/null +++ b/test/input/packages/node_modules/test-browser-field/package.json @@ -0,0 +1,7 @@ +{ + "type": "module", + "name": "test-browser-field", + "version": "1.0.0", + "main": "./main.js", + "browser": "./browser.js" +} diff --git a/test/input/packages/node_modules/test-browser-map/browser.js b/test/input/packages/node_modules/test-browser-map/browser.js new file mode 100644 index 000000000..f6a841dbc --- /dev/null +++ b/test/input/packages/node_modules/test-browser-map/browser.js @@ -0,0 +1 @@ +export const name = "test-browser-map:browser"; diff --git a/test/input/packages/node_modules/test-browser-map/main.js b/test/input/packages/node_modules/test-browser-map/main.js new file mode 100644 index 000000000..5eebd2895 --- /dev/null +++ b/test/input/packages/node_modules/test-browser-map/main.js @@ -0,0 +1 @@ +export const name = "test-browser-map:main"; diff --git a/test/input/packages/node_modules/test-browser-map/package.json b/test/input/packages/node_modules/test-browser-map/package.json new file mode 100644 index 000000000..64724ad51 --- /dev/null +++ b/test/input/packages/node_modules/test-browser-map/package.json @@ -0,0 +1,9 @@ +{ + "type": "module", + "name": "test-browser-map", + "version": "1.0.0", + "main": "./main.js", + "browser": { + "./main": "./browser.js" + } +} diff --git a/test/input/packages/node_modules/test-import-condition/default.cjs b/test/input/packages/node_modules/test-import-condition/default.cjs new file mode 100644 index 000000000..046b6f0dc --- /dev/null +++ b/test/input/packages/node_modules/test-import-condition/default.cjs @@ -0,0 +1 @@ +exports.name = "test-import-condition:default"; diff --git a/test/input/packages/node_modules/test-import-condition/import.mjs b/test/input/packages/node_modules/test-import-condition/import.mjs new file mode 100644 index 000000000..166ffd7f5 --- /dev/null +++ b/test/input/packages/node_modules/test-import-condition/import.mjs @@ -0,0 +1 @@ +export const name = "test-import-condition:import"; diff --git a/test/input/packages/node_modules/test-import-condition/package.json b/test/input/packages/node_modules/test-import-condition/package.json new file mode 100644 index 000000000..b70a1f58e --- /dev/null +++ b/test/input/packages/node_modules/test-import-condition/package.json @@ -0,0 +1,8 @@ +{ + "name": "test-import-condition", + "version": "1.0.0", + "exports": { + "import": "./import.mjs", + "default": "./default.cjs" + } +} diff --git a/test/input/packages/node_modules/test-shorthand-export/index.js b/test/input/packages/node_modules/test-shorthand-export/index.js new file mode 100644 index 000000000..aa9180b6c --- /dev/null +++ b/test/input/packages/node_modules/test-shorthand-export/index.js @@ -0,0 +1 @@ +export const name = "test-shorthand-export"; diff --git a/test/input/packages/node_modules/test-shorthand-export/package.json b/test/input/packages/node_modules/test-shorthand-export/package.json new file mode 100644 index 000000000..0f1a654ea --- /dev/null +++ b/test/input/packages/node_modules/test-shorthand-export/package.json @@ -0,0 +1,6 @@ +{ + "type": "module", + "name": "test-shorthand-export", + "version": "1.0.0", + "exports": "./index.js" +} diff --git a/test/input/packages/package.json b/test/input/packages/package.json new file mode 100644 index 000000000..be86eb3ed --- /dev/null +++ b/test/input/packages/package.json @@ -0,0 +1,9 @@ +{ + "type": "module", + "dependencies": { + "test-browser-condition": "^1.0.0", + "test-browser-field": "^1.0.0", + "test-import-condition": "^1.0.0", + "test-shorthand-export": "^1.0.0" + } +} diff --git a/test/node-test.ts b/test/node-test.ts index 75bdc2efe..818f927db 100644 --- a/test/node-test.ts +++ b/test/node-test.ts @@ -3,11 +3,11 @@ import {existsSync} from "node:fs"; import {rm} from "node:fs/promises"; import {extractNodeSpecifier, resolveNodeImport, resolveNodeImports} from "../src/node.js"; -describe("resolveNodeImport(root, spec)", () => { +describe.only("resolveNodeImport(root, spec)", () => { + const importRoot = "../../input/packages/.observablehq/cache"; before(async () => { - if (existsSync("docs/.observablehq/cache/_node")) { - await rm("docs/.observablehq/cache/_node", {recursive: true}); - } + await rm("docs/.observablehq/cache/_node", {recursive: true, force: true}); + await rm("test/input/packages/.observablehq/cache", {recursive: true, force: true}); }); it("resolves the version of a direct dependency", async () => { assert.deepStrictEqual(await resolveNodeImport("docs", "d3-array"), "/_node/d3-array@3.2.4/index.js"); @@ -22,6 +22,26 @@ describe("resolveNodeImport(root, spec)", () => { it("does not allow version ranges", async () => { await assert.rejects(() => resolveNodeImport("docs", "mime@4"), /Cannot find module/); }); + it("bundles a package with a shorthand export", async () => { + assert.strictEqual(await resolveNodeImport("test/input/packages", "test-shorthand-export"), "/_node/test-shorthand-export@1.0.0/index.js"); // prettier-ignore + assert.strictEqual((await import(`${importRoot}/_node/test-shorthand-export@1.0.0/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/test-import-condition@1.0.0/index.js"); // prettier-ignore + assert.strictEqual((await import(`${importRoot}/_node/test-import-condition@1.0.0/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/test-browser-field@1.0.0/index.js"); // prettier-ignore + assert.strictEqual((await import(`${importRoot}/_node/test-browser-field@1.0.0/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/test-browser-map@1.0.0/index.js"); // prettier-ignore + assert.strictEqual((await import(`${importRoot}/_node/test-browser-map@1.0.0/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/test-browser-condition@1.0.0/index.js"); // prettier-ignore + assert.strictEqual((await import(`${importRoot}/_node/test-browser-condition@1.0.0/index.js`)).name, "test-browser-condition:browser"); // prettier-ignore + }); }); describe("resolveNodeImports(root, path)", () => { From e61a3d227b6fdd161960f664731c974cfcbda04b Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Thu, 28 Mar 2024 08:53:10 -0700 Subject: [PATCH 19/22] fix file path resolution --- src/node.ts | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/node.ts b/src/node.ts index d3969235c..0b68d5a5b 100644 --- a/src/node.ts +++ b/src/node.ts @@ -103,11 +103,12 @@ async function bundle(input: string, cacheRoot: string, packageRoot: string): Pr function importResolve(input: string, cacheRoot: string, packageRoot: string): Plugin { async function resolve(specifier: string | AstNode): Promise { console.log("importResolve", {input, specifier, cacheRoot, packageRoot}); - return typeof specifier !== "string" || isPathImport(specifier) || specifier === input - ? null // relative import - : /^\w+:/.test(specifier) - ? {id: specifier, external: true} // https: import, e.g. - : {id: await resolveNodeImportInternal(cacheRoot, packageRoot, specifier), external: true}; // bare import + return typeof specifier !== "string" || // AST node? + isPathImport(specifier) || // relative path, e.g., ./foo.js + /^\w+:/.test(specifier) || // windows file path, https: URL, etc. + specifier === input // entry point + ? null // don’t do any additional resolution + : {id: await resolveNodeImportInternal(cacheRoot, packageRoot, specifier), external: true}; // resolve bare import } return { name: "resolve-import", From 95005f52f728e31cf01d6184c45892fb6bff12ec Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Thu, 28 Mar 2024 08:56:35 -0700 Subject: [PATCH 20/22] restore tests --- .github/workflows/test.yml | 6 +++--- src/node.ts | 3 --- test/node-test.ts | 2 +- 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 8f34355a2..cde590bdf 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -10,8 +10,8 @@ jobs: test: strategy: matrix: - version: [20] - os: [windows-latest] + version: [20, 21] + os: [ubuntu-latest, windows-latest] fail-fast: false runs-on: ${{ matrix.os }} steps: @@ -21,7 +21,7 @@ jobs: node-version: ${{ matrix.version }} cache: yarn - run: yarn --frozen-lockfile - - run: yarn test:mocha:serial + - run: yarn test:coverage - run: yarn test:tsc - run: | echo ::add-matcher::.github/eslint.json diff --git a/src/node.ts b/src/node.ts index 0b68d5a5b..374522cc3 100644 --- a/src/node.ts +++ b/src/node.ts @@ -22,7 +22,6 @@ export async function resolveNodeImport(root: string, spec: string): Promise>(); async function resolveNodeImportInternal(cacheRoot: string, packageRoot: string, spec: string): Promise { - console.log("resolveNodeImportInternal", {cacheRoot, packageRoot, spec}); const {name, path = "."} = parseNpmSpecifier(spec); const require = createRequire(pathToFileURL(op.join(packageRoot, "/"))); const pathResolution = require.resolve(spec); @@ -74,7 +73,6 @@ export function extractNodeSpecifier(path: string): string { } async function bundle(input: string, cacheRoot: string, packageRoot: string): Promise { - console.log("bundle", {input, cacheRoot, packageRoot}); const bundle = await rollup({ input, plugins: [ @@ -102,7 +100,6 @@ async function bundle(input: string, cacheRoot: string, packageRoot: string): Pr function importResolve(input: string, cacheRoot: string, packageRoot: string): Plugin { async function resolve(specifier: string | AstNode): Promise { - console.log("importResolve", {input, specifier, cacheRoot, packageRoot}); return typeof specifier !== "string" || // AST node? isPathImport(specifier) || // relative path, e.g., ./foo.js /^\w+:/.test(specifier) || // windows file path, https: URL, etc. diff --git a/test/node-test.ts b/test/node-test.ts index 818f927db..6b1ad2f08 100644 --- a/test/node-test.ts +++ b/test/node-test.ts @@ -3,7 +3,7 @@ import {existsSync} from "node:fs"; import {rm} from "node:fs/promises"; import {extractNodeSpecifier, resolveNodeImport, resolveNodeImports} from "../src/node.js"; -describe.only("resolveNodeImport(root, spec)", () => { +describe("resolveNodeImport(root, spec)", () => { const importRoot = "../../input/packages/.observablehq/cache"; before(async () => { await rm("docs/.observablehq/cache/_node", {recursive: true, force: true}); From 8f4746e25e4327ccfe92d2b44bb399a8b92951fa Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Thu, 28 Mar 2024 09:00:26 -0700 Subject: [PATCH 21/22] add missing dependency --- test/input/packages/package.json | 1 + 1 file changed, 1 insertion(+) diff --git a/test/input/packages/package.json b/test/input/packages/package.json index be86eb3ed..ee7376964 100644 --- a/test/input/packages/package.json +++ b/test/input/packages/package.json @@ -3,6 +3,7 @@ "dependencies": { "test-browser-condition": "^1.0.0", "test-browser-field": "^1.0.0", + "test-browser-map": "^1.0.0", "test-import-condition": "^1.0.0", "test-shorthand-export": "^1.0.0" } From add55fca3afe7a4062dfe22cb52a1cb2e06b00a4 Mon Sep 17 00:00:00 2001 From: Mike Bostock Date: Thu, 28 Mar 2024 10:49:03 -0700 Subject: [PATCH 22/22] adopt pkg-dir --- package.json | 1 + src/node.ts | 9 +++------ yarn.lock | 12 ++++++++++++ 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/package.json b/package.json index 4f9cae908..3f6444fa6 100644 --- a/package.json +++ b/package.json @@ -77,6 +77,7 @@ "mime": "^4.0.0", "minisearch": "^6.3.0", "open": "^10.1.0", + "pkg-dir": "^8.0.0", "rollup": "^4.6.0", "rollup-plugin-esbuild": "^6.1.0", "semver": "^7.5.4", diff --git a/src/node.ts b/src/node.ts index 374522cc3..e4c0e6555 100644 --- a/src/node.ts +++ b/src/node.ts @@ -5,6 +5,7 @@ import op from "node:path"; import {extname, join} from "node:path/posix"; import {pathToFileURL} from "node:url"; import {nodeResolve} from "@rollup/plugin-node-resolve"; +import {packageDirectory} from "pkg-dir"; import type {AstNode, OutputChunk, Plugin, ResolveIdResult} from "rollup"; import {rollup} from "rollup"; import esbuild from "rollup-plugin-esbuild"; @@ -25,12 +26,8 @@ async function resolveNodeImportInternal(cacheRoot: string, packageRoot: string, const {name, path = "."} = parseNpmSpecifier(spec); const require = createRequire(pathToFileURL(op.join(packageRoot, "/"))); const pathResolution = require.resolve(spec); - let packageResolution = pathResolution; - do { - const p = op.dirname(packageResolution); - if (p === packageResolution) throw new Error(`unable to resolve package.json: ${spec}`); - packageResolution = p; - } while (!existsSync(op.join(packageResolution, "package.json"))); + const packageResolution = await packageDirectory({cwd: op.dirname(pathResolution)}); + if (!packageResolution) throw new Error(`unable to resolve package.json: ${spec}`); 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)); diff --git a/yarn.lock b/yarn.lock index abe8ed633..f8da13c48 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1813,6 +1813,11 @@ fill-range@^7.0.1: dependencies: to-regex-range "^5.0.1" +find-up-simple@^1.0.0: + version "1.0.0" + resolved "https://registry.yarnpkg.com/find-up-simple/-/find-up-simple-1.0.0.tgz#21d035fde9fdbd56c8f4d2f63f32fd93a1cfc368" + integrity sha512-q7Us7kcjj2VMePAa02hDAF6d+MzsdsAWEwYyOpwUtlerRBkOEPBCRZrAV4XfcSN8fHAgaD0hP7miwoay6DCprw== + find-up@5.0.0, find-up@^5.0.0: version "5.0.0" resolved "https://registry.yarnpkg.com/find-up/-/find-up-5.0.0.tgz#4c92819ecb7083561e4f4a240a86be5198f536fc" @@ -2955,6 +2960,13 @@ picomatch@^2.0.4, picomatch@^2.2.1, picomatch@^2.3.1: resolved "https://registry.yarnpkg.com/picomatch/-/picomatch-2.3.1.tgz#3ba3833733646d9d3e4995946c1365a67fb07a42" integrity sha512-JU3teHTNjmE2VCGFzuY8EXzCDVwEqB2a8fsIvwaStHhAWJEeVd1o1QD80CU6+ZdEXXSLbSsuLwJjkCBWqRQUVA== +pkg-dir@^8.0.0: + version "8.0.0" + resolved "https://registry.yarnpkg.com/pkg-dir/-/pkg-dir-8.0.0.tgz#8f3de8ba83d46b72a05c80bfd4e579f060fa91e2" + integrity sha512-4peoBq4Wks0riS0z8741NVv+/8IiTvqnZAr8QGgtdifrtpdXbNw/FxRS1l6NFqm4EMzuS0EDqNNx4XGaz8cuyQ== + dependencies: + find-up-simple "^1.0.0" + possible-typed-array-names@^1.0.0: version "1.0.0" resolved "https://registry.yarnpkg.com/possible-typed-array-names/-/possible-typed-array-names-1.0.0.tgz#89bb63c6fada2c3e90adc4a647beeeb39cc7bf8f"