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

Add @octokit/webhooks #245

Closed

Conversation

tyankatsu0105
Copy link

re #147

  • Refactor packages/github's interface
    • Quote from @octokit/webhooks declaration

@ericsciple
Copy link
Contributor

@hross?

@ericsciple
Copy link
Contributor

@hross sorry at a glance i thought this was related to the change you made. it looks like this is about the github.context object instead

@hross
Copy link
Contributor

hross commented Dec 17, 2019

@ericsciple these changes are in the same spirit as my changes (importing types instead of re-defining our own). I think they are good changes that we should add but I don't have a good way of testing them. @thboop has a similar issue on his plate. Maybe worth setting up something in canary that would test the toolkit with webhooks like we did for my changes.

@srb2
Copy link

srb2 commented Dec 20, 2019

thank you very much :) appreciated

@thboop
Copy link
Collaborator

thboop commented Dec 20, 2019

Hey @srb2, thanks for the excellent PR!

@hross, @ericsciple I'm going to merge this unless you have any reservations, I've tested the changes they seem good and its the correct step forward for the project.

EDIT: we need one minor change before we can merge this.

}
repository?: Webhooks.PayloadRepository
issue?: Webhooks.WebhookPayloadIssuesIssue
pull_request?: Webhooks.WebhookPayloadPullRequest
Copy link
Collaborator

Choose a reason for hiding this comment

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

This should be WebhookPayloadPullRequestPullRequest

Copy link
Author

Choose a reason for hiding this comment

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

Thanks.
Done.

Copy link
Author

Choose a reason for hiding this comment

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

There is type error

Copy link
Author

Choose a reason for hiding this comment

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

Fix.

Copy link
Collaborator

@thboop thboop left a comment

Choose a reason for hiding this comment

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

One of the interfaces needs to be updated the best rest looks good to me!

@tyankatsu0105
Copy link
Author

Umm...
This PR's author is me. @thboop

@tyankatsu0105 tyankatsu0105 requested a review from thboop December 21, 2019 01:23
@tyankatsu0105
Copy link
Author

@thboop
Bumping to v3.0.0 is great.
But I need more quote type from @octokit/webhooks.
Because I think there are some types that not support yet.
So, I suppose it's early timing when bumping version.
What do you think about this?

@ericsciple
Copy link
Contributor

@bryanmacfarlane thoughts?

Do you know what the original motivation was, that these types were redefined rather than re-exported?

The size concerns me. Installing @octokit/webhooks@^7 adds 350k uncompressed, 50k compressed

There are thousands of lines of potential WebhookPayload* type definitions. I'm wondering about the original motivation of @actions/github. Why only a subset of types? Why not OR all types together? Are all types are even compatible (can they be OR'd)?

Assuming only a subset is desired (say for size consideration), at some point the solution is for the consumer to import @octokit/webhooks and cast to the correct type. Should we update scenario docs?

Is the motivation for bumping to v3.0.0 because PayloadRepository was removed? Do we need to make this breaking change? An alternative would be to re-export the interface:

import {PayloadRepository} from '@octokit/webhooks'
export {PayloadRepository}

@thboop
Copy link
Collaborator

thboop commented Jan 24, 2020

@tyankatsu0105 ,

I've added #310 which provides instructions on how to do this manually if needed.

If there is a particularly field you think would be helpful to add, we can add that to the existing typescript definitions, feel free to open a PR or create an issue!

Currently, this pr sets each field to its most common definition. For example, it sets pull_request to WebhookPayloadPullRequestPullRequest. That holds true for the pull_request event, but the pull_request_review event has pull_request field correspond to the type WebhookPayloadPullRequestReviewPullRequest. These have different fields, so the type information is inaccurate for this event.

If we are going to to import the @octokit/webhook definitions, we should look at importing the full payload types. The WebhookPayload field here is really just a union type between all of the possible payloads.

  | Webhooks.WebhookPayloadPush // If the event is "push"
  | Webhooks.WebhookPayloadPullRequest // if the event is "pull_request"
  | Webhooks.WebhookPayloadPullRequestReview // if the event is "pull_request_review"
 // ect

This would require bumping the major version of the module, and we don't want to push out a major version for just this at this time. I've tagged #147 with the enhancement flag and we will consider implementing it a future release.

@tyankatsu0105
Copy link
Author

@thboop
OK. I understand.
I close this PR.

@tyankatsu0105 tyankatsu0105 deleted the refactor/github-interface branch February 7, 2020 01:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants