Incorrect "exports" in package.json

[kettanaito]

• Related to #30

First of all, thank you for a great package!

What

This library currently points to index.web.js for the import condition. That is incorrect. What ends up happening is that Node.js ESM (which resolves the import condition) tries to load the browser build of this package, resulting in a failure.

This package, basically, assumes that import is used only in the browser. Luckily, we can use ESM in Node.js now. I want to be able to use this package there too!

Proposal

Instead, provide environment-based export conditions to correctly distribute this package:

  "exports": {
    ".": {
      "types": "./index.d.ts",
-     "import": "./index.web.js",
+     "node": {
+       "import": "./index.node.mjs",
+       "require": "./index.node.js"
+     },
      "browser": "./index.browser.js",
      "require": "./index.node.js",
      "default": "./index.web.js"
    }
  },

Note that you can nest import/require/default/etc. export conditions inside an environment condition. We've been using that to a great effect in MSW for years now (ref).

Bundling

The node.import condition must points to an ESM build of this package for Node.js. If the said build target doesn't exist, let's add it.

Additional context

I am willing to open a pull request for this.

[pimterry]

Nice, that makes sense! Yes, I'd happily accept PRs for this. It would be good to extend the test setup to cover this import case as part of that, to guarantee that this is working correctly in both this new setup and the old approaches.

Judging by my previous comment in #30 there might be some possible issues here, since the wasm-pack web target is explicitly intended for a browser ESM environment, not a Node environment. https://rustwasm.github.io/docs/wasm-bindgen/reference/deployment.html suggests that's a separate experemintal-nodejs-module target that should really be used for this case, but that all looks a bit out of date (warning that Node 8 is the minimum supported version is definitely a red flag!) and I haven't been keeping up with recent developments here.

In practice, if it fully works with modern node with this setup, and it doesn't break any of the tests etc running against older node, that's fine with me. This is almost certainly an interesting topic to bring up with the upstream wasm packaging tools though, since this will be a widespread thing in all "using Rust via WASM in Node" deployments, and if there is a single output format to rule them all then I'm sure everybody would love to explicitly migrate towards that.

[kettanaito]

That sounds great.

It would be good to extend the test setup to cover this import case as part of that

Yeah, this is always tricky. I wish there was better tooling around module compatibility testing. We have an entire test setup designated just for that in MSW. It's actually great, just not reusable.

since the wasm-pack web target is explicitly intended for a browser ESM environment

This should still be okay as long as brotli-wasm doesn't rely on any browser APIs (which I believe it doesn't).

[vitch]

I just ran into a situation where I needed this...

I'm using brotli-wasm (via hono-compress) in a bun project which I'm compiling to a single file executable.

I was getting the following error when I tried to run my executable:

ENOENT: no such file or directory, open '/$bunfs/root/brotli_wasm_bg.wasm'

After going down a lot of dead ends I put together a simple reproduction and figured out that the cause of the problem was that I was accidentally importing the web version of the library due to this issue. And bun didn't recognise the way the wasm was loaded there.

I had to tweak the suggested patch slightly:

      "node": {
        "import": "./index.node.js",
        "require": "./index.node.js"
      },

(index.node is now .js rather than .mjs?).

I'd love to see a fix for this merged but in the meantime I'm leaving the comment above to help anyone else who encounters the same problem and is searching for the bun error message :)

[LeBombusP]

I'd love to see a fix for this merged but in the meantime I'm leaving the comment above to help anyone else who encounters the same problem and is searching for the bun error message :)

Thanks, you saved me from a huge headache.

[pimterry]

As I've mentioned above and in #39, PRs to fix this are welcome! Currently all my code uses the existing entrypoints just fine, so I don't need any of this personally and I'm not going to implement this myself, but if you are interested in it I'm very happy to accept changes to broaden support.

When fixing package.json & entrypoint issues, the only requirements are to not break the existing API, and to add new tests that cover this runtime (e.g. a new test:node-esm script in package.json that runs the existing tests with Node via ESM, or similar). That's where #39 is stalled currently.