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

fix: update definitions for tsc 2.6, fixes #618 #619

Merged
merged 2 commits into from
Jan 19, 2018

Conversation

phra
Copy link
Contributor

@phra phra commented Nov 17, 2017

i've updated the definitions files to support typescript 2.6. fixes #618

@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Nov 17, 2017
@SendGridDX
Copy link
Collaborator

SendGridDX commented Nov 17, 2017

CLA assistant check
All committers have signed the CLA.

@phra
Copy link
Contributor Author

phra commented Nov 27, 2017

i'm still depending on quickfix in order to be able to deploy my project to production.. can you please merge this?

@thinkingserious
Copy link
Contributor

@spartan563,

If you have a moment, a quick review would be much appreciated. Thanks!

@notheotherben
Copy link
Contributor

Looks perfect to me 👍

@thinkingserious
Copy link
Contributor

Awesome, this has been added to our backlog for final review and merge.

@phra
Copy link
Contributor Author

phra commented Nov 28, 2017

@thinkingserious i've also sent #621 to fix the CI, i don't know if it's still required but 11 days ago was needed to make the CI work again.

@ShaharHD
Copy link

ShaharHD commented Dec 10, 2017

Been almost 3 weeks now. Any ETA on getting this small fix merged?
It does not have implications other than making this library usable in TS 2.6 from what I can see.

@joseSantacruz
Copy link

Hi, I also need to merge this PR to deploy to prod

@wwwy3y3
Copy link

wwwy3y3 commented Dec 13, 2017

+1 need this PR.
@thinkingserious any plan for this fix?

@thinkingserious
Copy link
Contributor

Hello @ShaharHD @joseSantacruz @wwwy3y3,

This PR is on our backlog, which we are working through from oldest to newest. Our backlog is a bit overloaded because of Hacktoberfest, tub we are currently working to get caught up.

My apologies for the delay and I do appreciate your support and patience.

With Best Regards,

Elmer

@thinkingserious
Copy link
Contributor

Thanks for taking the time to check it out @jdanyow!

@ShaharHD
Copy link

ShaharHD commented Jan 13, 2018

@thinkingserious Why wasn't this included in 6.2.0?

@phra
Copy link
Contributor Author

phra commented Jan 15, 2018

hopefully i can continue to ship to production thanks to quickfix. ✌️

@ShaharHD @thinkingserious @spartan563 this PR should be merged ASAP because doesn't have any conflicts or regressions and fixes problems with typescript 2.6, so i don't see any reason to not merge this!

@thinkingserious
Copy link
Contributor

Thanks @phra!

I have this on my list to deploy today.

@thinkingserious
Copy link
Contributor

Hi @phra,

Before I merge this, could you please take a look at this recently merged PR to see if any further TypeScript changes are necessary? Thanks!

With Best Regards,

Elmer

@notheotherben
Copy link
Contributor

It looks like there are some changes that are necessary to align it with the newly exported module spec. Specifically, rather than simply exporting Client, we should be exporting something like Client & { Client: typeof Client }. The same applies to the MailService export.

Should be a pretty easy fix.

@phra
Copy link
Contributor Author

phra commented Jan 17, 2018

i will do it later today. 👍

@phra
Copy link
Contributor Author

phra commented Jan 17, 2018

done. can you review the code? @thinkingserious @spartan563

Copy link
Contributor

@notheotherben notheotherben left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍 thanks @phra

@thinkingserious
Copy link
Contributor

Thanks to @phra and @spartan563 for being AWESOME and getting this done so quickly! I should have this deployed no later than tomorrow.

@bunnyvishal6
Copy link

@thinkingserious waiting for this pr to be merged. When will it be done?

@thinkingserious
Copy link
Contributor

@bunnyvishal6,

I'm shooting for some time today.

@thinkingserious thinkingserious merged commit 9e43910 into sendgrid:master Jan 19, 2018
@thinkingserious
Copy link
Contributor

Hello @phra,

Thanks again for the PR!

We want to show our appreciation by sending you some swag. Could you please fill out this form so we can send it to you? Thanks!

Team SendGrid DX

@0x80
Copy link

0x80 commented Jan 19, 2018

Thanks for fixing this 👍 I can now set the compiler back to strict mode 😄

@phra phra deleted the fix/typescript-definitions branch January 19, 2018 10:51
@thinkingserious
Copy link
Contributor

Awesome, thanks for the comment @0x80!

@hansenst
Copy link

Hey @phra,
In commit 37c22e7 you forgot the typeof. @spartan563 suggested to add Client & { Client: typeof Client }, but you added only Client & { Client: Client } and that breaks the bindings.

@phra
Copy link
Contributor Author

phra commented May 23, 2018

@hansenst at the time of committing, it was working properly. can you check it using tsc version ~2.6 ? i can eventually send another PR fixing it if it doesn't break v2.6 release or look for a solution that can work on tsc 2.6 and newer versions.

@hansenst
Copy link

Sorry @phra, I was somewhat unclear in my comment. Typescript itself is happy with the current types, but calling the setApiKey fails (it just gets stuck and eventually things time out). Changing the type definitions to Client & { Client: typeof Client } resolves the issue.

We're currently using TS 2.8.3.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: code review request requesting a community code review or review from Twilio
Projects
None yet
Development

Successfully merging this pull request may close these issues.

typescript definitions errors