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

feat: use new types from @octokit/webhooks-definitions #406

Merged
merged 2 commits into from
Feb 2, 2021

Conversation

G-Rath
Copy link
Member

@G-Rath G-Rath commented Jan 15, 2021

Here it is 🎉

There's a few things to polish off and discuss before landing this, but the bulk of it is done so making the PR now.

  • we're missing some events in our schema definitions.
  • I've done a big rename of the types so that they're more accurate (have exported the renamed types under their old names too to be less breaking).
  • I've improved the types in places so that they're accurate
  • have refactored the type generation script into TS - happy to move it back to JS, or to convert the other scripts into TS
    • I tried originally trying to build the type via types, but it's just too hard (if at all possible) - you can get close with 4.1 features, but even then it's a lot of work for TS to do meaning IDE autocompletion suffers vs us just generating a static type
  • probably something else I've forgotten to mention 😅

I'm breaking some things out into their own little PRs where possible.

@wolfy1339 wolfy1339 added the Type: Feature New feature or request label Jan 15, 2021
@wolfy1339 wolfy1339 added the typescript Relevant to TypeScript users only label Jan 15, 2021
@G-Rath G-Rath force-pushed the refactor-types branch 2 times, most recently from 2fc7296 to 4de05e7 Compare January 17, 2021 02:23
@wolfy1339 wolfy1339 linked an issue Jan 17, 2021 that may be closed by this pull request
@wolfy1339 wolfy1339 mentioned this pull request Jan 17, 2021
@G-Rath G-Rath force-pushed the refactor-types branch 2 times, most recently from c734c33 to 66ea236 Compare January 24, 2021 21:31
wolfy1339
wolfy1339 previously approved these changes Jan 24, 2021
@G-Rath G-Rath requested a review from wolfy1339 January 24, 2021 21:35
@G-Rath G-Rath marked this pull request as ready for review January 24, 2021 21:35
@G-Rath G-Rath requested review from oscard0m and gr2m January 24, 2021 21:37
@G-Rath
Copy link
Member Author

G-Rath commented Jan 24, 2021

I think we're good to go - note this'll be rebased at least twice when I merge in #403 & #410, so approves won't stick until then

While I've marked everything in the old generated payloads as deprecated, I'd like to delete them asap.
Also, there's probably a couple of redundant type aliases in there that could be cleaned up, but shouldn't be a big deal if that happens later.

wolfy1339
wolfy1339 previously approved these changes Jan 24, 2021
Copy link
Member

@oscard0m oscard0m left a comment

Choose a reason for hiding this comment

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

I don't feel confident enough to give an approval but, for what I read, everything looks good to me :)

@G-Rath G-Rath force-pushed the refactor-types branch 3 times, most recently from ef605e6 to e9a4e48 Compare January 24, 2021 23:12
@G-Rath G-Rath requested review from oscard0m and wolfy1339 January 24, 2021 23:12
src/generated/webhook-names.ts Outdated Show resolved Hide resolved
src/generated/get-webhook-payload-type-from-event.ts Outdated Show resolved Hide resolved
@gr2m
Copy link
Contributor

gr2m commented Jan 24, 2021

While I've marked everything in the old generated payloads as deprecated, I'd like to delete them asap.

Should these deprecations be part of this PR or did they already happen?

@octokit/webhooks has several deprecated APIs, we can cut a breaking release anytime and get rid of them all at once.

@G-Rath
Copy link
Member Author

G-Rath commented Jan 29, 2021

I've pinned the version constraint for @octokit/webhooks-definitions to be an exact version, and updated the update workflow to update the constraint with the latest version.

The reason for this is that otherwise it can be updated by anything that regenerates the lockfile, which we've got renovate doing every week, but we need to make sure generate-types is run every time the version of this package changes which won't happen so it's PRs will always fail CI.

wolfy1339
wolfy1339 previously approved these changes Jan 29, 2021
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.

My understanding is that we can merge this in as a feature release, as it does not introduce any breaking changes? If so, +1 for merging from me. Otherwise please let me know what the breaking changes are. If breaking changes are TypeScript types only we can release a feature release, I don't consider TypeScript type changes breaking changes as they don't affect production code, they only occur at build time

.github/workflows/update.yml Outdated Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
src/event-handler/remove-listener.ts Show resolved Hide resolved
@G-Rath G-Rath force-pushed the refactor-types branch 2 times, most recently from 6bbf256 to ea27ffa Compare January 31, 2021 19:01
gr2m
gr2m previously approved these changes Jan 31, 2021
wolfy1339
wolfy1339 previously approved these changes Feb 2, 2021
@G-Rath
Copy link
Member Author

G-Rath commented Feb 2, 2021

Alright, let's do this! we can fix up any issues in post 😎

@G-Rath G-Rath merged commit d3df9f3 into octokit:master Feb 2, 2021
@G-Rath G-Rath deleted the refactor-types branch February 2, 2021 17:53
@G-Rath
Copy link
Member Author

G-Rath commented Feb 2, 2021

Error: src/event-handler/index.ts(10,17): error TS4058: Return type of exported function has or is using name 'BaseWebhookEvent' from external module "/home/runner/work/webhooks.js/webhooks.js/src/types" but cannot be named.

well, that's odd. I'll see if I can figure out what's going wrong.

@G-Rath
Copy link
Member Author

G-Rath commented Feb 2, 2021

I know why this is breaking - will have a PR up shortly

@github-actions
Copy link
Contributor

github-actions bot commented Feb 2, 2021

🎉 This PR is included in version 7.22.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature New feature or request typescript Relevant to TypeScript users only
Projects
None yet
Development

Successfully merging this pull request may close these issues.

pull_request.merged event listener never invoked
4 participants