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

Declaring types within the repos packages itself #2056

Merged
merged 16 commits into from
Nov 28, 2018
Merged

Declaring types within the repos packages itself #2056

merged 16 commits into from
Nov 28, 2018

Conversation

joshstevens19
Copy link
Contributor

@joshstevens19 joshstevens19 commented Nov 18, 2018

Hi, i work with TypeScipt all the time and do use web3 constantly, i always find the types are well out of date and it just never keeps in sync. As per TypeScripts suggestion they state if possible putting your d.ts files in the library itself gives a better user ability for the developers, as it will instantly expose the types. These are still all versioned with updates etc so you are getting a better developer experience.

BigNumber.js do this as well as most big solutions which i think we should follow - https://github.com/MikeMcl/bignumber.js/blob/master/bignumber.d.ts

What i have done is created the first utils.d.ts types for us in the web3-utils package - i am planning to go through all the packages to expose these types so it's all in one repo if you approve of this PR. TypeScript only ever suggest using the @types if you are not in-control of the repo - which we are.

This solution will mean updating methods in the future and updating it's typing will be super easy for all the maintainers as it's in 1 repo. We can also add this to the contribution guidelines and pull request checklists meaning it will never be missed. I know a lot of people may just use js but as a regular TS developer using these library i think this would be a very good way to start improving this. Like i said i am happy to go through all of them and do a PR for each package.

The plan then would be once all types are in then we eventually remove type @types package and everything installed with 1 command, or depreciate it on a certain version.

It won’t make any difference to the way js developers work, it will still be

import * as Utils from 'web3-utils';

but when doing that in a .ts file it will now expose all the typings allowing people to know the parameters types, return types etc!

I have done this within the active ethereumProvider branch, but if you think it’s better suited for another branch let me know! 👍

I see @nivida as the most active community member recently - take a look and let me know :)

Thanks
Josh

@joshstevens19 joshstevens19 changed the title Declaring types within the repos itself Declaring types within the repos packages itself Nov 18, 2018
@nivida
Copy link
Contributor

nivida commented Nov 23, 2018

Thank you for submitting this PR! I will have a close look on it later but I will definitely merge it into the ethereumProvider branch. But yes as you said it would be good to have typings for each package. Could you please add the missing last line to the type declaration of utils? and it would be also good to add this files to the files property in the package.json of the utils package otherwise it want be published on npm :)

@joshstevens19
Copy link
Contributor Author

joshstevens19 commented Nov 23, 2018

Hey Nivida, thanks for the response i am glad you agree. Like i said once this one is merged in i will do a PR for every package :). This will be a huge step forward. So firstly there is no missing last line i forgot to remove the white space which i have done. Also by default npm brings all the files over unless they are in the .npmignore so i don't think we need to create file property in our package.json as everything in the package needs bringing through.

Lastly i just seen a really nice way to export these types, as we do not have classes we can do what fs do and fs-extra which is a lovely way, so quickly changing that now. This will then allow them to only import what they need i.e. import { isBigNumber } from 'web3-utils'. Will do my updates now :)

@joshstevens19
Copy link
Contributor Author

Hey just pushed my latest changes have a look and if happy merged in it and il get the other PRs to you when i get a chance. 1 PR per package 👍 cheers

@nivida
Copy link
Contributor

nivida commented Nov 27, 2018

@joshstevens19 Great thanks! I thought it would be the better approach to define the files property in the package.json instead of ignoring all. It's much easier to just say what I would like to have in the NPM package instead of ignore anything I don't want to have in my NPM package. I've added the files property in all package.json files.

I will resolve the conflicts and add your typings to the files property in the package.json.

As you can see here: https://github.com/ethereum/web3.js/blob/ethereumProvider/packages/web3-utils/src/index.js#L33 I'm exporting the methods from the Utils file and all others too. It should be already possible to have imports like: import { isBigNumber } from 'web3-utils'

packages/web3-utils/index.d.ts Outdated Show resolved Hide resolved
packages/web3-utils/README.md Outdated Show resolved Hide resolved
@nivida nivida self-assigned this Nov 27, 2018
@joshstevens19
Copy link
Contributor Author

Ok cool, I will resolve your comments when on a computer. In terms of what you said about only exporting what you need, yes I know we could already do it but in the TS world we would do that and we have no typing, so my point was whooop TypeScript devs can now import 1 function and have typing for it 👍

In terms of files in the NPM package - ok makes sense!

Thanks il sort last comments a little later 👍

@nivida
Copy link
Contributor

nivida commented Nov 27, 2018

@joshstevens19 oh then I got you wrong I thought you were talking about the index.js. Yes, I agree with you it is better to export the types like this. Thanks for being so active!

@joshstevens19
Copy link
Contributor Author

Pushed those changes up for you 👍 let me know if that's all good.. once this is merged in il go onto the next package and so on until all our complete. I may branch off after this merge so i can do many PRs at the same time (as we wont have any merge conflicts as they are different packages)

@nivida nivida merged commit 273e212 into web3:ethereumProvider Nov 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants