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

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

Closed
fubhy opened this issue Aug 31, 2020 · 25 comments
Closed

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

fubhy opened this issue Aug 31, 2020 · 25 comments

Comments

@fubhy
Copy link
Collaborator

fubhy commented Aug 31, 2020

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
Copy link
Member

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
Copy link
Collaborator Author

fubhy commented Aug 31, 2020

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
Copy link
Collaborator Author

fubhy commented Aug 31, 2020

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
Copy link
Member

pedrouid commented Sep 2, 2020

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
Copy link
Collaborator Author

fubhy commented Sep 6, 2020

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
Copy link

dievardump commented Mar 2, 2021

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
Copy link
Member

pedrouid commented Mar 3, 2021

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

@ronanyeah
Copy link

ronanyeah commented Apr 15, 2021

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
Copy link

@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
Copy link

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.

@thekevinbrown
Copy link

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
Copy link

dievardump commented Aug 13, 2021

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
Copy link

mfw78 commented Sep 16, 2021

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
Copy link

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
Copy link

mfw78 commented Sep 17, 2021

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
Copy link

ShravanSunder commented Oct 1, 2021

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: #341 (comment)

@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
Copy link

@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
Copy link

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

WalletConnect/walletconnect-utils#6

@vrde
Copy link

vrde commented Mar 11, 2022

I made it work but I have no idea how, check "Update 2": vitejs/vite#7257 (comment).

@redox
Copy link

redox commented Mar 22, 2022

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
Copy link

Nuzzlet commented Mar 26, 2022

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
Copy link

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

@ShravanSunder
Copy link

ShravanSunder commented Apr 25, 2022

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
Copy link
Contributor

Is this still an issue?

@hlopes-ledger
Copy link

hlopes-ledger commented Feb 10, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests