fix: options constraint has wrong type definition (#1940)

Co-authored-by: Kazuhiro Sera <[email protected]>
slackapi · Oct 2, 2023 · 44c5e01 · 44c5e01
1 parent 3684846
commit 44c5e01
Show file tree

Hide file tree

Showing 4 changed files with 35 additions and 11 deletions.
diff --git a/src/App-context-types.spec.ts b/src/App-context-types.spec.ts
@@ -703,7 +703,7 @@ describe('context typing', () => {
     });
 
     // Options with constraint is aware of global context and passes context to all middleware
-    app.options({ type: 'block_actions' }, async ({ context }) => {
+    app.options({ type: 'block_suggestion' }, async ({ context }) => {
       const globalCheck = {} as IfAnyThenElse<typeof context['globalContextKey'], never, valid>;
       globalCheck.valid = true;
     }, async ({ context }) => {
@@ -712,7 +712,7 @@ describe('context typing', () => {
     });
 
     // Options with constraint is aware of global and middleware context and passes context to all middleware
-    app.options<OptionsSource, MiddlewareContext>({ type: 'block_actions' }, async ({ context }) => {
+    app.options<OptionsSource, MiddlewareContext>({ type: 'block_suggestion' }, async ({ context }) => {
       const globalCheck = {} as IfAnyThenElse<typeof context['globalContextKey'], never, valid>;
       globalCheck.valid = true;
 
@@ -750,7 +750,7 @@ describe('context typing', () => {
     });
 
     // Options with constraint is aware of middleware context and passes context to all middleware
-    app.options<OptionsSource, MiddlewareContext>({ type: 'block_actions' }, async ({ context }) => {
+    app.options<OptionsSource, MiddlewareContext>({ type: 'block_suggestion' }, async ({ context }) => {
       const middlewareCheck = {} as IfAnyThenElse<typeof context['middlewareContextKey'], never, valid>;
       middlewareCheck.valid = true;
     }, async ({ context }) => {

diff --git a/src/App-routes.spec.ts b/src/App-routes.spec.ts
@@ -557,9 +557,21 @@ describe('App event routing', () => {
       app.options('external_select_action_id', async () => {
         await optionsFn();
       });
+      app.options({
+        type: 'block_suggestion',
+        action_id: 'external_select_action_id',
+      }, async () => {
+        await optionsFn();
+      });
       app.options({ callback_id: 'dialog_suggestion_callback_id' }, async () => {
         await optionsFn();
       });
+      app.options({
+        type: 'dialog_suggestion',
+        callback_id: 'dialog_suggestion_callback_id',
+      }, async () => {
+        await optionsFn();
+      });
 
       app.event('app_home_opened', noop);
       app.event(/app_home_opened|app_mention/, noop);
@@ -593,7 +605,7 @@ describe('App event routing', () => {
       assert.equal(actionFn.callCount, 3);
       assert.equal(shortcutFn.callCount, 4);
       assert.equal(viewFn.callCount, 5);
-      assert.equal(optionsFn.callCount, 2);
+      assert.equal(optionsFn.callCount, 4);
       assert.equal(ackFn.callCount, dummyReceiverEvents.length);
       assert(fakeErrorHandler.notCalled);
     });

diff --git a/src/App.ts b/src/App.ts
@@ -53,6 +53,7 @@ import {
   KnownEventFromType,
   SlashCommand,
   WorkflowStepEdit,
+  SlackOptions,
 } from './types';
 import { IncomingEventType, getTypeAndConversation, assertNever, isBodyWithTypeEnterpriseInstall, isEventTypeToSkipAuthorize } from './helpers';
 import { CodedError, asCodedError, AppInitializationError, MultipleListenerError, ErrorCode, InvalidCustomPropertyError } from './errors';
@@ -148,6 +149,15 @@ export interface ActionConstraints<A extends SlackAction = SlackAction> {
   callback_id?: Extract<A, { callback_id?: string }> extends any ? string | RegExp : never;
 }
 
+// TODO: more strict typing to allow block/action_id for block_suggestion etc.
+export interface OptionsConstraints<A extends SlackOptions = SlackOptions> {
+  type?: A["type"];
+  block_id?: A extends SlackOptions ? string | RegExp : never;
+  action_id?: A extends SlackOptions ? string | RegExp : never; 
+  // eslint-disable-next-line @typescript-eslint/no-explicit-any
+  callback_id?: Extract<A, { callback_id?: string }> extends any ? string | RegExp : never;
+}
+
 export interface ShortcutConstraints<S extends SlackShortcut = SlackShortcut> {
   type?: S['type'];
   callback_id?: string | RegExp;
@@ -772,20 +782,22 @@ export default class App<AppCustomContext extends StringIndexed = StringIndexed>
     Source extends OptionsSource = OptionsSource,
     MiddlewareCustomContext extends StringIndexed = StringIndexed,
   >(
-    constraints: ActionConstraints,
+    constraints: OptionsConstraints,
     ...listeners: Middleware<SlackOptionsMiddlewareArgs<Source>, AppCustomContext & MiddlewareCustomContext>[]
   ): void;
   // TODO: reflect the type in constraints to Source
   public options<
     Source extends OptionsSource = OptionsSource,
     MiddlewareCustomContext extends StringIndexed = StringIndexed,
   >(
-    actionIdOrConstraints: string | RegExp | ActionConstraints,
+    actionIdOrConstraints: string | RegExp | OptionsConstraints,
     ...listeners: Middleware<SlackOptionsMiddlewareArgs<Source>, AppCustomContext & MiddlewareCustomContext>[]
   ): void {
-    const constraints: ActionConstraints = typeof actionIdOrConstraints === 'string' || util.types.isRegExp(actionIdOrConstraints) ?
-      { action_id: actionIdOrConstraints } :
-      actionIdOrConstraints;
+    const constraints: OptionsConstraints =
+      typeof actionIdOrConstraints === "string" ||
+      util.types.isRegExp(actionIdOrConstraints)
+        ? { action_id: actionIdOrConstraints }
+        : actionIdOrConstraints;
 
     // eslint-disable-next-line @typescript-eslint/no-explicit-any
     const _listeners = listeners as any; // FIXME: workaround for TypeScript 4.7 breaking changes

diff --git a/src/middleware/builtin.ts b/src/middleware/builtin.ts
@@ -26,7 +26,7 @@ import {
   EventTypePattern,
   ViewOutput,
 } from '../types';
-import { ActionConstraints, ViewConstraints, ShortcutConstraints } from '../App';
+import { ActionConstraints, ViewConstraints, ShortcutConstraints, OptionsConstraints } from '../App';
 import { ContextMissingPropertyError } from '../errors';
 
 /**
@@ -122,7 +122,7 @@ export const onlyViewActions: Middleware<AnyMiddlewareArgs & { view?: ViewOutput
  * Middleware that checks for matches given constraints
  */
 export function matchConstraints(
-  constraints: ActionConstraints | ViewConstraints | ShortcutConstraints,
+  constraints: ActionConstraints | ViewConstraints | ShortcutConstraints | OptionsConstraints,
 ): Middleware<SlackActionMiddlewareArgs | SlackOptionsMiddlewareArgs | SlackViewMiddlewareArgs> {
   return async ({ payload, body, next, context }) => {
     // TODO: is putting matches in an array actually helpful? there's no way to know which of the regexps contributed