Uniswap / web3-react

A simple, maximally extensible, dependency minimized framework for building modern Ethereum dApps
https://web3-react-mu.vercel.app/
GNU General Public License v3.0
5.56k stars 1.52k forks source link

Consider releasing connectors as separate packages #16

Closed macbem closed 5 years ago

macbem commented 5 years ago

Right now, web3-react weighs more than my whole web app. It looks like most of this weight comes from Portis and WalletConnect SDKs. I guess that not everyone will need these and if they do, they can always load them asynchronically to improve app performance.

What do you think?

NoahZinsmeister commented 5 years ago

@macbem I was wrestling with this all day yesterday :/ I actually had it working quite nicely, SDKs would be fetched conditionally, and errors would be thrown only if you tried to initialize a connector that required an SDK which you hadn't installed yet.

Unfortunately it turns out that, for example, the create-react-app config really doesn't like dynamic imports where the referenced library isn't actually installed in the project. I was getting either a) un-suppressible warning from the webpack hot loader, or b) errors depending on which flavor of conditional/dynamic imports I was using.

I would honestly love to rip out all all the dependencies other than @0x/subproviders, because you're totally right, they're all optional and absolutely bloat the package.

Do you have any thoughts on this? It's possible that the best solution is to proceed with the unbundled code that works but causes un-suppressible errors in create-react-app.

macbem commented 5 years ago

I think that the best option would be to let users lazy-load connectors on their own - just expose them as a subpackage, like @web3-react/connectors/Portis using a tool like Lerna for managing all this.

Sure, web3-react users would have a bit more to do, but the lib would become much more useful in "professional" scenarios because downloading it wouldn't mean accepting a 2mb dependency. This would also solve your issues with create-react-app.

Most of the stuff this lib does is already async, because you have to wait for user approval in case of transactions, so IMO this wouldn't make using this lib too complex.

NoahZinsmeister commented 5 years ago

If you want to play around with this pattern, the unbundled version is 4.0.2.

The caveat is that you'll probably get those webpack warnings in dev. The upside is that I'm 95% sure that ultimately this doesn't matter, because once you bundle and ship to production, if you don't include connectors which use the SDKs you haven't installed, no errors will occur.

NoahZinsmeister commented 5 years ago

Ok @macbem great points, I will pursue that path. To clarify, exposing connectors as subpackages would help only if each one is exposed as an individual npm package, correct? Which I could use Lerna to manage?

macbem commented 5 years ago

Not a Webpack expert, it really depends on how Webpack resolves dynamic imports from packages. If you could bundle the base web3-react and then dynamically load connectors from the same package, this would be perfect. Lerna is just a tool for managing monorepos that makes it easier to manage developing packages that are somewhat connected.

NoahZinsmeister commented 5 years ago

bundle the base web3-react and then dynamically load connectors from the same package

Agreed that this is best, i'm just not confident that the dev environments of the average web3-react user are going to handle this pattern gracefully by default.

This problem bugs me a lot to be honest, so I think as a first step I will break out all connectors (or maybe just the ones that require an SDK, since the others have minimal bundle size overhead) into individually scoped @web3-react/connectors/{blah} or maybe @web3-react/{blah} packages. I will aim to have this done by the end of today, or tomorrow, if you'd like to hang tight until then.

macbem commented 5 years ago

Sure, I'll wait for the updates. If you need any help, let me know. BTW, I've sent you a message about a little util app I've made with web3-react on Twitter.

NoahZinsmeister commented 5 years ago

@macbem This is ready to go! New bundle size is >50% smaller. web3-react now only has 1 dependency: @0x/subproviders.

I moved TrezorConnector, WalletConnectConnector, FortmaticConnector, and PortisConnector into separate packages: @web3-react/trezor, @web3-react/walletconnect, @web3-react/fortmatic, and @web3-react/portis respectively, which all live in the new web3-react-connectors monorepo.

The github/docs/codesandbox examples have all been updated.

Let me know if you have any questions/issues!

macbem commented 5 years ago

Looks and works fine, although I'd probably release this as 5.0.0 - it's a breaking change after all. Btw, why not have both web3-react and connectors in a monorepo?

NoahZinsmeister commented 5 years ago

@macbem so i had a bit of a change of heart - rather than going the monorepo route, i think what i want to do for connectors that require an SDK is require users to pass it in at runtime. this has the benefit of getting rid of all the dynamic import problems and overhead of managing a monorepo, with the downside of relying on users to install particular SDKs. after initial implementation, i think the tradeoff is definitely worth it.

i released this as 5.0.0, tagged as unstable. i'm going to keep it like this for a bit, and if i don't find any errors, release it as latest.

NoahZinsmeister commented 5 years ago

closed as of 5.0.2, tagged as latest! i opted for the runtime argument route.