search

web3 / web3.js

Collection of comprehensive TypeScript libraries for Interaction with the Ethereum JSON RPC API and utility functions.
https://web3js.org/
Other
19.13k stars 4.9k forks source link

[4.x] Use web3 as only dependency #5508

Closed mpetrunic closed 1 year ago

mpetrunic commented 1 year ago

Is there an existing issue for this?

Feature Request

Expecting users to know what resides in which web3 package is optimistic, to say the least. It's hard for me (even though I do have extensive experience in developing apps), can't imagine how new web3 developers will find their way.

That said, you don't need to sacrifice tree shaking to achieve that.

Use Cases

Importing types and functionality from a bunch of different web3 packages is hard without knowing which package contains what.

It's also making it hard to link web3 and test changes against the dev version of web3.

Implementation Ideas

You can see solutions in existing sdks like:

Just do named re-export of all the required types, classes, and functions (you can even group them to make it easier for users). When a user uses named import, it will tree shake all not imported code paths.

Example usage would then be:

import {Web3, Contract} from "web3";

const web3 = new Web3(...)
const contract = new Contract(abi, address)
contract.link(web3) or whatever

Feature Examples/References

Are you willing to implement this feature?

Muhammad-Altabba commented 1 year ago

I think this is a very needed feature. However, it would break the backward compatibility with how users could import web3.js as they used to done with version 1.x (import Web3 from "web3"). And actually, we choose to favor backward compatibility earlier for this specific matter. As you can see in this MR: https://github.com/web3/web3.js/pull/5221 and its related issue: https://github.com/web3/web3.js/issues/5196

So, do you think it is worth doing this breaking change now? Or should it be done sometime later? When exactly do you think?

mpetrunic commented 1 year ago

Imho, we should certainly do that breaking change now and try to discourage users from importing like import Web3 from "web3". Named exports are way better for tree shaking, ide ergomics and to control exported api-s.

That said, if you add export default Web3 along with named exports, pretty sure import Web3 from "web3" would still work.

Muhammad-Altabba commented 1 year ago

I also feel that this is a very needed feature. But at the same time I see that this breaking change will cause a compiler error even with the simplest code written for 1.x when the user upgrades his dependencies to 4.x. But it is something tiny to fix at the same time. This all together makes me neutral on whether to recommend introducing this right now or not :blush: And I leave it to you. And it would be nice to hear from others...

