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

Adds (preliminary) TypeScript definitions #8

Closed
wants to merge 1 commit into from

Conversation

torkelrogstad
Copy link

@torkelrogstad torkelrogstad commented Aug 6, 2018

Closes #7

Adds TypeScript definitions for this library. They were
mostly automatically generated, using the tool pbjs from the
protobufjs library. See README.md for details.

After being generated, most of the work consisted of removing all
references to callbacks (as this library is all about promises),
removing some unused classes and tweaking some definitions. In
particular, openChannel and sendPayment were (and probably others as
well) not correct. Using conditional typings I managed to get some
pretty nice type inference on the various event listeners. Similar work
can probably be done on other methods as well. There were also some work
done on removing all references to number | Long (I replaced them all
with just number), and lot's of places where the definitions would suggest
the API returns null or undefined. If the call succeedes, that should
never happen, AFAIK.

Adds TypeScript definitions for this library. They were
mostly automatically generated, using the tool pbjs from the
protobufjs library. See README.md for details.

After being generated, most of the work consisted of removing all
references to callbacks (as this library is all about promises),
removing some unused classes and tweaking some definitions. In
particular, openChannel and sendPayment were (and probably others as
well) not correct. Using conditional typings I managed to get some
pretty nice type inference on the various event listeners. Similar work
can probably be done on other methods as well. There were also some work
done on removing all references to number | Long (I replaced them all
with just number), and lot's of places where the definitions would suggest
the API returns null or undefined. If the call succeedes, that should
never happen, AFAIK.
@torkelrogstad
Copy link
Author

@bmancini55, would it be possible to get some feedback/progress on this?

@bmancini55
Copy link
Member

Apologies on the delay. Fantastic job putting this together! It's much appreciated and excellent work!

I did find one issue with with CommonJS imports. The only type at the root import is default. After you reference default, the connect method is available. So should hopefully be an easy fix.

screen shot 2018-08-29 at 11 17 00 am

Other than that... due to the size of the TypeScript file, I think this would be better stored in the DefinitelyTyped project (https://github.com/DefinitelyTyped/DefinitelyTyped) and referenced in the lnd-async Readme. This way we can keep the lnd-async library just the source code pertaining to the project. Thoughts?

@torkelrogstad
Copy link
Author

Having them in DefinitelyTyped is reasonable when they're so large. Would it be appropriate to keep the notes on how to generate the types in README? A link to DefinitelyTyped could also be included.

@bmancini55
Copy link
Member

Certainly, I think that would make perfect sense to keep both information about generating type defs and instructions on how to include typing in your project in the README.

@hsjoberg
Copy link

Hi @torkelrogstad.
Great work! Any progress on moving this to DefinitelyTyped?

@torkelrogstad
Copy link
Author

torkelrogstad commented Apr 23, 2019

I have not done any more work on this, I'm afraid. I don't use TS day to day any more, so haven't been a priority.

Feel free to either push to this PR, close it, use the code for a DefinitelyTyped PR, or something else entirely:-)

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.

TypeScript types
3 participants