Skip to content

chore: add TypeScript definition file for most API #13

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

Merged
merged 1 commit into from
Dec 14, 2016

Conversation

eventualbuddha
Copy link

No description provided.

Copy link
Member

@alangpierce alangpierce left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This all looks good to me. Did you end up basically reading through all the code and translating it to type definitions, or was there a way or automating it? (Or did you find these definitions somewhere?)

Also, as I understand it, a mistake here could potentially cause runtime errors, but then we can just go and fix the type declarations, so it's not such a huge deal if this isn't 100% correct. Is that how you're thinking about it as well? It would require an npm publish and version bump on every change, right? My original vision for this fork was for it to be a small set of patches that are easy to read and understand, although I also don't feel too strongly about that, and this types stuff is pretty isolated from the actual CoffeeScript code anyway.

Also, did you consider making these type definitions external rather than in this repo? That's what we'd need to do for libraries we don't have control over, right? I guess one advantage of keeping type definitions with the source code is that it makes versioning easier?

Also, TypeScript has a way of importing a library where everything is the any type so you don't have to bother with type definitions, right? (Like flow does by default.) TBH this commit seems like a lot of up-front work to use an external library. But I guess it especially makes sense for things like AST traversals that it's nice to have real types.

@eventualbuddha
Copy link
Author

eventualbuddha commented Dec 14, 2016

Did you end up basically reading through all the code and translating it to type definitions?

Yeah, basically by hand.

…a mistake here could potentially cause runtime errors…

Nothing in the type declaration file itself is part of the runtime, but if the type declaration doesn't match reality and someone codes against it then yes, it could cause errors.

Also, did you consider making these type definitions external rather than in this repo?

Yeah. I don't have a strong opinion about that. Could put it into the DefinitelyTyped repo for decaffeinate-coffeescript.

But I guess it especially makes sense for things like AST traversals that it's nice to have real types.

Yeah, that's what I'm aiming for. We'll see if it's actually worth it.

@eventualbuddha
Copy link
Author

I'll wait to merge this until you state your preferences on external vs. embedded types, @alangpierce.

@alangpierce
Copy link
Member

No strong opinion from me either, this seems fine, so feel free to merge. Thanks!

@eventualbuddha
Copy link
Author

Still need to figure out importing non-main files for e.g. nodes.

@eventualbuddha eventualbuddha merged commit 458d042 into decaffeinate-fork-1.10.0 Dec 14, 2016
@eventualbuddha eventualbuddha deleted the typescript-definition branch December 14, 2016 20:39
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