Skip to content

Conversation

@aidant
Copy link
Contributor

@aidant aidant commented Sep 30, 2019

The current implementation of EventHandler#removeListener never iterates to index 0 in the state.


Example code to reproduce this issue:

const Octokit = require('@octokit/rest')
const Webhook = require('@octokit/webhooks')
const http = require('http')

const GITHUB_ORGANIZATION_NAME = ''
const GITHUB_ORGANIZATION_WEBHOOK_ID = 0
const GITHUB_ORGANIZATION_WEBHOOK_PATH = '/'
const GITHUB_ORGANIZATION_WEBHOOK_PORT= 8080
const GITHUB_ORGANIZATION_WEBHOOK_SECRET = ''
const GITHUB_PERSONAL_ACCESS_TOKEN = ''

const octokit = new Octokit({
  auth: GITHUB_PERSONAL_ACCESS_TOKEN
})

const webhook = new Webhook({
  path: GITHUB_ORGANIZATION_WEBHOOK_PATH,
  secret: GITHUB_ORGANIZATION_WEBHOOK_SECRET
})

const entrypoint = async () => {
  const listener = (event) => {
    webhook.removeListener('*', listener)
    console.log(`Received event: "${event.name}".`)
  }

  webhook.on('*', listener)

  await octokit.orgs.pingHook({
    org: GITHUB_ORGANIZATION_NAME,
    hook_id: GITHUB_ORGANIZATION_WEBHOOK_ID,
  })

  await octokit.orgs.pingHook({
    org: GITHUB_ORGANIZATION_NAME,
    hook_id: GITHUB_ORGANIZATION_WEBHOOK_ID,
  })
}

http.createServer(webhook.middleware)
  .listen(GITHUB_ORGANIZATION_WEBHOOK_PORT, () => entrypoint().catch(console.error))

Without the fix:

Received event: "ping".
Received event: "ping".

With the fix:

Received event: "ping".

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.

Wonderful pull request, thank you so much! I just have one little nitpick question

// remove last hook that has been added, that way
// it behaves the same as removeListener
for (let i = state.hooks[webhookNameOrNames].length; i > 0; i--) {
for (let i = state.hooks[webhookNameOrNames].length - 1; i >= 0; i--) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Wow, good catch, thanks so much for investigating and sending a pull request, with tests 😻

@@ -0,0 +1,42 @@
const test = require('tap').test

const receiverListener = require('../../event-handler/remove-listener')
Copy link
Contributor

Choose a reason for hiding this comment

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

have you meant to call this variable removeListener? I think that would make the tests a little more clear!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I agree it does look cleaner to call it removeListener. I've updated the test with the new name.

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.

Perfect, thank you!

@gr2m gr2m merged commit 02a41a6 into octokit:master Oct 2, 2019
@octokitbot
Copy link
Contributor

🎉 This PR is included in version 6.3.1 🎉

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

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants