[go: nahoru, domu]

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

typings source #100

Open
digualu opened this issue Jul 11, 2016 · 16 comments
Open

typings source #100

digualu opened this issue Jul 11, 2016 · 16 comments

Comments

@digualu
Copy link
digualu commented Jul 11, 2016

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

@bojand
Copy link
Collaborator
bojand commented Jan 24, 2017

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.

@ZLima12
Copy link
ZLima12 commented Feb 27, 2017

I could work on this, I'll take a closer look tomorrow.

@abeisgoat
Copy link

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?

@bojand
Copy link
Collaborator
bojand commented Jul 15, 2017

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:

  • The general mechanisms of the generation of the client based on JSON schema remains unchanged
  • The general structure and resulting API should not really change significantly
  • No real or significant changes to the functionality of the library are introduced

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!

@abeisgoat
Copy link
abeisgoat commented Jul 16, 2017

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...

  1. Switch requires to imports where possible to leverage underlying libraries typings
  2. Move prototypical class definitions to Typescript class definitions
  3. Add typings to all variables, fields and arguments
  4. Move vars to lets and consts 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

@abeisgoat
Copy link

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.

@abeisgoat
Copy link

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

@bojand
Copy link
Collaborator
bojand commented Jul 17, 2017

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)?

@abeisgoat
Copy link
abeisgoat commented Jul 17, 2017

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 :(

@bojand
Copy link
Collaborator
bojand commented Jul 21, 2017

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 Unref timeout #141.
  • noop is not used in request.ts (or in my code). feel free to remove.
  • add src to .npmignore
  • can you add a build script command to package.json that would do the build of the lib
  • I am leaning towards building as a prepublish step and not including it in source control. Opinion on this?

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 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.

@bojand
Copy link
Collaborator
bojand commented Jul 21, 2017

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.

@bilby91
Copy link
bilby91 commented Sep 5, 2017

Hello, is their any progress on this ?

@sampsonjoliver
Copy link

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.

@Cyberuben
Copy link

@sampsonjoliver Since I haven't seen any progress on this, would you mind publishing the types on DefinitelyTyped?

@sampsonjoliver
Copy link
sampsonjoliver commented Mar 19, 2018

@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 yarn add -D @types/mailgun-js :). Please contribute because they sure as heck aren't comprehensive!

@DrewImm
Copy link
DrewImm commented Nov 11, 2019

Have you tried ts-mailgun?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

8 participants