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

Update Payload Interface to use Definitions from @octokit/webhooks #292

Closed
wants to merge 8 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
28 changes: 28 additions & 0 deletions packages/github/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,31 @@ const newIssue = await octokit.issues.create({
body: 'Hello Universe!'
});
```

### Context Payload Casting in Typescript
The [GitHub Webhook Event](https://developer.github.com/webhooks/#events) payload is provided as a part of the context. Based on the type of event that [triggered your workflow](https://help.github.com/en/actions/automating-your-workflow-with-github-actions/events-that-trigger-workflows), the structure of that object may change. You can cast this field to access these members in a type safe way.

```ts
import * as core from '@actions/core'
import * as github from '@actions/github'
import * as Webhooks from '@octokit/webhooks'

const payload = github.context.payload
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@thboop thboop Jan 13, 2020

Choose a reason for hiding this comment

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

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

if (github.context.eventName === 'push') {
const pushPayload = github.context.payload as Webhooks.WebhookPayloadPush
core.info(`The head commit is: ${pushPayload.head_commit}`)
}
```

There may be cases where multiple events have a field you need. For example, we can get the issue number field from the `issue` event and the `issue_comment` event
```ts
import * as core from '@actions/core'
import * as github from '@actions/github'
import * as Webhooks from '@octokit/webhooks'

