package.json#exports.browser does not adhere to Node module type semantics

[mcous]

• Check if updating to the latest Preact version resolves the issue

Describe the bug

Hello! I'm coming here from testing-library/svelte-testing-library#381, where a user reported interop issues when using preact together with our svelteTesting Vite plugin. Our plugin makes one significant change to the Vite config: it adds browser to resolve.conditions to ensure Svelte's browser code is imported.

This also causes Preact's browser export - ./dist/preact.module.js - to be used:

{
  "exports": {
    ".": {
      "types": "./src/index.d.ts",
      "browser": "./dist/preact.module.js",
      "umd": "./dist/preact.umd.js",
      "import": "./dist/preact.mjs",
      "require": "./dist/preact.js"
    }
  }
}

The bug is:

• preact.module.js is ESM
• Preact's package.json does not specific type, so "type": "commonjs" is implied
• Because preact.module.js ends in .js rather than .mjs, it is treated as CommonJS

At worst, this produces a fairly inscrutable error:

SyntaxError: Named export 'options' not found. The requested module 'preact' is a
CommonJS module, which may not support all module.exports as named exports.
CommonJS modules can always be imported via the default export, for example using:

import pkg from 'preact';
const { options } = pkg;

With enough fiddling, I can get Vite to complain in this manner:

Module /Users/mc/cloned/vitest-preact-svelte/node_modules/preact/dist/preact.module.js:2 
seems to be an ES Module but shipped in a CommonJS package. You might want to create an 
issue to the package "preact" asking them to ship the file in .mjs extension or add
"type": "module" in their package.json.

If I go in and manually rename files to .module.mjs and update the package.json, everything works happily.

To Reproduce

I can put together a minimal reproduction if required - just let me know. There is also this repro from the original reporter: https://github.com/molily/vitest-preact-svelte

Expected behavior

I should be able to load the browser copy of Preact in Vitest without issue

[rschristian]

It's an issue others have mentioned before, but ultimately, this is a completely valid configuration we have here and it's other tools that are causing the issue.

Using the exports.browser condition in Node is completely misusing it and applying Node's file extension semantics to it (a module not meant to be used in Node at all) is incorrect. Vitest (as well as Jest w/ JSDOM) should not be using a module meant for use in the browser anywhere but a browser. Our browser build adheres only to the conditions of the intended runtime environment, not Node or any other runtime.

Additionally, altering package.json#exports is a breaking change and one that cannot occur outside of a major, so we can't do much here until v11 (though I still think we shouldn't really do anything here; it's a bug/misuse in other tools).

Additional references: preactjs/jest-preset-preact#13 (comment), #3634

[mcous]

Thanks for the detailed response and additional tickets! Apologies that my cursory search didn't turn them up. I definitely see (and agree with much of) where you're coming from here.

Out of curiosity, what are the differences between the preact.module.js and preact.mjs? It doesn't look to me like there are any, based on a quick diff.

It's an issue others have mentioned before, but ultimately, this is a completely valid configuration we have here and it's other tools that are causing the issue.

I might quibble with "completely valid configuration" here: package.json is consumed by Node / bundlers, not browsers directly. This package.json says: "here is a browser export, and it is CJS, according to the rules of exports conditions and the type field." Setting aside Vitest and the act of importing the browser bundle in Node, this seems like an incorrect configuration.

For example, I think this would be a more accurate (completely untested) export that says, "here is a browser export, and it is in ESM format:"

{
  "exports": {
    ".": {
      "types": "./src/index.d.ts",
      "browser": {
        "import": "./dist/preact.module.js",
      },
      "umd": "./dist/preact.umd.js",
      "import": "./dist/preact.mjs",
      "require": "./dist/preact.js"
    },
  }
}

Vitest (as well as Jest w/ JSDOM) should not be using a module meant for use in the browser anywhere but a browser. Our browser build adheres only to the conditions of the intended runtime environment.

I agree with most of this, and I certainly don't think Preact or any module should, when providing a browser export, expect it to run anywhere other than the browser. Vitest, to its credit, will prioritize Node exports for packages. The user must actively opt-in to resolving browser exports during tests.

But I think a user should be able to make that choice! For a lot of us, the upsides of running browser-intended code against a fake DOM outweigh the downsides of using an inaccurate environment. So, while no module should go out of their way to support this sort of runtime, I do think a module should accurately report its exports so that a user can choose to do this and take on whatever risk they feel is appropriate.

Additionally, altering package.json#exports is a breaking change and one that cannot occur outside of a major, so we can't do much here until v11 (though I still think we shouldn't really do anything here; it's a bug/misuse in other tools).

Yeah, definitely hear this. I saw in one of the threads you linked that this won't be an issue in v11, do you know if that's true? I saw that the v11 branch has no browser export conditions, but I don't know if that's up to date.

I wonder, but do not know, if something like my example above would be non-breaking. I'm not sure what tools are currently relying on the browser export nor what tools y'all expect to be relying on it

[rschristian]

Out of curiosity, what are the differences between the preact.module.js and preact.mjs?

None at the moment, and I think the latter is even generated from the former. We could make use of this but haven't (not sure if we will either).

I might quibble with "completely valid configuration" here: package.json is consumed by Node / bundlers, not browsers directly.

package.json is metadata for JS packages; this could be used by runtimes, bundlers, or CDNs. The last two don't need to adhere to Node semantics.

Node's docs pretty specifically state it does not govern any keys/conditions that Node itself does not consume which was a key design decision from the start. Additional keys are, per the spec, completely valid and can have their own semantics. At the moment, we do happen to make use of this.

and it is CJS, according to the rules of exports conditions and the type field.

browser isn't beholden to the type field (or shouldn't be, rather) as it isn't ever consumed by Node. File extension semantics simply don't apply to it, same as module (it is always ESM).

I do think a module should accurately report its exports so that a user can choose to do this and take on whatever risk they feel is appropriate.

I mean, we do, for the intended environment. If you want to use something in an env it's not meant to run in, there be dragons and all that. .js ESM is totally valid in the browser.

I saw in one of the threads you linked that this won't be an issue in v11, do you know if that's true? I saw that the v11 branch has no browser export conditions, but I don't know if that's up to date.

Can't say for sure; v11 has no planned release date at this time (we've gotten a lot more use out of v10 than we originally expected, delaying the need for a new major) and so we might revise our export conditions further before release. The v11 is indeed "up to date", but early beta-ish and things may change.

if something like my example above would be non-breaking.

It might, but with how fragile exports is in general, I wouldn't really trust it. Some env is (sadly) likely to fall over with any change. I think the general community consensus is still to avoid touching between majors if at all possible which is why I think our position has to be "can't fix" for now.