From 44c5e01d57214b5c990418a7e2aa68911a2a0211 Mon Sep 17 00:00:00 2001 From: Nemanja Stanic <105949556+nemanjastanic@users.noreply.github.com> Date: Mon, 2 Oct 2023 02:22:43 +0200 Subject: [PATCH] fix: options constraint has wrong type definition (#1940) Co-authored-by: Kazuhiro Sera --- src/App-context-types.spec.ts | 6 +++--- src/App-routes.spec.ts | 14 +++++++++++++- src/App.ts | 22 +++++++++++++++++----- src/middleware/builtin.ts | 4 ++-- 4 files changed, 35 insertions(+), 11 deletions(-) diff --git a/src/App-context-types.spec.ts b/src/App-context-types.spec.ts index 72d63f8f3..1d75956d6 100644 --- 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; 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({ type: 'block_actions' }, async ({ context }) => { + app.options({ type: 'block_suggestion' }, async ({ context }) => { const globalCheck = {} as IfAnyThenElse; 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({ type: 'block_actions' }, async ({ context }) => { + app.options({ type: 'block_suggestion' }, async ({ context }) => { const middlewareCheck = {} as IfAnyThenElse; middlewareCheck.valid = true; }, async ({ context }) => { diff --git a/src/App-routes.spec.ts b/src/App-routes.spec.ts index 75c6b04aa..e1e22f04b 100644 --- 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 index 697630e95..2644388d3 100644 --- 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 { callback_id?: Extract extends any ? string | RegExp : never; } +// TODO: more strict typing to allow block/action_id for block_suggestion etc. +export interface OptionsConstraints { + 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 extends any ? string | RegExp : never; +} + export interface ShortcutConstraints { type?: S['type']; callback_id?: string | RegExp; @@ -772,7 +782,7 @@ export default class App Source extends OptionsSource = OptionsSource, MiddlewareCustomContext extends StringIndexed = StringIndexed, >( - constraints: ActionConstraints, + constraints: OptionsConstraints, ...listeners: Middleware, AppCustomContext & MiddlewareCustomContext>[] ): void; // TODO: reflect the type in constraints to Source @@ -780,12 +790,14 @@ export default class App Source extends OptionsSource = OptionsSource, MiddlewareCustomContext extends StringIndexed = StringIndexed, >( - actionIdOrConstraints: string | RegExp | ActionConstraints, + actionIdOrConstraints: string | RegExp | OptionsConstraints, ...listeners: Middleware, 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 index aa43e2445..64dfe9714 100644 --- 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 { 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