Add exports field to subpackages to align module resolution with the root level package.json

[sheremet-va]

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

Describe the bug
While working on vitest-dev/vitest#737 I found that preact/hooks module entry points to cjs package instead of hooks.mjs. Is this intentional?

If I change it to .mjs everything works fine. Usually Vitest doesn't resolve what is stored in main field, but it is configurable with resolve.mainFields Vite option (this is what Svelte is doing).

So, I guess Vite prioritizes closest package.json resolvings instead of root level package exports.

Might be related: preactjs/preset-vite#52

To Reproduce

Reproduction: https://github.com/molily/vitest-test/tree/preact-and-svelte

1. Install packages
2. Run pnpm run vitest
3. See error, because it imports the wrong preact lib

Expected behavior
No error

[JoviDeCroock]

That is intentional, we don't use type module so main is cjs

[sheremet-va]

main as cjs is fine, but your module is also pointing to cjs inside a local package json. But you point to the correct one in the root level package.json. What is the purpose of this package.json?

[danielweck]

In my codebase I delete node_modules preact/hooks package.json to fix this.

[danielweck]

see vitest-dev/vitest#747 (comment)

[sheremet-va]

see vitest-dev/vitest#747 (comment)

I am aware of the workaround 😄

I'm interested to get feedback from maintainers if this behaviour is desirable.

[JoviDeCroock]

@developit is your best bet for that, was before me joining

[marvinhagemeister]

FYI: We added those package.json files way before ESM was in node or the exports field was a thing. This was the only way to allow multiple entries into a package so that you could do preact/hooks instead of preact/hooks/dist/hooks.module.js.

These days with the exports field being standard and well supported those package.json files have outlived their usefulness.

The criteria for removing them here are:

• Check if microbundle supports exports. If not I'd be totally fine with a handrolled rollup config file
• Do CDNs support the exports field?

Either way I'd love to see them be removed too.

[rschristian]

main as cjs is fine, but your module is also pointing to cjs inside a local package json. But you point to the correct one in the root level package.json. What is the purpose of this package.json?

module does point to a ESM entry point, just with .js as the file extension. If Vitest is enforcing Node semantics (.cjs/.mjs) upon "module", that's likely to run into many issues in the ecosystem at large (as "module" is far, far older than those semantics) and I'd say is incorrect behavior. There's no reason why that field needs to adhere to Node file extension semantics as it's always ESM.

@marvinhagemeister microbundle's support for "exports" is a bit limited and even more so in the context of something like Preact here will all of the entry points. I don't imagine it'll be a great fit (at least not without some post-build scripts) in the immediate future.

[sheremet-va]

If Vitest is enforcing Node semantics

As I said, Vitest doesn't enforce it by default because of the reasons you described, but users can configure that.

I'm just trying to find a solution here for the end user. Maybe we can at least have both module field and exports field in subpackages (or how would you call it?)? - You can use browser field for .js ESM that way, and .mjs for import (you already bundle that file). My issue with this is that preact itself supports both ESM with Node semantics and commonjs, but it's subpackages don't have this support for some reason.

[rschristian]

As I said, Vitest doesn't enforce it by default because of the reasons you described, but users can configure that.

That doesn't sound correct. Users aren't configuring whether or not Node's extension semantics should be enforced, but what entry conditions are able to be used. If consuming a valid & spec compliant "module" causes issues in Vitest, it's not really an issue with Preact.

While we do probably want to revise the setup here, that would only (maybe) fix the issue for Preact. Any usage of "module": "x.js" in any other package would see the same problem occur if "module" was to be consumed.

[sheremet-va]

That doesn't sound correct

Importing with module field doesn't throw any errors, it works as expected, but it imports from commonjs preact entry instead of ESM one.

If consuming a valid & spec compliant

Your module is not valid and spec complaint to Node, to bundlers - yes. I am ok with module being only spec complaint to bundlers, but at least add correct exports field.

in any other package would see the same problem occur

That is correct. Usually this packages will also have an exports field alongside module and main - if not, we create an issue in this repositories. We already did the same for preact (this is the second one). I wouldn't ask for it, if you didn't already do this for your top level package.json.

In this specific issue problem comes from Svelte that adds module to the module resolution, but they usually process this modules themselves.

[rschristian]

Importing with module field doesn't throw any errors, it works as expected, but it imports from commonjs preact entry instead of ESM one.

Sorry, I don't follow. Are you stating that Vitest falls back to "main" instead of "module"?

Your module is not valid and spec complaint to Node, to bundlers - yes.

Node has no way of consuming "module", so bundlers are all that matters.

The entry predates Node's semantics by quite a bit, hence why Node moved to "exports" rather than adopting "module", as there's no backwards compatible way of enforcing file extensions on existing fields.

That is correct. Usually this packages will also have an exports field alongside module and main - if not, we create an issue in this repositories. We already did the same for preact (this is the second one).

Better solution long-term would probably be to adhere to the "module" spec, no?

[sheremet-va]

Sorry, I don't follow. Are you stating that Vitest falls back to "main" instead of "module"?

