Skip to content
New issue

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

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

Already on GitHub? Sign in to your account

node imports #1156

Merged
merged 24 commits into from
Mar 28, 2024
Merged

node imports #1156

merged 24 commits into from
Mar 28, 2024

Conversation

mbostock
Copy link
Member

Fixes #360.

@mbostock mbostock marked this pull request as ready for review March 28, 2024 03:57
@mbostock mbostock requested a review from Fil March 28, 2024 04:18
@mbostock
Copy link
Member Author

mbostock commented Mar 28, 2024

Probably the big thing missing here is that we need to set the “browser” condition instead of the “node” condition for conditional exports. Unfortunately I don’t think Node exposes any functionality for resolving a require with custom conditions; require.resolve always uses the “node” and “import” conditions, which means we’ll then attempt to bundle the wrong entry for the browser. Perhaps we can use @rollup/plugin-node-resolve, or at least inspect its implementation to learn how to do it.

This was referenced Mar 28, 2024
@Fil
Copy link
Contributor

Fil commented Mar 28, 2024

Another case where things don't work:
yarn add --dev @duckdb/[email protected]

```js echo
const duck = import("@duckdb/duckdb-wasm");
```

results in
RuntimeError: Failed to fetch dynamically imported module: http://127.0.0.1:3000/_node/@duckdb/[email protected]/dist/duckdb-node.cjs

the .cjs extension is a first blocker; the console says:
Failed to load module script: Expected a JavaScript module script but the server responded with a MIME type of "application/octet-stream". Strict MIME type checking is enforced for module scripts per HTML spec.

@mbostock
Copy link
Member Author

Yup, that’s the node entry point rather than the browser entry point.

@mbostock
Copy link
Member Author

Okay, now using @rollup/plugin-node-resolve! 🚀

@mbostock
Copy link
Member Author

Argh, it doesn’t work on windows for some reason. ☹️

@mbostock
Copy link
Member Author

Okay, it was my mistake. 😌 Fixed now!

@Fil
Copy link
Contributor

Fil commented Mar 28, 2024

yoohoo!

@Fil
Copy link
Contributor

Fil commented Mar 28, 2024

I can't find how to load the DuckDB wasm this way.

import * as duckdb from "@duckdb/duckdb-wasm";  // works
import duckdb_wasm from '@duckdb/duckdb-wasm/dist/duckdb-mvp.wasm'; // doesn't work

The file is correctly copied from node_modules/@duckdb/duckdb-wasm/dist/duckdb-mvp.wasm to the cache at docs/.observablehq/cache/_node/@duckdb/[email protected]/dist/duckdb-mvp.wasm but the browser complains that its mime-type is wasm not javascript.

Console: "Expected a JavaScript module script but the server responded with a MIME type of "application/wasm"."

@mbostock
Copy link
Member Author

You can’t import a .wasm; you use import.meta.resolve instead.

@Fil
Copy link
Contributor

Fil commented Mar 28, 2024

Yes I was just getting to the point where this seems to work:

```js echo
import * as duckdb from "@duckdb/duckdb-wasm";
const MANUAL_BUNDLES = {
    mvp: {
        mainModule: import.meta.resolve('@duckdb/duckdb-wasm/dist/duckdb-mvp.wasm'),
        mainWorker: import.meta.resolve('@duckdb/duckdb-wasm/dist/duckdb-browser-mvp.worker.js'),
    },
    eh: {
        mainModule: import.meta.resolve('@duckdb/duckdb-wasm/dist/duckdb-eh.wasm'),
        mainWorker: import.meta.resolve('@duckdb/duckdb-wasm/dist/duckdb-browser-eh.worker.js'),    },
};
// Select a bundle based on browser checks
const bundle = await duckdb.selectBundle(MANUAL_BUNDLES);
// Instantiate the asynchronous version of DuckDB-Wasm
const worker = new Worker(bundle.mainWorker);
const logger = new duckdb.ConsoleLogger();
const db = new duckdb.AsyncDuckDB(logger, worker);
await db.instantiate(bundle.mainModule, bundle.pthreadWorker);
```

I'll approve the PR and continue tinkering with DuckDB :)

@mbostock
Copy link
Member Author

Seems to work! I tested this:

```js echo
const connection = await db.connect();
try {
  display(await connection.query(`SELECT 1 + 2`));
} finally {
  await connection.close();
}
```

@mbostock
Copy link
Member Author

Confirmed that this works with Yarn’s resolutions, so you can for example tell @duckdb/duckdb-wasm to use the latest version of apache-arrow like so in package.json:

  "resolutions": {
    "@duckdb/duckdb-wasm/apache-arrow": "15.0.2"
  }

src/node.ts Outdated
Comment on lines 26 to 34
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 {version} = JSON.parse(await readFile(op.join(packageResolution, "package.json"), "utf-8"));
Copy link
Member Author

@mbostock mbostock Mar 28, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For posterity, here is an alternative approach to finding the package.json that I discovered:

nodejs/node#33460 (comment)

I guess there are some cases of a “nested” package.json that our approach doesn’t handle; the linked approach is to trim the relative path to the entry point from the fully-resolved path. I think our approach is fine for now, though. There’s also this:

https://github.com/sindresorhus/package-up

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I’ve switched this branch to use pkg-dir.

@mbostock mbostock enabled auto-merge (squash) March 28, 2024 17:51
@Fil
Copy link
Contributor

Fil commented Mar 28, 2024

we need to document! it's on https://observablehq.com/framework/getting-started

@mbostock mbostock merged commit 0e6919d into main Mar 28, 2024
4 checks passed
@mbostock mbostock deleted the mbostock/node-modules branch March 28, 2024 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Import from node_modules with bare module specifiers
2 participants