Add TypeScript typings definition file for v3 API by notheotherben · Pull Request #251 · sendgrid/sendgrid-nodejs · GitHub
Skip to content

Add TypeScript typings definition file for v3 API#251

Merged
thinkingserious merged 1 commit into
sendgrid:masterfrom
EMSSConsulting:feat/typescript-definitions
Sep 15, 2016
Merged

Add TypeScript typings definition file for v3 API#251
thinkingserious merged 1 commit into
sendgrid:masterfrom
EMSSConsulting:feat/typescript-definitions

Conversation

@notheotherben

@notheotherben notheotherben commented Jun 27, 2016

Copy link
Copy Markdown
Contributor

Hi guys,

Recently updated an application of ours to use your 3.0.0 library and noticed there weren't any up to date typings files in the wild. Seeing as the current approach with TypeScript is to embed these definition files in their respective NPM modules, I've taken the time to put together a compatible set of type definitions for your library.

In addition to adding the raw type definitions, I've also taken the liberty of adding a quick test that ensures the definition file is well formatted, it should help ensure that things aren't broken accidentally going forward.

As far as manual verification of the typings file goes, aside from running your unit test suites through the TypeScript compiler to confirm that they comply with the typings file (always a good way to ensure the types match what's expected), we're also running these typings on a production system with no issues as yet.

I'll ensure I sign the contributor license agreement and get that through to you guys, in the mean time if you wouldn't mind taking a look and letting me know if there's anything that stands out as being incorrect, or raising any issues you may have with this addition to the codebase.

Things that may be issues

  • There's now a devDependency on the TypeScript compiler. While it isn't significantly large, I notice you've tried to keep the number of dependencies you have to a minimum (build times I assume being the reason). We can easily remove it should you wish, however that will remove the ability to test that the definitions are indeed valid.
  • There's something else to manage as the API changes. Unfortunately that's not something one can do much about (aside from switching to TypeScript for the library, not really an option). What I've done previously with projects on which we hold a dependency is offer to assist in updating the definitions whenever the API does change. In exchange I just ask that any of the project leads try and keep me informed (using a CC) whenever they are aware of any API changes.

Kind regards and all the best,
Benjamin

@notheotherben

notheotherben commented Jun 27, 2016

Copy link
Copy Markdown
Contributor Author

@notheotherben

Copy link
Copy Markdown
Contributor Author

Hey guys, any word on whether this'll get included or whether I should instead be investigating other approaches to publishing it? Still having to hack our way around the lack of a usable type definition from any official source.

@thinkingserious

Copy link
Copy Markdown
Contributor

Hello @spartan563,

It's still on our backlog and we plan to merge it.

The only way this can move up our backlog queue is through +1's or comments on this thread.

Thanks for checkin in!

@notheotherben

Copy link
Copy Markdown
Contributor Author

I see, not a problem at all so long as there's a process in place - let me know if you need anything from me at any point and I'll do my best to help out.

@CelsoSantos

CelsoSantos commented Aug 10, 2016

Copy link
Copy Markdown

I too am writing an application in TypeScript and also need to use SendGrid. It's not that it's impossible to use without the Definition file, but it does become much harder as one has to constantly check on documentation instead of getting intellisense (in visual studio code) completions.

@thinkingserious

Copy link
Copy Markdown
Contributor

Thanks for the upvote @CelsoSantos, it helps move this issue up our queue.

@thinkingserious

Copy link
Copy Markdown
Contributor

Hi @spartan563,

Could you please resolve the conflicts so that I may test and merge? Thanks!

@notheotherben notheotherben force-pushed the feat/typescript-definitions branch from 596ea71 to 3b982fd Compare September 15, 2016 07:48
@notheotherben

Copy link
Copy Markdown
Contributor Author

Hi @thinkingserious, should be up to date, rebased on master.

@thinkingserious thinkingserious merged commit d4acbce into sendgrid:master Sep 15, 2016
@thinkingserious

Copy link
Copy Markdown
Contributor

@spartan563 please email us at dx@sendgrid.com with your T-shirt size and mailing address :)

thinkingserious added a commit that referenced this pull request Sep 15, 2016
@frederickfogerty

frederickfogerty commented Sep 19, 2016

Copy link
Copy Markdown

Hey @spartan563 thanks for adding this to the lib, I LOVE it when modules have type definitions in their core. 🙌

I'm having a problem when trying to use this module in my TypeScript project. I'm receiving this error when exporting a SendGrid instance: Exported variable 'sendGrid' has or is using name 'SendGrid.SendGrid' from external module "[...]/node_modules/sendgrid/index" but cannot be named.

This is a repro:

import { SendGrid } from 'sendgrid';
import * as SG from 'sendgrid'; // This is what we usually do to try squash the error, it doesn't work in this scenario :(

const sendGrid = SendGrid(process.env.SENDGRID_API_KEY);
//    ^^^^^^^^ error here

export default sendGrid;

And since SendGrid is a namespace, I can't access it import it (?!), and thus can't explicitly type sendGrid with something like const sendGrid: SendGrid.SendGrid = ....

🙏 🙌 Any help would be greatly appreciated, don't make us fall back to any! 😱

@notheotherben

Copy link
Copy Markdown
Contributor Author

Hi @frederickfogerty, just spun up an essentially empty project and wasn't able to reproduce the issue you're describing.

I've put down the example code you gave me in a quick repo which demonstrates this, if you could please fork it and make any changes necessary on your side to reproduce the issue then I'd be more than willing to look into it further.

Repo: https://github.com/SPARTAN563/sendgrid-ts-demo

Regards,
Benjamin

@frederickfogerty

Copy link
Copy Markdown

Hey @spartan563, thanks for taking a look at my issue, I really appreciate it.

I have submitted a PR to your repo which reproduces the issue: notheotherben/sendgrid-ts-demo#1

Thanks!

notheotherben pushed a commit to notheotherben/sendgrid-nodejs that referenced this pull request Sep 20, 2016
See sendgrid#251 and <notheotherben/sendgrid-ts-demo#1> for more details on what wasn't working
@thinkingserious

Copy link
Copy Markdown
Contributor

@frederickfogerty,

Could the issue be that you are using sendGrid instead of SendGrid?

@frederickfogerty

Copy link
Copy Markdown

shkabo pushed a commit to shkabo/sendgrid-nodejs that referenced this pull request Oct 5, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type: community enhancement feature request not on Twilio's roadmap

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants