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 MailService constructor property definition in TypeScript declaration #921

Merged
merged 2 commits into from
Feb 12, 2020

Conversation

dhritzkiv
Copy link
Contributor

@dhritzkiv dhritzkiv commented Apr 26, 2019

Before, {MailService: MailService} suggested an instance of MailService was being passed. Changing the syntax to {new(): MailService} typeof MailService suggests that it's a callable constructor reference available as a property

Fixes

Checklist

  • I have made a material change to the repo (functionality, testing, spelling, grammar)
  • I have read the [Contribution Guide] and my PR follows them.
  • I updated my branch with the development branch.
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation about the functionality in the appropriate .md file
  • I have added in line documentation to the code I modified

Short description of what this PR does:

If you have questions, please send an email to SendGrid, or file a Github Issue in this repository.

Before, `{MailService: MailService}` suggested an instance of `MailService` was being passed. Changing the syntax to `{new(): MailService}` suggests that it's a callable constructor reference available as a property
@thinkingserious thinkingserious added the status: code review request requesting a community code review or review from Twilio label Apr 26, 2019
@SendGridDX
Copy link
Collaborator

SendGridDX commented Apr 26, 2019

CLA assistant check
All committers have signed the CLA.

@dhritzkiv dhritzkiv changed the title Fix MailService constructor property definition Fix MailService constructor property definition in TypeScript declaration Apr 26, 2019
@thinkingserious
Copy link
Contributor

@spartan563,

Do you mind giving this one a quick look? Thanks!

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.

Good catch, however I believe you can also use typeof MailService to more correctly reflect the fact that the property is actually exposing the full (uninstantiated) type.

Use `typeof MailService` over `{new (): MailService}` to indicate constructor reference
@dhritzkiv
Copy link
Contributor Author

@spartan563 you're right; made the change to typeof

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 great, thanks Daniel.

@bs85
Copy link

bs85 commented Dec 16, 2019

@thinkingserious any reason this wasn't merged? This is forcing us to use ugly hacks anywhere we need to create a new instance of that class.

@bs85 bs85 mentioned this pull request Dec 20, 2019
@childish-sambino childish-sambino force-pushed the master branch 5 times, most recently from 389fa8c to 37473ad Compare January 15, 2020 21:44
@childish-sambino childish-sambino changed the title Fix MailService constructor property definition in TypeScript declaration fix: update MailService constructor property definition in TypeScript declaration Feb 12, 2020
@childish-sambino childish-sambino merged commit d5cc545 into sendgrid:master Feb 12, 2020
@thinkingserious
Copy link
Contributor

Hello @dhritzkiv,

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

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.

6 participants