httptoolkit / brotli-wasm

A reliable compressor and decompressor for Brotli, supporting node & browsers via wasm
Apache License 2.0
264 stars 21 forks source link

Broken exports in node mjs #30

Open posva opened 1 year ago

posva commented 1 year ago

The current exports are invalid because the package is a commonjs (default type in package.json) but the import field of exports points to a file that uses modules and has a js extension. Using an mjs extension should be enough to fix this.

Here is how to reproduce the issue:

package.json

{
  "name": "brotli-test",
  "version": "0.0.0",
  "type": "module",
  "private": true,
  "main": "index.mjs",
  "scripts": {
    "start": "node index.mjs"
  },
  "dependencies": {
    "brotli-wasm": "^2.0.1"
  }
}

index.mjs

import { compress } from 'brotli-wasm'

run npm start

myl7 commented 1 year ago

This is expected.

brotli-wasm exports CommonJS API for the node environment. So if you are using CommonJS, everything is fine. If you are using ESM, via either .mjs or "type": "module", Node will (seems to, at least wrongly) choose the export of the web environment, making it broken.

First require brotli-wasm in CommonJS and then import the local file in ESM works. Examples are available in https://github.com/myl7/brotli-wasm/tree/example-node .

@pimterry How about adding the examples to this repo?

posva commented 1 year ago

Node will (seems to, at least wrongly) choose the export of the web environment,

FYI Node is picking up the correct export based on this package.json exports. This is not a node bug. Making the extension of the file mjs should make it compatible by letting node know it's a module syntax in a commonjs project.

I think another way to fix this is to provide a node entry like in https://github.com/vuejs/pinia/blob/v2/packages/pinia/package.json#L13

BTW the types also have a few issues: https://arethetypeswrong.github.io/?p=brotli-wasm%402.0.1

myl7 commented 1 year ago

Sorry for the above arbitrary assertion. I does not mean Node makes mistakes (and it is not wrong of course), but rather want to say the import entry is actually for the web environment.

Only modifying the extension to mjs is not enough. Since it is for the web, there is fetch inside it and Node would complain TypeError: fetch failed if only modify that. And according to https://nodejs.org/api/packages.html#conditional-exports , there is not an entry just for the web.

anywhichway commented 1 year ago

For a work around, I did the below. I think it is what myl7 was alluding too, but the examples at the link provided did not make it this simple.

  1. Created a file brotli.cjs:
const brotli = require('brotli-wasm');
module.exports = brotli;
  1. Imported the file into my app:
import brotli from './brotli.cjs';
const {compress,decompress} = brotli;
pimterry commented 1 year ago

From this description, I think the real problem here is inherited, rather than intentional - the current three export formats (commonjs Node, native browser, browser bundler) are exactly the three primary targets provided by wasm-pack.

They have more background on that here: https://rustwasm.github.io/docs/wasm-bindgen/reference/deployment.html. Wasm-pack does basically all the heavy lifting to turn this Rust code in this repo into something that you can consume from JavaScript (and AFAIK it's the standard tool to do this) and we're mostly just following their recommendations in terms of build setup & supported targets.

Unfortunately, we're clearly not mapping these targets right into the export map. Really, according to wasm-pack, we should not provide ESM in any Node.js environments at all.

@posva if you do make the changes you suggest, and use the module in node via the index.web.js entrypoint (the one used by the import export) does it actually all work correctly for you? Can you run the tests for example? The idea of these different targets in wasm-pack is that small differences in how WASM is handled etc mean that each environment uses a slightly different wrapper and setup, and their docs imply that this build output shouldn't work in Node.js at all.

If it does, that's very interesting, and a) wasm-pack should update their docs & target names, since the ecosystem has now improved enough that they can (at least in this case) ship a single web+node bundle, b) we could plausibly find ways to align these and use a single web module for both Node & browsers in one go (which would be great!).

It depends how widely this is supported though I suppose, especially since part of the value of this module is backporting brotli to old (now very old) Node versions. If there are Node versions that do support ESM but don't compatibly run the entire web build, then we'd probably need to either disable ESM for Node entirely instead, or create an entrypoint that checks the Node version and does something more complicated (eugh).