const payload = github.context.payload
if (payload && 'pull_request' in payload) {
core.info(`The issue is: ${JSON.stringify(payload.pull_request.number)}`)
}
```

36 changes: 22 additions & 14 deletions packages/github/__tests__/lib.test.ts
Original file line number Diff line number Diff line change
@@ -1,8 +1,13 @@
import * as path from 'path'
import {Context} from '../src/context'
import {PayloadRepository} from '@octokit/webhooks'
import {WebhookPayload} from '../src/interfaces'

/* eslint-disable @typescript-eslint/no-require-imports */

// TODO: https://github.com/actions/toolkit/issues/291, ESLint chokes on the a?.b syntax introduced in Typescript 3.7
/* eslint-disable @typescript-eslint/no-object-literal-type-assertion */

describe('@actions/context', () => {
let context: Context

Expand All @@ -16,11 +21,18 @@ describe('@actions/context', () => {
expect(context.payload).toEqual(require('./payload.json'))
})

it('returns an empty payload if the GITHUB_EVENT_PATH environment variable is falsey', () => {
it('returns an undefined payload if the GITHUB_EVENT_PATH environment variable is falsey', () => {
delete process.env.GITHUB_EVENT_PATH

context = new Context()
expect(context.payload).toEqual({})
expect(context.payload).toEqual(undefined)
})

it('returns an undefined payload if the GITHUB_EVENT_PATH environment variable does not point to a file', () => {
process.env.GITHUB_EVENT_PATH = path.join(__dirname, 'invalidfile.json')

context = new Context()
expect(context.payload).toEqual(undefined)
})

it('returns attributes from the GITHUB_REPOSITORY', () => {
Expand All @@ -29,18 +41,13 @@ describe('@actions/context', () => {

it('returns attributes from the repository payload', () => {
delete process.env.GITHUB_REPOSITORY

context.payload.repository = {
name: 'test',
owner: {login: 'user'}
}
expect(context.repo).toEqual({owner: 'user', repo: 'test'})
})

it("return error for context.repo when repository doesn't exist", () => {
delete process.env.GITHUB_REPOSITORY

context.payload.repository = undefined
delete process.env.GITHUB_EVENT_PATH
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@thboop thboop Jan 13, 2020

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@thboop thboop Jan 13, 2020

Choose a reason for hiding this comment

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

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

context = new Context()
expect(() => context.repo).toThrowErrorMatchingSnapshot()
})

Expand All @@ -55,9 +62,10 @@ describe('@actions/context', () => {
it('works with pullRequest payloads', () => {
delete process.env.GITHUB_REPOSITORY
context.payload = {
pullRequest: {number: 2},
repository: {owner: {login: 'user'}, name: 'test'}
}
// eslint-disable-next-line @typescript-eslint/camelcase
pull_request: {number: 2},
repository: {owner: {login: 'user'}, name: 'test'} as PayloadRepository
} as WebhookPayload
expect(context.issue).toEqual({
number: 2,
owner: 'user',
Expand All @@ -69,8 +77,8 @@ describe('@actions/context', () => {
delete process.env.GITHUB_REPOSITORY
context.payload = {
number: 2,
repository: {owner: {login: 'user'}, name: 'test'}
}
repository: {owner: {login: 'user'}, name: 'test'} as PayloadRepository
} as WebhookPayload
expect(context.issue).toEqual({
number: 2,
owner: 'user',
Expand Down
23 changes: 23 additions & 0 deletions packages/github/package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 2 additions & 1 deletion packages/github/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,8 @@
},
"dependencies": {
"@octokit/graphql": "^4.3.1",
"@octokit/rest": "^16.15.0"
"@octokit/rest": "^16.15.0",
"@octokit/webhooks": "^7.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

@thboop thboop Jan 13, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

},
"devDependencies": {
"jest": "^24.7.1"
Expand Down
16 changes: 8 additions & 8 deletions packages/github/src/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ export class Context {
/**
* Webhook payload object that triggered the workflow
*/
payload: WebhookPayload
payload?: WebhookPayload

eventName: string
sha: string
Expand All @@ -20,7 +20,6 @@ export class Context {
* Hydrate the context from the environment
*/
constructor() {
this.payload = {}
if (process.env.GITHUB_EVENT_PATH) {
if (existsSync(process.env.GITHUB_EVENT_PATH)) {
this.payload = JSON.parse(
Expand All @@ -40,11 +39,12 @@ export class Context {
}

get issue(): {owner: string; repo: string; number: number} {
const payload = this.payload

// TODO: https://github.com/actions/toolkit/issues/291 to remove no-unnecessary-type-assertion
// eslint-disable-next-line @typescript-eslint/no-explicit-any, @typescript-eslint/no-unnecessary-type-assertion
const payload = this.payload as any
return {
...this.repo,
number: (payload.issue || payload.pullRequest || payload).number
number: (payload.issue || payload.pull_request || payload).number
}
}

Expand All @@ -54,10 +54,10 @@ export class Context {
return {owner, repo}
}

if (this.payload.repository) {
if (this.payload?.repository) {
return {
owner: this.payload.repository.owner.login,
repo: this.payload.repository.name
owner: this.payload?.repository.owner.login,
repo: this.payload?.repository.name
}
}

Expand Down
64 changes: 25 additions & 39 deletions packages/github/src/interfaces.ts
Original file line number Diff line number Diff line change
@@ -1,39 +1,25 @@
/* eslint-disable @typescript-eslint/no-explicit-any */

export interface PayloadRepository {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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

[key: string]: any
full_name?: string
name: string
owner: {
[key: string]: any
login: string
name?: string
}
html_url?: string
}

export interface WebhookPayload {
[key: string]: any
repository?: PayloadRepository
issue?: {
[key: string]: any
number: number
html_url?: string
body?: string
}
pull_request?: {
[key: string]: any
number: number
html_url?: string
body?: string
}
sender?: {
[key: string]: any
type: string
}
action?: string
installation?: {
id: number
[key: string]: any
}
}
import Webhooks from '@octokit/webhooks'
export type WebhookPayload =
| Webhooks.WebhookPayloadPush
| Webhooks.WebhookPayloadPullRequest
| Webhooks.WebhookPayloadPullRequestReview
| Webhooks.WebhookPayloadPullRequestReviewComment
| Webhooks.WebhookPayloadStatus
| Webhooks.WebhookPayloadIssues
| Webhooks.WebhookPayloadIssueComment
| Webhooks.WebhookPayloadRelease
| Webhooks.WebhookPayloadRepositoryDispatch
| Webhooks.WebhookPayloadCheckRun
| Webhooks.WebhookPayloadDeployment
| Webhooks.WebhookPayloadCheckSuite
| Webhooks.WebhookPayloadWatch
| Webhooks.WebhookPayloadDeploymentStatus
| Webhooks.WebhookPayloadCreate
| Webhooks.WebhookPayloadDelete
| Webhooks.WebhookPayloadProjectCard
| Webhooks.WebhookPayloadPageBuild
| Webhooks.WebhookPayloadFork
| Webhooks.WebhookPayloadGollum
| Webhooks.WebhookPayloadMilestone
| Webhooks.WebhookPayloadProject
| Webhooks.WebhookPayloadLabel