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

Make packages proper ESM with "type": "module" #121

Closed
vovacodes opened this issue Oct 13, 2021 · 6 comments · Fixed by #132
Closed

Make packages proper ESM with "type": "module" #121

vovacodes opened this issue Oct 13, 2021 · 6 comments · Fixed by #132
Labels
help wanted Extra attention is needed

Comments

@vovacodes
Copy link

vovacodes commented Oct 13, 2021

Is your feature request related to a problem? Please describe.
The packages from this repo currently export modules in "loose" ESM format which means you cannot import them into a webpack 5 or nodejs project that uses "native" node ESM module resolution.

Describe the solution you'd like
We could solve it by making the files in lib/* proper ESM (add file extensions to imported module specifiers) and add "type": "module" to package.json.

See https://webpack.js.org/guides/ecma-script-modules/ for more details

@vovacodes vovacodes changed the title Would you be open to making this package a "type": "module" Make packages proper ESM with "type": "module" Oct 13, 2021
@jordaaash
Copy link
Collaborator

Hmm. What effect does this have on downstream apps or projects that depend on the library? Do they also need to be ESM modules?

We'd need to know whether this breaks compatibility with existing starter projects, projects like dapp-scaffold, etc. but I'm open to the idea, and a PR to evaluate would be welcome.

@vovacodes
Copy link
Author

vovacodes commented Oct 13, 2021

This is indeed a bit of a risky move, so we'd need to test the bundler behavior in a scenario where the package is imported from a "commonjs" setup.

It might require some additional config in the package.json's exports field: https://nodejs.org/api/packages.html#packages_conditional_exports

@jordaaash
Copy link
Collaborator

Please let me know what you find out, a PR would help understand the tradeoffs.

@jordaaash
Copy link
Collaborator

I've tested out building the library after converting all the relevant packages to ESM: #132

It seems to work okay, since apps that use this already need to transpiling the modules one way or another and the import/export syntax hasn't changed.

This could be expanded to build ESM and CJS versions if needed, but I'd rather not unless there's an obvious benefit.

Let me know what you think.

@jordaaash
Copy link
Collaborator

This has been implemented. All the core and wallet packages are ESM modules. The React, Angular, and Vue UI packages are not, as I ran into issues with these when using sideEffects: false, which was necessary to fix #69.

@jordaaash
Copy link
Collaborator

Published:

  • @solana/wallet-adapter-base@0.7.0
  • @solana/wallet-adapter-react@0.13.0
  • @solana/wallet-adapter-vue@0.3.0
  • @solana/wallet-adapter-wallets@0.11.0
  • @solana/wallet-adapter-example@0.14.0
  • @solana/wallet-adapter-material-ui-starter@0.10.0
  • @solana/wallet-adapter-nextjs-starter@0.5.0
  • @solana/wallet-adapter-react-ui-starter@0.6.0
  • @solana/wallet-adapter-ant-design@0.9.0
  • @solana/wallet-adapter-material-ui@0.13.0
  • @solana/wallet-adapter-react-ui@0.6.0
  • @solana/wallet-adapter-bitpie@0.3.0
  • @solana/wallet-adapter-blocto@0.3.0
  • @solana/wallet-adapter-clover@0.2.0
  • @solana/wallet-adapter-coin98@0.3.0
  • @solana/wallet-adapter-ledger@0.7.0
  • @solana/wallet-adapter-mathwallet@0.7.0
  • @solana/wallet-adapter-phantom@0.7.0
  • @solana/wallet-adapter-safepal@0.3.0
  • @solana/wallet-adapter-slope@0.3.0
  • @solana/wallet-adapter-solflare@0.4.0
  • @solana/wallet-adapter-sollet@0.8.0
  • @solana/wallet-adapter-solong@0.7.0
  • @solana/wallet-adapter-tokenpocket@0.2.0
  • @solana/wallet-adapter-torus@0.8.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
help wanted Extra attention is needed
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants