-
Notifications
You must be signed in to change notification settings - Fork 129
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
typings source #100
Comments
Besides just generally looking into it, I have never used nor do I presently use TypeScript anywhere. Not something useful to me personally; and as I am not familiar with the process really, generating a good proper type definition file would be significant effort for me I think. Help or PR would be much appreciated from parties more experienced and knowledgable. |
I could work on this, I'll take a closer look tomorrow. |
If I rewrite this entire library into typescript (which'll just add types in the source, not any other crazyness), would you accept the PR @bojand or should I start a |
Hello, thanks for your interest in helping with this issue! I think I would be open to this. My only concerns specifically for the PR would be:
AFAIK this library is used extensively by a lot of folks and we use it in production as well. While there is always room for improvements (at least in code sense), it more or less just works pretty well. So basically in general I would like the PR to be cohesive and concise and just solve this issue of typescript, and not introduce any other practical side effects if unnecessary. If a rewrite to TypeScript to include type annotation, etc, is the best way to achieve that I am open to it. I just would like to reduce risk of adding any bugs or unintended side effects. Any other enhancements could be iterative afterwards if necessary. Anyway I hope I am making sense; it does sound like this is what you had in mind as well. If this sounds sensible and reasonable to you please go ahead and do up a PR! |
Hey @bojand, Thanks for the quick and well thought-out reply. I agree this library is widely used and it's important to keep the API surface identical while simply adding typings and bringing the code up to date with Typescript standards. I've migrated libraries from JS to TS without introducing any runtime bugs in the past (in fact I generally have caught bugs in the process of adding typings) and I'm confident we could do the same here as the library is small enough with few enough dependencies that I'd be confident in the transition. Obviously the solid test coverage will help ensure that we don't bonk any of the API in the progress and since I don't intend to touch any of the underlying logic, I suspect it'll go pretty smooth. I've went ahead and migrated The process is...
If that sounds alright and the Let me know! |
Alright, another quick update! I've went ahead and moved all the Now the remaining elephant in the room is the If this sounds good, I'll progress on that when I get another couple hours. |
Annnnd another update! I've added a script The only thing here that'll jump out to you as potentially scary is the addition of the
Or via the TS style
It's a no-op for existing devs, but adds some flexibility. You can see the slightly-wacky implementation in mailgun.ts. Anywho - now I really should wait for you feedback as we're nearly done! Thanks again, |
Thanks for this! I will try and take a look later today and tomorrow. Help my ignorance regarding TS please, I can't seem to find the answer quickly by browsing the docs; but is the output of the compilation (ie. |
I don't believe there is a standard here and I don't particularly have a preference. I checked in I would think having it as a Also thanks for taking a look! I'm sorry the diffs aren't cleaner :( |
Hello, sorry this took longer than expected. Some comments:
I say looks good to me. So maybe fix up those and do up a PR. Once merged I might try and fix up the code style a little bit. I prefer standardjs and am surprised I might also fix up the fixture and tests so that it automatically takes the domain from Thanks for your work on this. |
Regarding the schema it's just my manually hand written json schema of the mailgun API based on their documentation. The schema can be extended to include whatever you wish more or less. so I would think it can be extended somehow so we can parse it and generate type declaration for the generated api. |
Hello, is their any progress on this ? |
I'm not quite sure why so much work has happened on this. I understand the preference to want to generate typings by rewriting the whole project in typescript, or by generating types from the schema (as the schema is fundamentally the source of truth for the api), because then it's all automated. But how likely is it that the schema will change, breakingly? Similarly, how much effort is it to rewrite this whole package in typescript? Months apparently. We can just manually define a type declaration file that mirrors the current structure. If the Mailgun REST API changes, then the schema file needs to be changed anyway, so update the typings along with it. Not that difficult. I've spent a few hours and I've added typing info for maybe half of this API. https://github.com/sampsonjoliver/types-bojand-mailgun-js Pretty much just need to include the |
@sampsonjoliver Since I haven't seen any progress on this, would you mind publishing the types on DefinitelyTyped? |
@Cyberuben Absolutely. I haven't used this project in a few months now, so I can't comment as to the staleness of these types, but a PR to merge what I have done exists for DT here: DefinitelyTyped/DefinitelyTyped#24368 And for anyone reading this from the future, types should (hopefully) be available by doing a |
Have you tried ts-mailgun? |
My application uses typescript and I am not able to find the type definition.
$ typings search mailgun
No results found for search
Any chance to generate it?
Thanks
The text was updated successfully, but these errors were encountered: