jspm / project

Roadmap and management repo for the jspm project
161 stars 8 forks source link

Missing docs for npm authors - consistency with browser/module specifiers in package.json #272

Open s-cork opened 1 year ago

s-cork commented 1 year ago

I am trying to use @walletconnect/sign-client and @web3modal/standalone through JSPM:

https://generator.jspm.io/#Y2VgYGBiLk/MyUktSc7Py0tNLmFwQOHqF2em5+km52Sm5pU4GOmZ6xkBFaQmGefmpyTm6BeXJOYB6fy8VKCcsZ45AJVr5qVQAA

However, I can't achieve consistency with the CommonJS and ESM versions of the modules. In the link above, I specify the "browser" field, but some of the exports are still pointing to dist/esm/index.js.

e.g. the import map for @walletconnect/jsonrpc-provider points to the ESM version: "@walletconnect/jsonrpc-provider": "https://ga.jspm.io/npm:@walletconnect/jsonrpc-provider@1.0.12/dist/esm/index.js"

The package.json for this package has the following configuration: https://github.com/WalletConnect/walletconnect-utils/blob/master/jsonrpc/provider/package.json

  "main": "dist/cjs/index.js",
  "module": "dist/esm/index.js",
  "browser": "dist/esm/index.js",
  "types": "dist/cjs/index.d.ts",
  "unpkg": "dist/umd/index.min.js",

The "browser" field points to the ESM version, which causes issues since the code seems to expect a CommonJS module.

The code performs an import like import * as l from "@walletconnect/jsonrpc-provider", followed by var S = "default" in l ? l.default : l;. This breaks the code later on because "default" is the actual default export and not the module. Manually changing the import map to dist/cjs/index.js resolves the issue, but it's not ideal.


If I specify both the "browser" and "module" fields, I encounter issues in the opposite scenario where module versions are not specified.


I am happy to follow up with the authors of @walletconnect, but I couldn't find any documentation on JSPM that I could point them to, such as a "best practice for npm library authors."

JayaKrishnaNamburu commented 1 year ago

Maybe this one can help to some extent ? https://jkrishna.dev/post/building-packages-for-runtimes

guybedford commented 1 year ago

Did you try using the "module" condition?

https://generator.jspm.io/#U2VhYGBiLk/MyUktSc7Py0tNLmFwQOHqF2em5+km52Sm5pU4GOmZ6xkBFaQmGefmpyTm6BeXJOYB6fy8VKCcsZ45ALK+Y2NQAA

guybedford commented 1 year ago

Note that having both CJS and ESM being resolved is an ok thing to have.

What specifically is the issue here?

s-cork commented 1 year ago

Hi @guybedford - yes I also tried the "module" condition but this gives a different/related error

with browser + module

TypeError: i is not a constructor
    at new J (https://ga.jspm.io/npm:@walletconnect/core@2.7.2/dist/index.es.js:1:54893)

This is the line that breaks

(this.storage = null != e && e.storage ? e.storage : new i(Lt(Lt({}, Ci), e?.storageOptions))),

where i is defined as

import i from "@walletconnect/keyvaluestorage";

looking at the import map we get

"@walletconnect/keyvaluestorage": "https://ga.jspm.io/npm:@walletconnect/keyvaluestorage@1.0.2/dist/cjs/browser/index.js",

i.e. @walletconnect/core is using the default export from @walletconnect/keyvaluestorage but @walletconnect/keyvaluestorage is providing a cjs file and default is actually the entire module.

the package.json for @walletconnect/keyvaluestorage looks like:

  "main": "dist/cjs/index.js",
  "types": "dist/cjs/index.d.ts",
  "unpkg": "dist/umd/index.min.js",
  "browser": "dist/cjs/browser/index.js",
  "react-native": "dist/cjs/react-native/index.js",

esm versions of this package exist - but the package.json doesn't point to them


with browser only

I get:

TypeError: Z.JsonRpcProvider is not a constructor
    at it.createProvider (https://ga.jspm.io/npm:@walletconnect/core@2.7.2/dist/index.cjs.js:1:34118)

This is the line that causes the error:

this.provider = new Z.JsonRpcProvider( // rest of code

where Z is defined as

import * as l from "@walletconnect/jsonrpc-provider";
var S = "default" in l ? l.default : l;
var Z = S;

the importmap looks like:

"@walletconnect/jsonrpc-provider": "https://ga.jspm.io/npm:@walletconnect/jsonrpc-provider@1.0.12/dist/esm/index.js",

and the final line of that file looks like:

export{JsonRpcProvider,JsonRpcProvider as default};

so @walletconnect/core is trying to use @walletconnect/jsonrpc-provider as a cjs file, and failing since it is actually a esm file with a default export.


I don't see that this is a problem with jspm directly, I'm assuming here that @walletconnect could fix these issues with better package.json definitions. (I might be wrong).

But I didn't know how to approach this with @walletconnect since I couldn't see anything in the jspm documentation that I could point them to.


Let me know if you need anything else.

guybedford commented 1 year ago

@s-cork this is a really thorough breakdown, nicely done. Yes this is a common issue with the "module" condition and why we're looking at changing it to no longer be the default in JSPM actually. Because bundlers are able to dynamically check the object at runtime they can do more magic, whereas Node.js and JSPM must use static analysis to handle CJS and named exports interop.

I would suggest therefore looking into the fix on the "browser" path without "module" as the primary bug here. In that case, the issue is as you say that we have CJS loading ESM not working, and the reason being that it is importing the default export while accessing the named export.

The fact that jsonrpc-provider is being loaded as ESM and not CJS in this workflow then seems to be the issue. My guess on that is that there must be another package that is importing jsonrpc-provider as ESM somewhere in the dependencies, which is then forcing both to the ESM version. So tracking the issue down further would likely be working that out.

This is something that might be possible to fix in the generator through scoping to have two versions being supported - both the ESM and CJS versions running together.

If you are interested in figuring out a proper configuration I think it is clear you want everything to be ESM or CJS for the most part. Applying the "exports" main with entries like:

"exports": {
  "browser": "./main-browser-esm.js",
  "default": "./main-node-esm.js"
}

is probably the most advisable path. These configurations can be made via PR or we have the JSPM override service for making compatibility patches - https://github.com/jspm/overrides.

But you likely still need to figure out which package is using jsonrpc-provider to determine that. It is possible to run JSPM generator with the debugging enabled to do more analysis on this and get some traces, let me know if you'd like further help with that kind of thing too.