nit: const payload = github.context.payload as Webhooks.WebhookPayloadPush would eliminate the redundant casts further below

I don't think we should recommend users do that though. Type assertions rely on the developer to perform relevant checks before casting, and this example shows that. Going to take a look at the event_name field and then cast based on that

should an after all restore the original values? otherwise may affect test suites that execute after

The before each function sets up these values
https://github.com/actions/toolkit/pull/292/files#diff-75df1656ab4669c70d84cd99613c8a6eL10

should an after-all cleanup? so the changes don't spill-over and affect other test suites?

Oh sorry, I misunderstood your previous comment. Yeah, I'll go ahead and update this.

EDIT: I'm not sure we should do this. Currently, the only place this env is set is in this suite (or if you happen to have it set on the machine). If we set the value after the suite runs, suites that run before this suite will have different values then suites that run after, changing their behavior if they took a dependency on this value. If another suite needs this value, they should set it. If we build out multiple suites that need it, we should

what is the size impact to consumers of the github module?

None of the code ends up in compiled ncc folders so there is no impact for consumers using that.

However, I think we can install this as a dev dependency, so users doing npm install --prod won't even download it. Going to test that out and make that corresponding change, so the only real users effected would be those upload their node_modules and installed the dev dependencies in there, which is not recommended and should be solved with better messaging and documentation to action authors.

There's a bit of a mixed reaction in the ts/js community, but it looks like we shouldn't install as dev-dependencies we export these types open @types issue

In its current state (saved as a dependency)
Users who upload their production npm packages rather then using ncc are going to have the size of their action increased by the size of the npm package and its dependencies, which is 291 KB or 400kb on disk for my machine.

if we re-export PayloadRepository then can we avoid a new major version?

I don't believe that there's a great way to keep back-compat and set us on a good foot going forward.

We took some of the events and combined them into one big type where we marked each field as optional in WebhookPayload This doesn't really set us on a great path forward because it makes it hard to tell which fields should exist on the event.

For example, a push event would not have an issue or a pull_request, but they are marked as optional when consuming. Ideally, this would be distinct from fields that are actually optional fields, but there is no way to tell now.

It conveys the wrong message to a user that those fields are optional, when in fact they depend on the type of event. Our users who want type safety should be casting based on the type of event, we shouldn't combine every event into one big interface with each field is marked as optional, it makes it harder to consume.

It also sends the wrong message because the individual fields depend on the type of event. For example the "pull_request" field on a PullRequestReview is a different object with different fields from the pull_request field on a "PullRequest" event. payload.pull_request.commits is only a valid field for PullRequests but not PullRequestReviews.

By setting these as union types, we enforce that users need to do a type assertion using the as keyword, or ignore types by using these as any objects