pmndrs / valtio

🧙 Valtio makes proxy-state simple for React and Vanilla
https://valtio.dev
MIT License
9.16k stars 257 forks source link

Unsupported export map resolution #168

Closed lxsmnsyc closed 3 years ago

lxsmnsyc commented 3 years ago

Currently, the package' export map looks like this:

{
  "types": "./utils.d.ts",
  "module": "./esm/utils.js",
  "default": "./utils.js"
}

However, most bundlers that uses the export map (e.g. Vite, ESBuild, Skypack) uses the import/require resolution (see here) and NodeJS supports this natively. Currently, the behavior is that the CJS bundle gets picked up even if the bundle target is ESM.

Suggested solution would be to use import/require over module. (We can still retain "default")

dai-shi commented 3 years ago

Specifying import seems to cause Dual package hazard.

This isn't very nice. Suggestions are welcome. Past discussions are scattered around unfortunately. We would like to keep all zustand/jotai/valtio consistent. zustand does probably not have the dual package issue. jotai and valtio do have the dual package issue because they rely on symbols.

It would be nice if we can reproduce the dual package problem and try to find a working config.

dai-shi commented 3 years ago

110 is also an issue, we'd like to solve.

lxsmnsyc commented 3 years ago

Thanks for linking that issue.

image

It seems that the esm module is attempting to re-import the CJS bundle, which seems to be the issue for Skypack (as Skypack uses the import/require resolution). As we know, Node, by default, resolves to the default/require resolution if the package is not defined with "type": "module" and this could be the cause of what seems to be ESM and CJS mixing up. We could probably solve this by opting into using relative imports instead of node_modules resolution (e.g. ./vanilla instead of valtio/vanilla). Less roundtrips, too.

On a side note, the index export is resolvable by resolve.exports, but this is not true for the rest of the exports (vanilla, utils, etc.). The legacy resolution would still attempt to resolve the relative path from the index export.

Edit:

Edit2:

lxsmnsyc commented 3 years ago

image

I tested the package.json against import / require resolution and this is how all of the bundlers and resolvers resolve valtio. As you can see, if the resolvers uses the export map, it picks up the CJS bundle.

image

Above is the resolution output if we use the import / require. Do take note that since the resolution is correct, mixture of CJS and ESM modules will never happen as the modules are correctly resolved (the currrent behavior was, if the we import valtio, it resolves to either the CJS or ESM, however if valtio imports valtio/vanilla, it can only resolve to valtio/vanilla's CJS bundle.)

dai-shi commented 3 years ago

some previous discussion: https://github.com/pmndrs/jotai/issues/354

I think dual package hazard is rare, but it can be intentionally produced, no?

lxsmnsyc commented 3 years ago

but it can be intentionally produced, no?

Could be with Node versions that do not support the new resolution spec: https://nodejs.org/api/esm.html#esm_resolution_algorithm which is the current resolution most bundlers follow (e.g. @rollup/plugin-node-resolve, ESBuild, etc.)

dai-shi commented 3 years ago

package valtio: cjs and esm a 3rd-party package valtio-foo: cjs only app using both packages: webpack?

does this work?

lxsmnsyc commented 3 years ago

I'm not that familiar with Webpack's resolution algorithm, although I'll try with ESBuild and see how the bundle output looks like.

lxsmnsyc commented 3 years ago

I managed to make a reproduction of dual package hazard. Here's the output log of the modules:

image

module D:

console.log('imported module-d');

export default function moduleD() {
  console.log('D');
}

module A:

import D from 'module-d';

console.log('imported module-a');

export default function A() {
  D();
}

module B:

import D from 'module-d';

console.log('imported module-b');

export default function B() {
  D();
}

module C:

import A from 'module-a';
import B from 'module-b';

console.log(A, B);

Behavior:

The resolution chain would be:

Do take note that this only happens because the packages A and B only resolves to the bundle they provide.

So the answer to this

but it can be intentionally produced, no?

Was yes, although it can only be intentionally produced by a third-party author.

Here's the link: https://github.com/lxsmnsyc/dual-hazard-poc

lxsmnsyc commented 3 years ago

IMO, 1P should not be the one reliable for providing a bridge between CJS and ESM (for preventing state isolation). The only reason dual hazard should exist is if packages with dependencies only supports the CJS or ESM format.

dai-shi commented 3 years ago

@LXSMNSYC Thanks for the repro! Yeah, feels like a rare case.

https://github.com/pmndrs/jotai/pull/289#discussion_r574271494 Would you check this conversation? It originally suggested "module". Can you follow, why webpack introduced it?


I think we'd like this?

{
  "types": "./utils.d.ts",
  "module": "./esm/utils.js",
  "import": "./esm/utils.js",
  "default": "./utils.js"
}
lxsmnsyc commented 3 years ago

It originally suggested "module".

It seems that this condition was introduced by Webpack (much like the "module" field in package.json)

According to here: https://webpack.js.org/guides/package-exports/#providing-commonjs-and-esm-version-stateful

If the package was stateful (in valtio's case, the symbols), the package would still need to provide an import/require resolution along side module, however this time, import needs to provide a wrapper file for the CJS file.

Compatibility-wise, this would be good, however I'm not sure if tree-shaking would work here.

lxsmnsyc commented 3 years ago

I found another solution: https://nodejs.org/api/packages.html#packages_approach_2_isolate_state

We can move valtio's state and side-effects to another module. This way, valtio can provide both CJS and ESM bundles while sharing the same state and side-effect

dai-shi commented 3 years ago

Okay, if symbols are only blocking, we should prefer using Symbol.for. Let me draft a PR.