Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Unique package detection, when importing both as CommonJS and ESM #4315

Closed
tjwelde opened this issue Dec 10, 2021 · 8 comments
Closed

Unique package detection, when importing both as CommonJS and ESM #4315

tjwelde opened this issue Dec 10, 2021 · 8 comments

Comments

@tjwelde
Copy link

tjwelde commented Dec 10, 2021

General issue description

We have built a library on top of polkadot/api (and other @PolkaDot packages), which currently only transpiles to CommonJS.
Since polkadot/api supports both ESM and CommonJS, but requires projects to import modules just once, people using both our library and the polkadot/api directly, with ESM import, get warnings in the console.

Additionally, the warnings include <unknown> for the second path, which I don't know why and doesn't hint to the underlying problem.

I have added some solutions, we thought of at the bottom of this issue, but wanted to get another opinion, maybe you already thought of something like this.
So what do you think would be the best solution?

How to reproduce

  1. Make new directory and change into it
  2. yarn init
  3. yarn add @polkadot/util-crypto
  4. yarn add @kiltprotocol/sdk-js
  5. Set "type": "module" in package.json
  6. Make new file index.js
  7. Fill with following content:
import { Utils } from '@kiltprotocol/sdk-js'
import { mnemonicGenerate } from '@polkadot/util-crypto'
  1. node index.js

Output will be similar to:

@polkadot/util has multiple versions, ensure that there is only one installed.
Either remove and explicitly install matching versions or dedupe using your package manager.
The following conflicting packages were found:
	8.1.2	/private/var/folders/l3/2rd1qdl90ddfn2dl427vj1zm0000gn/T/tmp.YbEZ5yPz/node_modules/@polkadot/util
	8.1.2	<unknown>
@polkadot/util-crypto has multiple versions, ensure that there is only one installed.
Either remove and explicitly install matching versions or dedupe using your package manager.
The following conflicting packages were found:
	8.1.2	/private/var/folders/l3/2rd1qdl90ddfn2dl427vj1zm0000gn/T/tmp.YbEZ5yPz/node_modules/@polkadot/util-crypto
	8.1.2	<unknown>
@polkadot/wasm-crypto has multiple versions, ensure that there is only one installed.
Either remove and explicitly install matching versions or dedupe using your package manager.
The following conflicting packages were found:
	4.5.1	/private/var/folders/l3/2rd1qdl90ddfn2dl427vj1zm0000gn/T/tmp.YbEZ5yPz/node_modules/@polkadot/wasm-crypto
	4.5.1	<unknown>

Solutions, we thought of

  • Instruct our users to use CommonJS only (worst / fast)
  • Re-export all needed polkadot/api functionality (like mnemonicGenerate) (? / fast)
  • Have users hand needed polkadot.js dependencies to our library (via config, or as arguments) (questionable / medium)
  • Convert our library to also support ESM (best / slow)
@jacogr
Copy link
Member

jacogr commented Dec 10, 2021

The unknown is via ESM, it cannot use __dirname (We cannot use import.meta either in the ESM files since Webpack 4 barfs on it, so it only works in CJS mode via __dirname. Well, there is a solution for it, but it is hacky and a lot of duplication, so I have not been in the mood for it as of yet)

(EDIT: Had a brief play with my hack, will have to wait 6+ months until WP4 is indeed deal as just do it correctly - trying to hack this is just a road to insanity, as horrible as having no paths are, it is a nightmare between Web, Node and RN to make it nice and clean/portable)

Anyway, back on-topic... let me ponder this question a bit as of now have no clue.

(EDIT 2: Still pondering...)

@jacogr
Copy link
Member

jacogr commented Dec 12, 2021

So this is exactly the dual-package-hazard, https://nodejs.org/api/packages.html#dual-package-hazard

So comments on what was laid out -

  1. This approach won't really work, i.e. when somebody used Webpack as a bundler, they would generally use ESM - and it becomes really tricky then
  2. This actually can work, however it assumes that everything is exported and they only use your stubbed packages as imports.
  3. Not sure how this approach would work. So on the API it works well, i.e. you can pass instances of eg classes around. For util/crypto with single exports it is not easy :(
  4. It is great when packages do ESM as well, but at this point in the upgrade cycle you need to export both CJS and ESM - there are a number of people in the JS ecosystem that have gone pure ESM only, but then users cannot use it when using CJS. (And this approach creates a lot of support noise around it).

On the last point, this may not be "as slow as imagined" i.e. you can have a tsconfig.esm.json that builds ESM - I actually contributed something similar here recently https://github.com/paulmillr/noble-hashes/blob/main/tsconfig.esm.json & https://github.com/paulmillr/noble-hashes/blob/main/package.json#L15 (the tricky part was this https://github.com/paulmillr/noble-hashes/blob/main/package.json#L61 - it was added manually, in the @polkadot repos it is added via script)

@jacogr
Copy link
Member

jacogr commented Dec 12, 2021

See the chore here - KILTprotocol/sdk-js#446 (it is a start, not a full-blown final solution)

@tjwelde
Copy link
Author

tjwelde commented Dec 14, 2021

Yes, that seems exactly what it is.
Do you think about esm wrapping stateful files instead of exporting them twice (one commonjs, one esm), which would mitigate issues on your side?
Or since you are already requiring node v14.0.0, do you think about switching to just ESM anytime soon?

@jacogr
Copy link
Member

jacogr commented Dec 14, 2021

The issue with ESM-only is that is forced all users to be ESM-only as well. It will break the world and force everybody to migrate. So it is not on the cards for at least 12 months. (Probably longer)

The issue with shim-ing ESM from CJS is that the CJS outputs are absolutely crazy horrible. There is a lot of baggage in that world. Plus it makes the whole development process 2x as strenuous since everything is doubled-up as source instead of just as a compile-step.

@polkadot-js-bot
Copy link

This issue has been open for 21 days with no activity and is not labelled as an enhancement. It will be closed in 7 days.

@jacogr
Copy link
Member

jacogr commented Feb 25, 2022

I'm going to close this. It has 2 parts -

(a) it is absolutely horrible having to dictate what downstream packages do, i.e. having to do both ESM and CJS to not run into the dual-package hazard trap
(b) there are no "quick fixes" aport from reverting completely to only CJS and then re-visiting (pure ESM is certainly the future, but as of now it breaks the world)

With this in mind, it is certainly an issue, but at this point one with no real solution.

@jacogr jacogr closed this as completed Feb 25, 2022
@polkadot-js-bot
Copy link

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue if you think you have a related problem or query.

@polkadot-js polkadot-js locked as resolved and limited conversation to collaborators Mar 4, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

3 participants