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 #1758 Correct type definitions for OptionGroups and *Options types #1790

Merged
merged 5 commits into from
Apr 3, 2023

Conversation

zimeg
Copy link
Member

@zimeg zimeg commented Mar 31, 2023

Summary

This PR corrects the type definitions for the OptionGroups type by enforcing a PlainTextElement type for the label of OptionGroups and Option[] for options of MessageOptions. Fixes #1758 by removing compilation errors when using these types.

Reviewers

With the changes in this PR, verify that the code sample in the linked issue compiles without error.

To inspect this change in Slack, create the following command (and add it to a testing app) to test the noted sample. Verify text appears after typing "plain_text" in the selector.

Example app.options typing

To properly compile the related example, ack must be typed with a block_suggestion or interactive_message middleware type like so:

- app.options('someCallbackId', async ({ ack }) => {
+ app.options('someCallbackId', async ({ ack }: SlackOptionsMiddlewareArgs<'block_suggestion'>) => {
Example command code
app.command('/ticket', async ({ ack, body, client }: SlackCommandMiddlewareArgs  & { client: WebClient }) => {
  await ack();

  try {
    const result = await client.views.open({
      trigger_id: body.trigger_id,
      view: {
        type: 'modal',
        callback_id: 'someCallbackId',
        title: {
          type: 'plain_text',
          text: 'Selections'
        },
        blocks: [
          {
            type: "section",
            block_id: "selector",
            text: {
              type: "mrkdwn",
              text: "Do you like plain text?"
            },
            accessory: {
              action_id: "option_callback",
              type: "external_select",
              placeholder: {
                type: "plain_text",
                text: "plain_text?"
              }
            }
          },
        ],
      }
    });
    console.log("yay!");
  }
  catch (error) {
    console.error("oops:", error);
  }
});

Notes

  • One test no longer expects an error for interactive_message! There is another app.options test still expecting an error when using a { type: 'interactive_message' } parameter, and I am unsure if this is correct behavior. The FIXME comment above suggests that the type should be OptionsRequest<'interactive_message'>, however this is a deprecated type.
  • The generic Option is used for *Options instead of a PlainTextOption or MrkdwnOption since the type of Option might differ by OptionGroups – see the differences in the text section for the Options object.
  • Filtering of options in these groupings seems to not work... I am curious if there is already an issue to track this? 🤔

Requirements

Sorry, something went wrong.

@zimeg zimeg added bug M-T: confirmed bug report. Issues are confirmed when the reproduction steps are documented semver:patch labels Mar 31, 2023
@zimeg zimeg added this to the 3.12.3 milestone Mar 31, 2023
@zimeg zimeg requested a review from seratch March 31, 2023 23:54
@zimeg zimeg self-assigned this Mar 31, 2023
@codecov
Copy link

codecov bot commented Mar 31, 2023

Codecov Report

Merging #1790 (0cb3fc1) into main (4cfb6f7) will not change coverage.
The diff coverage is n/a.

❗ Current head 0cb3fc1 differs from pull request most recent head 062f0c7. Consider uploading reports for the commit 062f0c7 to get more accurate results

@@           Coverage Diff           @@
##             main    #1790   +/-   ##
=======================================
  Coverage   82.27%   82.27%           
=======================================
  Files          18       18           
  Lines        1523     1523           
  Branches      441      441           
=======================================
  Hits         1253     1253           
  Misses        172      172           
  Partials       98       98           

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

@zimeg
Copy link
Member Author

zimeg commented Mar 31, 2023

Note: This seems related to the changes in #878.

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.

@E-Zim I know this is confusing but the legacy dialog's options/option groups are already correct so we should not change them. We need to improve the types only for external select for Block Kit.

src/types/options/index.ts Show resolved Hide resolved
label: string;
value: string;
}[];
options: Option[];
}
export interface OptionGroups<Options> {
Copy link
Member

Choose a reason for hiding this comment

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

If the option group here is used for both legacy dialog and block kit based external data select, perhaps we need to have the two (meaning DialogOptionGroups and OptionGroup) instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I added the DialogOptionGroups<Options> interface to accommodate these different types, with differences being in the label type. I opted to follow a similar pattern as OptionGroups<Options> (by including the generic), but if you'd think explicitly using the DialogOptions for options would be a better pattern (or something else), I'm happy to change this! For example:

export interface DialogOptionGroups {
  option_groups: ({
    label: string;
    options: DialogOptions[];
  })[];
}

zimeg added 2 commits April 3, 2023 12:35

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@zimeg
Copy link
Member Author

zimeg commented Apr 3, 2023

@seratch Thank you for catching that difference and reviewing these changes! I have updated the types to match the different option types, though I am curious if you have any different thoughts about how these changes should be made (if so, happy to make the changes).

I also noticed (and noted in the description) an oddity between two tests for the interactive_message type. Wanted to raise this in case this isn't behaving as expected!

@zimeg zimeg requested a review from seratch April 3, 2023 20:08
@@ -50,7 +50,7 @@ expectType<void>(
app.options<'interactive_message'>({ callback_id: 'a' }, async ({ options, ack }) => {
expectType<InteractiveMessageSuggestion>(options);
// https://github.com/slackapi/bolt-js/issues/720
expectError(ack({ options: blockSuggestionOptions }));
ack({ options: blockSuggestionOptions });
Copy link
Member

Choose a reason for hiding this comment

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

This test change should be valid as the app.options pattern here now works properly

types-tests/options.test-d.ts Outdated Show resolved Hide resolved

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@seratch
Copy link
Member

seratch commented Apr 3, 2023

@E-Zim Thanks for working on this! This PR is ready to merge now 🎉

@seratch seratch merged commit 4934d52 into main Apr 3, 2023
@seratch seratch deleted the 1758-OptionGroups-typing branch April 3, 2023 22:48
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 semver:patch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OptionGroups types definition may be incorrect
2 participants