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

Fix #1512 Remove say from SlackShortcutMiddlewareArgs for GlobalSho… #1849

Merged
merged 3 commits into from
May 24, 2023

Conversation

mlauter
Copy link
Contributor

@mlauter mlauter commented May 22, 2023

Summary

This pull request resolves #1512 by removing say from the SlackShortcutMiddlewareArgs type for GlobalShortcuts. Global shortcuts are not associated with a conversation, so global shortcut listeners are not passed a say function.

Details

Global shortcuts are not associated with a conversation (the payloads do not have a channel id), so no say function is set in listenerArgs for them.

Message shortcuts are associated with a conversation, so listeners do receive the say function.

This change uses a conditional type to set the type of say to SayFn for a message shortcut's SlackShortcutMiddlewareArgs but undefined otherwise.

Testing

  • Automated tests are passing.
  • I've added two new type tests for this change. I'm a new contributor though, and I was unsure looking at the test suite whether there are other tests you'd like to see added.
  • I set up a dummy slack app and linked my working copy of the bolt-js repo to confirm that the types work as expected:
// No type errors
app.shortcut<MessageShortcut>(
  'shortcut_echo',
  async ({ ack, payload, say }) => {
    ack();
    say(payload.message);
    console.log(payload);
  },
);

// Type error
app.shortcut<GlobalShortcut>('wave', async ({ ack, payload, say }) => {
  ack();
  say('hi'); // Cannot invoke an object which is possibly 'undefined'.ts(2722) (parameter) say: undefined
  console.log(payload);
});

// Type error -- caller must verify that say exists
app.shortcut('wave', async ({ ack, payload, say }) => {
  ack();
  say('hi'); // Cannot invoke an object which is possibly 'undefined'.ts(2722) (parameter) say: SayFn | undefined
  console.log(payload);
});

Requirements (place an x in each [ ])

@salesforce-cla
Copy link

Thanks for the contribution! Before we can merge this, we need @mlauter to sign the Salesforce Inc. Contributor License Agreement.

@seratch seratch added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented TypeScript-specific labels May 23, 2023
@seratch seratch added this to the 3.13.2 milestone May 23, 2023
@seratch seratch changed the title Fixes 1512: Remove say from SlackShortcutMiddlewareArgs for GlobalSho… Fix #1512 Remove say from SlackShortcutMiddlewareArgs for GlobalSho… May 23, 2023
@seratch seratch self-assigned this May 23, 2023
@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Merging #1849 (a30d034) into main (a3532c3) will not change coverage.
The diff coverage is n/a.

❗ Current head a30d034 differs from pull request most recent head 0144e24. Consider uploading reports for the commit 0144e24 to get more accurate results

@@           Coverage Diff           @@
##             main    #1849   +/-   ##
=======================================
  Coverage   82.15%   82.15%           
=======================================
  Files          18       18           
  Lines        1519     1519           
  Branches      435      435           
=======================================
  Hits         1248     1248           
  Misses        175      175           
  Partials       96       96           

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@seratch seratch left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM 👍

@seratch
Copy link
Member

seratch commented May 23, 2023

Thanks a lot for your contribution! If there is no objection from other maintainers, we can merge this PR tomorrow in Pacific timezone.

@mlauter
Copy link
Contributor Author

mlauter commented May 24, 2023

@seratch let me know if there's anything else you need from me

@seratch
Copy link
Member

seratch commented May 24, 2023

@mlauter Thanks for the reminder. I was on PTO yesterday. Now we can merge this PR 👍

@seratch seratch merged commit 53bb992 into slackapi:main May 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented cla:signed TypeScript-specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incorrect typing for global shortcut parameters
2 participants