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(typescript): mark verify parameters as required #373

Merged
merged 2 commits into from
Dec 5, 2020

Conversation

G-Rath
Copy link
Member

@G-Rath G-Rath commented Dec 4, 2020

The parameters for verify are not optional - this matches the pattern used for sign.

@G-Rath G-Rath force-pushed the dont-mark-params-as-optional branch 5 times, most recently from 3443749 to 203305c Compare December 4, 2020 22:50

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@G-Rath G-Rath force-pushed the dont-mark-params-as-optional branch from 203305c to 4fb1a78 Compare December 4, 2020 22:54
const matchesSignature = verify(state.secret, event.payload, event.signature);
// verify will validate that the secret is not undefined
const matchesSignature = verify(
state.secret!,
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm typically not a fan of using ! - ideally I'd have an assertion here about the state.secret, but tbh this isn't the worse use of it 🤷

Copy link
Contributor

Choose a reason for hiding this comment

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

We are all still figuring out the whole TypeScript thing, we got much better at it since we migrated @octokit/webhooks, but there is surely lots of potential of improvement :)

Copy link
Member Author

Choose a reason for hiding this comment

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

No problem - I'm a big TS fan myself, so happy to help and be pinged on things :)

@gr2m gr2m added Type: Bug Something isn't working as documented, or is being fixed typescript Relevant to TypeScript users only labels Dec 5, 2020
Copy link
Contributor

@gr2m gr2m left a comment

Choose a reason for hiding this comment

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

Great changes, thanks!

@gr2m gr2m merged commit fc4ddd3 into octokit:master Dec 5, 2020
@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 2020

🎉 This PR is included in version 7.16.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@G-Rath G-Rath deleted the dont-mark-params-as-optional branch December 5, 2020 21:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Bug Something isn't working as documented, or is being fixed typescript Relevant to TypeScript users only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants