WalletConnect / walletconnect-monorepo

WalletConnect Monorepo
Apache License 2.0
1.46k stars 714 forks source link

Properly declare "main" and "browser" entrypoints in package.json #341

Closed fubhy closed 1 year ago

fubhy commented 4 years ago

Instead of only exposing a main entry point in package.json, there should be a browser entrypoint for web bundlers to use.

E.g. instead of (or in addition for) doing this https://github.com/WalletConnect/walletconnect-monorepo/blob/next/packages/helpers/qrcode-modal/src/index.ts there should be a browser.ts entrypoint that simply doesn't import any node dependencies in its entire dependency hierarchy whatsoever.

Modern bundlers are starting to drop the out-of-the-box polyfilling of node dependencies for good reason and e.g. with Snowpack, Vite or esbuidler this is currently failing by default.

In fact, in addition to main and browser it would be good to also expose the build output of the packages in this monorepo as esm modules using the module field in package.json.

I see that you are currently using webpack for bundling the libraries. Instead, I'd recommend to use something like rollup.js as it's much better suited to package libraries (webpack is a bundler and hence more suited for applications, not for libraries).

pedrouid commented 4 years ago

Agreed. I've had issues building ESM modules and that's why I didn't have a module field for packages.

Regarding the browser field currently I'm exposing the UMD bundle in the unpkg field to be served by CDNs. I'm guessing that's what you would expect to be exposed by browser field, right?

fubhy commented 4 years ago

Yes, that would be one way to do it. I'd suggest giving rollup a shot and explicitly preparing different bundles for module, main and browser that way. But yes, unpkg and browser would then both be UMD. Simply speaking:

module: esm
browser: umd
main: cjs

If you want, I can lend a hand with rollup over the weekend.

fubhy commented 4 years ago

Oh but just to be add to my previous point: The UMD bundle should be free of any (even conditional) node dependencies. So e.g. https://github.com/WalletConnect/walletconnect-monorepo/blob/next/packages/helpers/qrcode-modal/src/index.ts is not okay.

The browser bundle should use browser.ts and hence the umd bundle should be built with browser.ts as an entry point.

pedrouid commented 4 years ago

That's a good point. I should have two different entry points for the qrcode-modal package.

Definitely would appreciate some help with rollup configuration 🙏

fubhy commented 4 years ago

Sure thing! Should I just go ahead and work on a PR for that for the entire monorepo or do you want to discuss that further?

dievardump commented 3 years ago

Is this worked on? Not being able to bundle WalletConnect with rollup is a big issue.

Would be nice to have something "that just works" when not used with React or webpack.

I'm not a ts developer so I wouldn't even know where to start to fix this myself.

pedrouid commented 3 years ago

@fubhy has actually offered to help with this 🙌 enable notifications to follow up on this repo

ronanyeah commented 3 years ago

Not sure if this is related but I'm having trouble with Node.js imports crashing frontend builds with Webpack 5: Screenshot from 2021-04-14 21-05-26

I can get it to build if I add this to my webpack.config.js:

  resolve: {
    fallback: {
      os: false,
      https: false,
      http: false,
      util: false,
      stream: false,
      assert: false,
      crypto: false,
    },
  },

But seeing this in the console: Screenshot from 2021-04-14 21-14-09

hardyjosh commented 3 years ago

@fubhy did you end up working on this?

I'm also trying to use WalletConnect with rollup and am getting a "require not defined" error in the console. I'm assuming this has something to do with the module not being bundled correctly.

Is this something I can fix with a polyfill?

thekevinbrown commented 3 years ago