No. I am saying that preact/hooks/hooks.module.js imports preact/index.js instead of preact/index.mjs, as it should, if it was a valid Node ESM entry. (it is not, and it's ok, as long as you provide correct exports)

The entry predates Node's semantics by quite a bit, hence why Node moved to "exports" rather than adopting "module", as there's no backwards compatible way of enforcing file extensions on existing fields.

This is why I am asking to also add exports field (as you already did for preact). It has higher priority.

Better solution long-term would probably be to adhere to the "module" spec, no?

What do you mean? We are trying to focus on performans, so we don't run external libraries themselves, but delegate it to Node. We also don't have a resolution algorithms, it is inside Vite, so we don't know if it's a module field or the path came from exports - and I don't see why we should?

I also think "better solution long-term" is for libraries to have correct bundlers fields and exports field, instead of trying to patch module to be consumed correctly by Node.

[rschristian]

No. I am saying that preact/hooks/hooks.module.js imports preact/index.js instead of preact/index.mjs, as it should, if it was a valid Node ESM entry. (it is not, and it's ok, as long as you provide correct exports)

Gotcha!

What do you mean? We are trying to focus on performans, so we don't run external libraries themselves, but delegate it to Node.

I mean that if you're going to allow users to consume from "module", you cannot rely on Node to do it. It's a spec and a standard outside of Node, and for very good reason. Performance is great, but the best performing way to support "module" unfortunately is to write your own resolution. It's unfortunate, but the whole ESM/CJS schism was never gonna be clean sadly.

We also don't have a resolution algorithms, it is inside Vite

Sorry, take my comments re:Vitest with a big grain of salt; I don't know where/how it connects up to Vite. I use neither tool. Whichever is resolving "module" and treating its import of preact as CJS is incorrect.

I also think "better solution long-term" is for libraries to have correct bundlers fields and exports field, instead of trying to patch module to be consumed correctly by Node.

preact/hooks's "module" entry is correct though. I don't disagree, relying on modern bundlers to resolve against the root package.json might be a bit fiddly (and could do with an update), but the setup here is correct.

I meant "better solution long-term" in reference to the entire Node ecosystem. "module" has been in use like 5-6-ish years now, while "exports" is only the past couple. It's going to be a huge headache for you guys to keep notifying upstream packages (some of which are long without maintainers) that they need to add "exports" as Vitest cannot use "module" correctly (be that a Vite issue or what have you). There's probably tens of thousands^{[citation needed]} of modules you might see this issue in.

[sheremet-va]

It's going to be a huge headache for you guys to keep notifying upstream packages

Vite team does this since the start, we are just helping with that. I thinks it's better that way, because it also affects SSR.

preact/hooks's "module" entry is correct though

If it's correct then why preact references preact/hook/hook.mjs instead of the "correct" module entry? (This is rhetoric question). I am just asking you to be consistent with your own package. preact root package.json has an exports field pointing to import, but your inner package points to module, so when module resolution kicks in, preact's hooks are resolved to valid Node ESM, but inner package resolves to Browser ESM.

So I am just asking, can you also addexports to your inner "packages", so module resolution stays the same between your package.json files? This is definitely not a problem with Vite/Vitest/UFO/NASA or Secret Government.

that they need to add "exports" as Vitest cannot use "module" correctly

As I said before this is rarely an issue, because we don't process module by default.

[sheremet-va]

To conclude, your package.json in subpackages is incorrect, because it contradicts with the root level package.json. This confuses module resolution algorithm, because the closest package.json has a higher priority. This is not a problem with Vitest, but also with Vite (this is the reason for preactjs/preset-vite#52).

If you have the exports field in all the subpackages, this won't happen.

[rschristian]

Vite team does this since the start, we are just helping with that. I thinks it's better that way, because it also affects SSR.

Well, fair enough. Good luck! Honestly I do hope that continues to go well.

If it's correct then why preact references preact/hook/hook.mjs instead of the "correct" module entry? (This is rhetoric question). I am just asking you to be consistent with your own package.

You may think that's a rhetorical question, but the reality is that they're both correct as they do different things and need to abide by different specs. "exports" in the root package.json needs to abide by Node's .mjs/.cjs semantics for import, node, and default conditions; "module" on the other hand should not abide by these semantics, as it breaks backwards compatibility. Older tools cannot properly handle .mjs, so that field needs to stay as .js to ensure no breakages.

It's not "inconsistent" as they're two different things. They aren't supposed to match necessarily* ("type": "module" would allow them to align).

preact root package.json has an exports field pointing to import, but your inner package points to module,

I imagine this was just a case of every other tool using the root root package.json (and only that) so that there was no need to add "exports" to the nested. Mostly there for legacy reasons I believe, perhaps CDNs (as mentioned above)?

This is definitely not a problem with Vite/Vitest/UFO/NASA or Secret Government.

I'm not sure what I've said that makes you think I'm starting a conspiracy theory(?) here? Apologies, that's certainly not my intention.

As I said before this is rarely an issue, because we don't process module by default.

Sure, and that's fair enough of course. Just warning that an incorrect handling of "module", even if rarely handled, will probably be easier to fix than going to every upstream package. Just my educated guess though, happy to be wrong (as that means the Node ecosystem is moving faster than my experience has shown).

[sheremet-va]

It's not "inconsistent" as they're two different things.

I don't mean inconsistency in that way. I know perfectly well why you have both module and exports.import with different files there. Inconsistency comes when you don't have the same in your subpackages.

[rschristian]

I don't mean inconsistency in that way. I know perfectly well why you have both module and exports.import with different files there. Inconsistency comes when you don't have the same in your subpackages.

Sorry, I thought you were referring to the previous sentence with that comment:

If it's correct then why preact references preact/hook/hook.mjs instead of the "correct" module entry?