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

allowSyntheticDefaultImports is too opinionated #2382

Closed
trungfinity opened this issue Feb 16, 2019 · 7 comments
Closed

allowSyntheticDefaultImports is too opinionated #2382

trungfinity opened this issue Feb 16, 2019 · 7 comments
Labels
Types Incorrect or missing types

Comments

@trungfinity
Copy link

Expected behavior

allowSyntheticDefaultImports should be turned off since this is a very opinionated rule. When it's turned on like what we have right now, other projects which depend on web3 have to obey the rule no matter what.

This is bad since some projects (and I personally) prefer non-synthetic default imports, they are easier to reason in term of understanding what's happened behind the scene. Namespace imports and default imports have different meanings and they raise different awareness on what to expect when importing a module.

Since the typings have been added into web3, a lot of packages got broken because the new typings are not compatible with @types/web3 (which is good enough typing package in my opinion, except some minor missing typings).

I think a package like web3 shouldn't dictate on how imports should be made. I understand the ease and consistency of using default imports all the time but it only makes sense on an application level, not a library level like what web3 should be.

Actual behavior

allowSyntheticDefaultImports is turned on in all tsconfig.json, which makes all dependent projects have to follow this rule against their will.

@joshstevens19 tagging you here for discussion. I'd be happy to make a PR to fix.

@joshstevens19 joshstevens19 added the Types Incorrect or missing types label Feb 16, 2019
@joshstevens19
Copy link
Contributor

joshstevens19 commented Feb 16, 2019

Hey @ngthanhtrung thanks for raising this, I completely agree with you. I didn't even know that config got turned on must of been missed along the way. Happy to change this and get it deployed on the next release. Its only a few config files changes i think 16 to be exact. I can make these change this weekend when i get a chance as i have another change i want to make with the typing's. I have just seen that it is actually turned off in web3-utils ha so yeah along the way it was re enabled.

Since the typings have been added into web3, a lot of packages got broken because the new typings are not compatible with @types/web3

Sorry to hear that @types/web3 was not part of the web3 development it was managed by a few public members, a lot of refactoring happened and we went with a fresh mind to bring the best types we can. Sorry for any inconvenience this coursed but i think in the long run it should give you better typing's support and show you compile errors before your software is released. We also covered every aspect of the web3 lib with full test coverage on all the types which is a huge thing.

Cheers for the feedback on this really appreciate it 👍

@trungfinity
Copy link
Author

Thanks a bunch, @joshstevens19, will work on a PR now so you don't have to bother with implementing this.

@joshstevens19
Copy link
Contributor

Ok yes thanks that would be great, please reference this issue on the PR 👍

@trungfinity
Copy link
Author

Hey @joshstevens19 is there anywhere to chat with you outside of GitHub? I got some questions to ask.

@joshstevens19
Copy link
Contributor

Telegram - @joshstevens19 👍

@joshstevens19
Copy link
Contributor

#2415 👍 @ngthanhtrung

@joshstevens19
Copy link
Contributor

Will close as PR is up and will be merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Types Incorrect or missing types
Projects
None yet
Development

No branches or pull requests

2 participants