Incorrect "exports" in package.json

[kettanaito]

• Related to Broken exports in node mjs #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).