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

[WIP] Type Definitions #141

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

Conversation

omarish
Copy link

@omarish omarish commented Nov 14, 2020

I was using this library as part of a project and ran into this issue. Saw a couple of people were thinking the same thing, so I went ahead and started some work to add type definitions here. Here's what I've done and a proposed roadmap to get us to supporting types here:

What's done so far:

  • I added a simple conversion script that copies all javascript files in the source tree and adds typescript equivalents. Run ./convert.sh to run it. I've only tested it using zsh; I think there might be some issues if you try and do it in bash because of how bash handles glob expansion.
  • Added typescript and a few @types libs to package.json.

Right now I've disabled noImplicitAny and noImplicitThis, but we might want togo full type safety before merging this PR. Reasoning is: 1) this is a testing library that a lot of projects depend on, 2) you can never be too paranoid in crypto, 3) should be pretty straightforward.

To get a list of errors, run

$ tsc --build tsconfig.json

Right now I'm seeing 37 errors, I think it's probably 1-2 hours to get working.

So, what's left?

To Do

What Who's in charge Status
Add basic typescript skeleton and open this PR @omarish
Get the parallel typescript source tree to build without any errors. Anyone In progress
Decision: going full typescript. Maintainers
If this library stays JS native, open a PR in (DefinitelyTyped)[https://github.com/DefinitelyTyped] that add these type definitions

Want to make a change? Open a PR against https://github.com/omarish/openzeppelin-test-helpers and I'll merge it in.

Anything I'm missing? I really like Typescript and OpenZeppelin, so I figured this is a good opportunity to learn how to properly add type definitions to a 💯 project.

@frangio
Copy link
Contributor

frangio commented Nov 16, 2020

Thank you so much @omarish! A few notes:

We are interested in migrating the codebase to TypeScript.

Please use this tsconfig.json.

Are you interested in continuing this work? If so, please commit the converted ts files into this branch (and remove the js files).

I would make liberal use of any to get the library to build on TypeScript without errors. Only then I would start filling in with more precise types.

@omarish
Copy link
Author

omarish commented Nov 17, 2020

@frangio Of course, glad I can contribute here.

I just replaced most of the JS files with typescript. There are some files I didn't touch, like truffle config and migrations, etc.

I'm not sure if I have the time to take this all the way to the finish line right now, and I'm hoping that this start will encourage more volunteers to get involved with this effort.

@frangio
Copy link
Contributor

frangio commented Nov 17, 2020

@omarish Feel free to take take this as far as your time allows!

@codler
Copy link

codler commented May 30, 2021

Will this be merged soon?

@FrozenKiwi
Copy link

Hey guys, what's the status of this PR? From a quick look through the code changes it appears as if there is a bit of work to go - but this isn't a large code base.

I have a real aversion to untyped code, what would be the minimal effort expected to complete this task?

@frangio
Copy link
Contributor

frangio commented Dec 8, 2021

@FrozenKiwi This is not a large codebase but converting everything to TypeScript is still probably a significant effort.

I think it might be a better approach to hand-write .d.ts files for each of the modules. What do you think?

@FrozenKiwi
Copy link

I would personally choose to go the full-TS route. I think we agree it's preferable, as you then get the compiler to validate the work (and I trust compilers much more than I would trust anyone - including me).

If you're OK with (very) liberal use of any & //@ts-ignore, we can effectively ignore the bits of the code that isn't public-facing.

@frangio
Copy link
Contributor

frangio commented Dec 10, 2021

I'm ok with going the full TypeScript route and with liberal use of any.

@leiiiooo
Copy link

leiiiooo commented Jun 8, 2022

Annnny update? sir

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.

None yet

5 participants