And actually, we tried to enable both import { Web3} from "web3" and import Web3 from "web3" [updated the comment to add this as it was missed: along with require("web3")] but this is not supported by Typescript as it gives issues with CommonJS (https://github.com/web3/web3.js/issues/5196#issuecomment-1184531579). And you are welcome to try and see if you can figure out a way to do it.

mpetrunic commented 1 year ago

I also feel that this is a very needed feature. But at the same time I see that this breaking change will cause a compiler error even with the simplest code written for 1.x when the user upgrades his dependencies to 4.x. But it is something tiny to fix at the same time. This all together makes me neutral on whether to recommend introducing this right now or not blush And I leave it to you. And it would be nice to hear from others...

And actually, we tried to enable both import { Web3} from "web3" and import Web3 from "web3" but this is not supported by Typescript as it gives issues with CommonJS (#5196 (comment)). And you are welcome to try and see if you can figure out a way to do it.

Issue is only const web3 = require("web3") as you would have to do const web3 = require("web3").default. If you really thing this is issue, you could define separate entrypoint for require where there will be commonjs export: https://nodejs.org/api/packages.html#conditional-exports

Muhammad-Altabba commented 1 year ago

I also feel that this is a very needed feature. But at the same time I see that this breaking change will cause a compiler error even with the simplest code written for 1.x when the user upgrades his dependencies to 4.x. But it is something tiny to fix at the same time. This all together makes me neutral on whether to recommend introducing this right now or not blush And I leave it to you. And it would be nice to hear from others... And actually, we tried to enable both import { Web3} from "web3" and import Web3 from "web3" but this is not supported by Typescript as it gives issues with CommonJS (#5196 (comment)). And you are welcome to try and see if you can figure out a way to do it.

Issue is only const web3 = require("web3") as you would have to do const web3 = require("web3").default. If you really thing this is issue, you could define separate entrypoint for require where there will be commonjs export: https://nodejs.org/api/packages.html#conditional-exports

Thanks @mpetrunic,

I think, we can use the same export for all to avoid possible confusion by the user.

And so, the following would be the code for index.ts at web3 package:


import Web3 from './web3';
export default Web3;

export * from 'web3-core';
export * from 'web3-eth';
export * from 'web3-eth-accounts';
export * from 'web3-eth-ens';
export * from 'web3-eth-personal';
export * from 'web3-net';
export * from 'web3-providers-http';
export * from 'web3-providers-ws';
export * from 'web3-types';
export * from 'web3-validator';
export * from 'web3-errors';
export * from 'web3-eth-abi';
export * from 'web3-eth-contract';
export * from 'web3-eth-iban';
export * from 'web3-providers-ipc';
export * from 'web3-rpc-methods';
export * from 'web3-utils';

However, this would give us the following errors (and resolving those errors will require some needed and helpful changes to give different names to functions from other packages, based on the different jobs they do).

web3: src/index.ts(329,1): error TS2308: Module 'web3-eth' has already exported a member named 'sign'. Consider explicitly re-exporting to resolve the ambiguity.
web3: src/index.ts(329,1): error TS2308: Module 'web3-eth' has already exported a member named 'signTransaction'. Consider explicitly re-exporting to resolve the ambiguity.
web3: src/index.ts(337,1): error TS2308: Module 'web3-types' has already exported a member named 'Web3Error'. Consider explicitly re-exporting to resolve the ambiguity.
web3: src/index.ts(338,1): error TS2308: Module 'web3-types' has already exported a member named 'AbiInput'. Consider explicitly re-exporting to resolve the ambiguity.
web3: src/index.ts(338,1): error TS2308: Module 'web3-types' has already exported a member named 'AbiParameter'. Consider explicitly re-exporting to resolve the ambiguity.
web3: src/index.ts(339,1): error TS2308: Module 'web3-eth' has already exported a member named 'LogsSubscription'. Consider explicitly re-exporting to resolve the ambiguity.
web3: src/index.ts(343,1): error TS2308: Module 'web3-types' has already exported a member named 'Web3DeferredPromise'. Consider explicitly re-exporting to resolve the ambiguity.
web3: src/index.ts(343,1): error TS2308: Module 'web3-validator' has already exported a member named 'TypedArray'. Consider explicitly re-exporting to resolve the ambiguity.
web3: src/index.ts(343,1): error TS2308: Module 'web3-validator' has already exported a member named 'checkAddressCheckSum'. Consider explicitly re-exporting to resolve the ambiguity.
web3: src/index.ts(343,1): error TS2308: Module 'web3-validator' has already exported a member named 'isAddress'. Consider explicitly re-exporting to resolve the ambiguity.
web3: src/index.ts(343,1): error TS2308: Module 'web3-validator' has already exported a member named 'isBloom'. Consider explicitly re-exporting to resolve the ambiguity.
web3: src/index.ts(343,1): error TS2308: Module 'web3-validator' has already exported a member named 'isContractAddressInBloom'. Consider explicitly re-exporting to resolve the ambiguity.
web3: src/index.ts(343,1): error TS2308: Module 'web3-validator' has already exported a member named 'isHex'. Consider explicitly re-exporting to resolve the ambiguity.
web3: src/index.ts(343,1): error TS2308: Module 'web3-validator' has already exported a member named 'isHexStrict'. Consider explicitly re-exporting to resolve the ambiguity.
web3: src/index.ts(343,1): error TS2308: Module 'web3-validator' has already exported a member named 'isInBloom'. Consider explicitly re-exporting to resolve the ambiguity.
web3: src/index.ts(343,1): error TS2308: Module 'web3-validator' has already exported a member named 'isNullish'. Consider explicitly re-exporting to resolve the ambiguity.
web3: src/index.ts(343,1): error TS2308: Module 'web3-validator' has already exported a member named 'isTopic'. Consider explicitly re-exporting to resolve the ambiguity.
web3: src/index.ts(343,1): error TS2308: Module 'web3-validator' has already exported a member named 'isTopicInBloom'. Consider explicitly re-exporting to resolve the ambiguity.
web3: src/index.ts(343,1): error TS2308: Module 'web3-validator' has already exported a member named 'isUserEthereumAddressInBloom'. Consider explicitly re-exporting to resolve the ambiguity.

Am I on the same page on this? Or your recommendation is something little or more different?

mpetrunic commented 1 year ago

I don't think you should do it like this but rather export as namespaces:

export * as utils from "web3-utils"

https://www.typescriptlang.org/docs/handbook/modules.html#export-all-as-x

But also think about if you wanna re-export all modules or just the ones people actually need to use^^

Muhammad-Altabba commented 1 year ago

I don't think you should do it like this but rather export as namespaces:

export * as utils from "web3-utils"

https://www.typescriptlang.org/docs/handbook/modules.html#export-all-as-x

But also think about if you wanna re-export all modules or just the ones people actually need to use^^

So with this then the code at index.ts at web3 package would be like:

export * as Contract from "web3-eth-contract" // Or export * as contract from "web3-eth-contract" ?
export * as ens from "web3-eth-ens"
export * as iban from "web3-eth-iban"
export * as core from "web3-core"
...

And the user code would be:

import {Web3, ens, iban, core, Contract /* or contract */} from "web3";

const web3 = new Web3(...)
const ensInstance = new ens.ENS(...);
const ibanInstance = new iban.Iban(...);
const contractInstance = new Contract(...);  // or: new contract(...)
const contractInitOptions = new Contract.ContractInitOptions(...); //  or: new contract.ContractInitOptions(...) 
const web3Context = new core.Web3Context(...);

Actually, I see it as not very nice to have things like const ibanInstance = new iban.Iban(...); . Or what do you think?

And for some different cases like web3-eth-contract that the user would previously do import Contract, { ContractInitOptions } from 'web3-eth-contract'; what would be the new form for using ContractInitOptions? More specifically, will we use a capital first letter for export as Contract or a small first letter (`export as Contract from "web3-eth-contract"orexport * as contract from "web3-eth-contract")? And with the second choice (contract), we would then discourage usingnew contract(...)and encouragenew contract.Contract(...)`?

mpetrunic commented 1 year ago

Actually, I see it as not very nice to have things like const ibanInstance = new iban.Iban(...); . Or what do you think?

People working with ethers and hardhat doesn't mind so I wouldn't say it's not nice :)

But if someone doesn't like they are welcome to use web3-eth-iban package directly. Even better solution would be to enable additional esm style entrypoints as well where you would import something like ˙import {Iban} from "web3/iban"`. My main benefit for this approach is that it allows us to remove headache from users to use appropriate package versions and searching for packages. Like devs searching for packages could accidentally install malicious package with similar prefix (web3-eth-something)

And for some different cases like web3-eth-contract that the user would previously do import Contract, { ContractInitOptions } from 'web3-eth-contract';

Think named export should always be available and preferred for multiple reasons.

Muhammad-Altabba commented 1 year ago

Actually, I see it as not very nice to have things like const ibanInstance = new iban.Iban(...); . Or what do you think?

People working with ethers and hardhat doesn't mind so I wouldn't say it's not nice :)

Sorry, I did find that ethers, hardhat or polkadot.js uses export * as, or did they? (https://github.com/polkadot-js/api/blob/master/packages/api/src/bundle.ts#L12, https://github.com/ethers-io/ethers.js/blob/master/packages/ethers/src.ts/ethers.ts#L63 and https://github.com/NomicFoundation/hardhat/blob/main/packages/hardhat-web3/src/index.ts#L8)

But if someone doesn't like they are welcome to use web3-eth-iban package directly. Even better solution would be to enable additional esm style entrypoints as well where you would import something like ˙import {Iban} from "web3/iban"`. My main benefit for this approach is that it allows us to remove headache from users to use appropriate package versions and searching for packages. Like devs searching for packages could accidentally install malicious package with similar prefix (web3-eth-something)

And so for import {Iban} from "web3/iban" do you mean to change the scope of this issue to this better approach? Or is this a proposal for additional future enhancements?

And for some different cases like web3-eth-contract that the user would previously do import Contract, { ContractInitOptions } from 'web3-eth-contract';

Think named export should always be available and preferred for multiple reasons.

I think so. And for the related question that I asked, I think your answer then is to do: export * as contract from "web3-eth-contract" and then the user will do: const contractInstance = new contract.Contract(...);?

mpetrunic commented 1 year ago

Sorry, I did find that ethers, hardhat or polkadot.js uses export * as, or did they?

Some stuff is exported directly but all others are exported as namespace: https://github.com/ethers-io/ethers.js/blob/v5.7.2/packages/ethers/src.ts/ethers.ts#L12

Harhat code changed since last time I looked at it so I cannot figure it out now haha.

And so for import {Iban} from "web3/iban" do you mean to change the scope of this issue to this better approach? Or is this a proposal for additional future enhancements?

Proposal for future enhancements

I think so. And for the related question that I asked, I think your answer then is to do: export * as contract from "web3-eth-contract" and then the user will do: const contractInstance = new contract.Contract(...); ?

In general yes, but for this specific example it probably makes sense to export Contract in root as well^^

Muhammad-Altabba commented 1 year ago

Hello @mpetrunic I created a draft MR: https://github.com/web3/web3.js/pull/5771 in which the changes for web3 package are mainly as follow:

Is there any comment or concern before I update the CHANGELOG.md files and the documentation? Thanks,