search

Keydonix / liquid-long

The Unlicense
8 stars 1 forks source link

Changes the way Ethers importing is done to make library work in brower. #78

Closed MicahZoltu closed 5 years ago

MicahZoltu commented 5 years ago

The Ethers library has two sets of outputs, one that lives in the root of the project and one that lives in dist subdirectory. The dist subdirectory is compiled targeting browser, while the rest is compiled targeting NodeJS. When you do an import of a library using import { ... } from 'library/path' it ignores the package.json paths and accesses the library directly at that path. This means that if you do import { ... } from 'ethers/utils' it will always be pulling in the NodeJS version, even when you are targeting the browser.

To complicate matters, TSC seemse to be unhappy with the way ethers exports are setup and it complains if a dependency of this library tries to do import { utils } from 'ethers' and fulfill a type requirement in the library where it also has a similar import. To resolve this, the library now re-exports the Ethers pieces that are necessary to create a custom Provider. This should only be necessary for testing/mocking things, so end-users shouldn't ever see or notice this change.

The important high-level takeaway from this change is that the library is now friendly to being used in the browser. It has been tested against a previously failing Angular app (which bundles with Webpack under the hood) and everything is now working.

asnov commented 5 years ago

@MicahZoltu , I am not sure why you're creating an additional variable for every child module of ethers.js: import { utils as EthersUtils, providers as EthersProviders } from 'ethers' and then using the long constructions like EthersUtils.BigNumber

instead of importing the ethers default export: import * as ethers from 'ethers'; and then using the names as they are in the library: ethers.utils.BigNumber

It looks like additional variables make the code more complex. So what is the benefit to add them?

Could you check how it would look like without them in my PR #79 ? btw, this way you don't need to re-exports the Ethers pieces.

MicahZoltu commented 5 years ago

Your PR seems reasonable to me, but I have a few comments that I would like to get addressed before merging. In the meantime, I would like to merge this as it will let us get a new package released to NPM. Since your PR doesn't change any public surface area (just internal implementation), we can merge it after releasing to NPM and we won't need to re-realese.