Same issue here with require not defined, but from Vite (and therefore Rollup) in production build mode only. (https://github.com/vitejs/vite/issues/4593)

The issue is the file published to NPM as @walletconnect/web3-provider/esm/index.js starts like this:

import WalletConnect from "@walletconnect/client";
import QRCodeModal from "@walletconnect/qrcode-modal";
import HttpConnection from "@walletconnect/http-connection";
import { payloadId, signingMethods, parsePersonalSign, getRpcUrl } from "@walletconnect/utils";
const ProviderEngine = require("web3-provider-engine");
const CacheSubprovider = require("web3-provider-engine/subproviders/cache");
const FixtureSubprovider = require("web3-provider-engine/subproviders/fixture");
const FilterSubprovider = require("web3-provider-engine/subproviders/filters");
const HookedWalletSubprovider = require("web3-provider-engine/subproviders/hooked-wallet");
const NonceSubprovider = require("web3-provider-engine/subproviders/nonce-tracker");
const SubscriptionsSubprovider = require("web3-provider-engine/subproviders/subscriptions");

Mixing require and import is bad mojo. I'll have a look at how I can work around this and/or fix it.

thekevinbrown commented 3 years ago

I resolved that in my local copy, but then started hitting:

TypeError: brorand.Rand is not a constructor
    at new MillerRabin$2 (index.85c518f5.js:22986)
    at index.85c518f5.js:23075

It looks like we'll need to do a fair bit of work to get this library to work with Rollup, as that's likely a circular dependency issue with brorand.

dievardump commented 3 years ago

The dirty solution I found / used was:

-> creating a repo that uses Webpack to bundle @walletconnect for browser (a simple export * from '@walletconnect'; with webpack conf for browser) -> use the build in my svelte (rollup / vite) apps.

This is not good for the future since you don't get updates etc... and would not recommend it for a long term solution... but at least, you can use the library.

mfw78 commented 3 years ago

Hi, is there any movement on this issue, or guidance as to how this can be done? I'm hitting the same issue on a Vue3 + Vite project.

Alternatively, @dievardump do you have an example repo that handles the bundling as you mentioned, caveats are noted.

dievardump commented 3 years ago

I don't have any repo.

However you can now use https://www.unpkg.com/browse/walletconnect@1.6.5/dist/umd/ that you can import in you HTML and then use it as explained here: https://www.npmjs.com/package/walletconnect

There is no doc (or I didn't find it) for this "full SDK", but it works and it doesn't need to be bundled (which is not that bad).

mfw78 commented 3 years ago

I'm sorry, I'm very new to this stuff, and I don't understand how to do that. I've got a Vue3 + Vite app, and I've rewritten Web3Modal into a Vue3 component - hence the desire in there to use Wallet Connect. I wouldn't really care about Wallet Connect, but when wanting to interact with Gnosis Safes, it's basically a requirement.

I'd rather not have external dependencies and have everything bundled together so I may package everything and not get blindsided by some dependency dying later on, and the rabbit hole that will come trying to diagnose that problem when I'm least expecting it.

What's the hold up with trying to get walletconnect to bundle with rollup (and therefore with Vite)? This dependency tree of walletconnect truly is ancient (from my understanding). Is there not an effort somewhere to update to newer dependencies, and hopefully fix these issues?

ShravanSunder commented 3 years ago

i'm having the same issue with vite and web3modal. here is a repo from scaffold-eth-typescript: https://github.com/scaffold-eth/scaffold-eth-typescript/tree/feature/4%2Fapp-provider-refactor

the mix of require and imports noted here is causing issues when I build my project: https://github.com/WalletConnect/walletconnect-monorepo/issues/341#issuecomment-898171981

@pedrouid is it a matter of replacing the require in the code with webpack aliasing or externals? Is there any project to update the project to use ethers?

Same issue here with require not defined, but from Vite (and therefore Rollup) in production build mode only. (vitejs/vite#4593)

The issue is the file published to NPM as @walletconnect/web3-provider/esm/index.js starts like this:

import WalletConnect from "@walletconnect/client";
import QRCodeModal from "@walletconnect/qrcode-modal";
import HttpConnection from "@walletconnect/http-connection";
import { payloadId, signingMethods, parsePersonalSign, getRpcUrl } from "@walletconnect/utils";
const ProviderEngine = require("web3-provider-engine");
const CacheSubprovider = require("web3-provider-engine/subproviders/cache");
const FixtureSubprovider = require("web3-provider-engine/subproviders/fixture");
const FilterSubprovider = require("web3-provider-engine/subproviders/filters");
const HookedWalletSubprovider = require("web3-provider-engine/subproviders/hooked-wallet");
const NonceSubprovider = require("web3-provider-engine/subproviders/nonce-tracker");
const SubscriptionsSubprovider = require("web3-provider-engine/subproviders/subscriptions");

Mixing require and import is bad mojo. I'll have a look at how I can work around this and/or fix it.

ShravanSunder commented 3 years ago

@mfw78 @thekevinbrown If you're using vite and vite build this rollup option solves the issue

commonjsOptions: {
      transformMixedEsModules: true,
    },

here's the whole config if you need it:

export default defineConfig({
  plugins: [nodePolyfills(), reactRefresh(), macrosPlugin(), tsconfigPaths()],
  build: {
    sourcemap: true,
    commonjsOptions: {
      transformMixedEsModules: true,
    },
  },
  esbuild: {
    jsxFactory: 'jsx',
    jsxInject: `import {jsx, css} from '@emotion/react'`,
  },
  define: {},
  optimizeDeps: {
    exclude: ['@apollo/client', `graphql`],
    include: ['*/@portis/**'],
  },
  resolve: {
    alias: {
      '~~': resolve(__dirname, 'src'),
      /** browserify for web3 components */
      stream: 'stream-browserify',
      http: 'http-browserify',
      https: 'http-browserify',
      timers: 'timers-browserify',
      process: 'process',
    },
  },
});
alex-shortt commented 2 years ago

bumping this, still facing the issue. made a pr to the walletconnect-utils repo to fix it, but realizing the issue is deeper across more libraries. this is causing huge issues trying to build with rollup

https://github.com/WalletConnect/walletconnect-utils/pull/6

vrde commented 2 years ago

I made it work but I have no idea how, check "Update 2": https://github.com/vitejs/vite/issues/7257#issuecomment-1064636024.

redox commented 2 years ago

Hey @ShravanSunder, thank you for sharing how you fixed it on your end. We successfully fixed it by adding:

  build: {
   [...]
    commonjsOptions: {
      transformMixedEsModules: true,
    },
  },
  optimizeDeps: {
    include: ['*/@portis/**'],
  },
 [...]

I would love to hear from you what's the actual difference with what we had before, why would the pre-bundling+transformMixedEsModules solve the issue? Is @portis/web3 doing something weird?

Nuzzlet commented 2 years ago

Hey guys. I found another solution to this problem. I simply import wallet connect via: import WalletConnectProvider from '@walletconnect/web3-provider/dist/umd/index.min.js'; and to get the types working, I made a d.ts file with: declare module '@walletconnect/web3-provider/dist/umd/index.min.js' { import WalletConnectProvider from '@walletconnect/web3-provider/dist/esm/index'; export default WalletConnectProvider }

jakubdonovan commented 2 years ago

@pedrouid Has there been any progress in making this lib work properly with sveltekit?

ShravanSunder commented 2 years ago

Hey @ShravanSunder, thank you for sharing how you fixed it on your end. We successfully fixed it by adding:

  build: {
   [...]
    commonjsOptions: {
      transformMixedEsModules: true,
    },
  },
  optimizeDeps: {
    include: ['*/@portis/**'],
  },
 [...]

I would love to hear from you what's the actual difference with what we had before, why would the pre-bundling+transformMixedEsModules solve the issue? Is @portis/web3 doing something weird?

@redox i'm pretty sure that portis is not properly exporting an es module and has some common js mixed in their lib or deps.

finessevanes commented 1 year ago

Is this still an issue?

hlopes-ledger commented 1 year ago

It is. We at Ledger are bundling the ethereum-provider and legacy-provider in our DApps Connect Kit using rollup and the only way we found around this was, like @Nuzzlet described, to import the dist files directly and declaring the modules locally to make typescript happy.

We were able to bundle ethereum-provider v2.x using node pollyfills without errors. but it then fails on the browser because pino, the logging library, depends on worker-threads, which does not seem to be polyfilled.