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

Add Typescript type definitions #54

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

TheActualWalko
Copy link

No description provided.

@gerardobort
Copy link
Owner

Hi @TheActualWalko! Thank you so much for your contribution.
I think this is a great addition to the project, specially for the TS folks. I'd like to know if there's a way to automate a test or something, to make sure these type definitions will remain in sync with the actual JS code. I'm not that familiar with TS, but as the code evolves I'm concerned how this would evolve as well. Any hint?

@TheActualWalko
Copy link
Author

@gerardobort my pleasure! Good idea re: tests to confirm the types stay in sync. I'm relatively seasoned with TS, but this is my first time submitting type annotations for an all-JS project - I'm afraid I haven't got any firsthand experience implementing such tests. After looking around, this looks like a promising option from the TS implementers: https://github.com/microsoft/dtslint.

Here it is in action on the express.js type library: https://github.com/DefinitelyTyped/DefinitelyTyped/blob/master/types/express/express-tests.ts#L124

I can implement this when I have some free time. However, the ultimate way of accomplishing this would be to convert the project to TS. I would be more than happy to help if you find yourself coming to believe that it would be a worthwhile move. It would be ideal to make the switch before the project grows in size.

If I may make one remark to support going TS, the work I did digging around for public type annotations did reveal a few potential bugs (mostly toJSON methods including class instances in their return values - check out my comments in the .d.ts file for more details) and going all-in on typing would almost certainly reveal more potential improvements.

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