Open digualu opened 8 years ago
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 mailgun-ts
fork?
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 mailgun.js
to mailgun.ts so you can take a look at what the expected code changes would be.
The process is...
require
s to import
s where possible to leverage underlying libraries typingsclass
definitionsvar
s to let
s and const
s wherever possible (this is a compile time check so it will not introduce any bugs)If that sounds alright and the mailgun.ts
file looks reasonable enough, then I'll continue with the migration while also putting in place the necessary build steps to allow the src/
folder to build into a completely function (and functionally identical) lib/
folder for complete compatibility.
Let me know! abe
Alright, another quick update! I've went ahead and moved all the lib/.js
files to src/.ts
and you can view it here. That's all done with all the tests passing except for the tags().info()
check which I think is failing because my mailgun account doesn't have a tag1
? Anyway regardless we're close.
Now the remaining elephant in the room is the schema.js
file you use to generate the actual structure of the API. I think our best bet will be to extent this format just a little so that it also includes return types for all methods, then we can add in one extra build step which parses the file and generates an associated declaration file which will allow Typescript to use a combination of the built in types (from our work so far) and the generate declaration files to understand both the hard-coded and generated methods.
If this sounds good, I'll progress on that when I get another couple hours.
Annnnd another update! I've added a script bin/dts
which will eventually generate the entire final type declaration file, for now it'll get close, but I had to do some manual changes which are reflected in lib/mailgun.d.ts. Even if you're not familiar with typescript, I suspect you'll be able to get the gist of it. We basically just write signatures for all the generated methods so TS can be aware of them while importing and relying on many of the types that were added to the source for during the JS->TS transition, so an end dev will get served a mix of the generated typings and the actual typings on the underlying classes.
The only thing here that'll jump out to you as potentially scary is the addition of the NewClient
method. This is added so we can cleanly support importing in TypeScript. TS isn't great with "function as a module" setups like we have here, so I've tacked on an optional NewClient
method which allows you to instantiate the library via the traditional...
const mg = require("mailgun-js")(opts);
Or via the TS style
import * as mailgun from "mailgun-js";
const mg = mailgun.NewClient();
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, abe
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. lib
directory) normally just stored in the source code, or is it normally just part of build / prepublish and .gitignored otherwise (similar for example to how we would normally do babel-based transpiling)?
I don't believe there is a standard here and I don't particularly have a preference. I checked in lib/
mostly for your benefit so you could see how it compiles out (and see the "hand made" lib/mailgun.d.ts
)
I would think having it as a prepublish
step would be fine.
Also thanks for taking a look! I'm sorry the diffs aren't cleaner :(
Hello, sorry this took longer than expected. Some comments:
noop
is not used in build.ts
(or in my code). feel free to remove.unref
the timeout. see PR #141.noop
is not used in request.ts
(or in my code). feel free to remove.src
to .npmignore
build
script command to package.json
that would do the build of the libI 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 mailgun-js
wasn't using it already as most of my projects already do and have been using it for some time now. I may try and see what kind of luck I have with typescript-eslint-parser.
I might also fix up the fixture and tests so that it automatically takes the domain from auth.json
rather than be hard coded.
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 .d.ts
file from the above into a TypeScript project and you're good to go. If these types get sufficiently comprehensive (would appreciate some contributions) then they can either be packaged with this or I'll put them up on DefinitelyTyped.
@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: https://github.com/DefinitelyTyped/DefinitelyTyped/pull/24368
And for anyone reading this from the future, types should (hopefully) be available by doing a yarn add -D @types/mailgun-js
:). Please contribute because they sure as heck aren't comprehensive!
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