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.

Sign up for GitHub

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 #1098 next() is optional in middleware in TypeScript #1099

Merged
merged 1 commit into from
Oct 6, 2021
Merged

Fix #1098 next() is optional in middleware in TypeScript #1099

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 9 additions & 9 deletions src/App.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -483,8 +483,8 @@ describe('App', () => {
// Arrange
app.use(fakeFirstMiddleware);
app.use(async ({ next }) => {
await next!();
await next!();
await next();
await next();
});
app.use(fakeSecondMiddleware);
app.error(fakeErrorHandler);
Expand All @@ -503,7 +503,7 @@ describe('App', () => {
await delay(100);
changed = true;

await next!();
await next();
});

await fakeReceiver.sendEvent(dummyReceiverEvent);
Expand All @@ -517,7 +517,7 @@ describe('App', () => {

app.use(async ({ next }) => {
try {
await next!();
await next();
} catch (err: any) {
caughtError = err;
}
Expand Down Expand Up @@ -1216,7 +1216,7 @@ describe('App', () => {

app.use(async ({ next }) => {
await ackFn();
await next!();
await next();
});
app.shortcut({ callback_id: 'message_action_callback_id' }, async () => {
await shortcutFn();
Expand Down Expand Up @@ -1317,7 +1317,7 @@ describe('App', () => {

app.use(async ({ next }) => {
await ackFn();
await next!();
await next();
});
app.shortcut({ callback_id: 'message_action_callback_id' }, async () => {
await shortcutFn();
Expand Down Expand Up @@ -1635,7 +1635,7 @@ describe('App', () => {
});
app.use(async ({ logger, body, next }) => {
logger.info(body);
await next!();
await next();
});

app.event('app_home_opened', async ({ logger, event }) => {
Expand Down Expand Up @@ -1683,7 +1683,7 @@ describe('App', () => {
});
app.use(async ({ logger, body, next }) => {
logger.info(body);
await next!();
await next();
});

app.event('app_home_opened', async ({ logger, event }) => {
Expand Down Expand Up @@ -1746,7 +1746,7 @@ describe('App', () => {
});
app.use(async ({ client, next }) => {
await client.auth.test();
await next!();
await next();
});
const clients: WebClient[] = [];
app.event('app_home_opened', async ({ client }) => {
Expand Down
7 changes: 5 additions & 2 deletions src/App.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ import {
} from './types';
import { IncomingEventType, getTypeAndConversation, assertNever } from './helpers';
import { CodedError, asCodedError, AppInitializationError, MultipleListenerError, ErrorCode } from './errors';
import { AllMiddlewareArgs } from './types/middleware';
// eslint-disable-next-line import/order
import allSettled = require('promise.allsettled'); // eslint-disable-line @typescript-eslint/no-require-imports
// eslint-disable-next-line @typescript-eslint/no-require-imports, import/no-commonjs
Expand Down Expand Up @@ -891,13 +892,15 @@ export default class App {
context,
client,
this.logger,
// When the listener middleware chain is done processing, call the listener without a next fn
// When all of the listener middleware are done processing,
// `listener` here will be called without a `next` execution
async () => listener({
...(listenerArgs as AnyMiddlewareArgs),
context,
client,
logger: this.logger,
}),
// `next` is already set in the outer processMiddleware
Copy link
Member Author

Choose a reason for hiding this comment

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

The code here is a bit tricky. You can check the internals of processMiddleware. The outer processMiddleware call quietly sets next and middleware execution relies on it.

Copy link
Member

Choose a reason for hiding this comment

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

Are the comments on 884 and 871 technically misleading then, if next is actually set?

Copy link
Member Author

Choose a reason for hiding this comment

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

@srajiang Indeed, last() and L871 may be confusing. In my understanding, the last element of an array of middleware technically can have next() but we don't do anything with it. As you saw, inside the processMiddleware method, we call last() instead of next middleware with recursive invokeMiddleware call.

We may want to update the L871 comment when merging this PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated the comment

} as AnyMiddlewareArgs & AllMiddlewareArgs),
);
});

Expand Down
9 changes: 2 additions & 7 deletions src/WorkflowStep.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,12 +291,11 @@ describe('WorkflowStep', () => {

describe('processStepMiddleware', () => {
it('should call each callback in user-provided middleware', async () => {
const { next: _next, ...fakeArgs } = createFakeStepEditAction() as unknown as AllWorkflowStepMiddlewareArgs;
Copy link
Member Author

Choose a reason for hiding this comment

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

createFakeStepEditAction() (and others similar) adds next inside but we remove it from fakeArgs in this line. AllWorkflowStepMiddlewareArgs minus next property does not compile with processStepMiddleware in this test method. This is why I've removed the next: sinon.fake() in this test. Just in case, we can need to do more tests to verify that we are not breaking anything.

Copy link
Contributor

Choose a reason for hiding this comment

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

I only vaguely remember writing this, but what I think I recall is that the removal of next here was to mimic the switch out that occurs of the default/usual next and the replacement of it with the user-provided callback. It's been a while, however, so not sure if this file has since been changed since that was originally written.

const { ...fakeArgs } = createFakeStepEditAction() as unknown as AllWorkflowStepMiddlewareArgs;
seratch marked this conversation as resolved.
Show resolved Hide resolved
const { processStepMiddleware } = await importWorkflowStep();

const fn1 = sinon.spy((async ({ next: continuation }) => {
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await continuation!();
await continuation();
}) as Middleware<WorkflowStepEdit>);
const fn2 = sinon.spy(async () => {});
const fakeMiddleware = [fn1, fn2] as WorkflowStepMiddleware;
Expand All @@ -323,7 +322,6 @@ function createFakeStepEditAction() {
workflow_step: {},
},
context: {},
next: sinon.fake(),
};
}

Expand All @@ -341,7 +339,6 @@ function createFakeStepSaveEvent() {
callback_id: 'test_save_callback_id',
},
context: {},
next: sinon.fake(),
};
}

Expand All @@ -362,7 +359,6 @@ function createFakeStepExecuteEvent() {
},
},
context: {},
next: sinon.fake(),
};
}

Expand All @@ -380,6 +376,5 @@ function createFakeViewEvent() {
callback_id: 'test_view_callback_id',
},
context: {},
next: sinon.fake(),
};
}
4 changes: 1 addition & 3 deletions src/WorkflowStep.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,9 +154,7 @@ export class WorkflowStep {
if (isStepEvent(args) && this.matchesConstraints(args)) {
return this.processEvent(args);
}
// `next` here exists for sure bu the typing on the MiddlwareArgs is not great enough
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
return args.next!();
return args.next();
};
}

Expand Down
4 changes: 1 addition & 3 deletions src/conversation-store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,8 +74,6 @@ export function conversationContext<ConversationState = any>(
} else {
logger.debug('No conversation ID for incoming event');
}
// TODO: remove the non-null assertion operator
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await next!();
await next();
};
}
52 changes: 13 additions & 39 deletions src/middleware/builtin.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,9 +39,7 @@ export const onlyActions: Middleware<AnyMiddlewareArgs & { action?: SlackAction
}

// It matches so we should continue down this middleware listener chain
// TODO: remove the non-null assertion operator
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await next!();
await next();
};

/**
Expand All @@ -57,9 +55,7 @@ export const onlyShortcuts: Middleware<AnyMiddlewareArgs & { shortcut?: SlackSho
}

// It matches so we should continue down this middleware listener chain
// TODO: remove the non-null assertion operator
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await next!();
await next();
};

/**
Expand All @@ -72,9 +68,7 @@ export const onlyCommands: Middleware<AnyMiddlewareArgs & { command?: SlashComma
}

// It matches so we should continue down this middleware listener chain
// TODO: remove the non-null assertion operator
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await next!();
await next();
};

/**
Expand All @@ -87,9 +81,7 @@ export const onlyOptions: Middleware<AnyMiddlewareArgs & { options?: SlackOption
}

// It matches so we should continue down this middleware listener chain
// TODO: remove the non-null assertion operator
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await next!();
await next();
};

/**
Expand All @@ -102,9 +94,7 @@ export const onlyEvents: Middleware<AnyMiddlewareArgs & { event?: SlackEvent }>
}

// It matches so we should continue down this middleware listener chain
// TODO: remove the non-null assertion operator
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await next!();
await next();
};

/**
Expand All @@ -117,9 +107,7 @@ export const onlyViewActions: Middleware<AnyMiddlewareArgs & { view?: ViewOutput
}

// It matches so we should continue down this middleware listener chain
// TODO: remove the non-null assertion operator
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await next!();
await next();
};

/**
Expand Down Expand Up @@ -206,9 +194,7 @@ export function matchConstraints(
if (body.type !== constraints.type) return;
}

// TODO: remove the non-null assertion operator
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await next!();
await next();
};
}

Expand Down Expand Up @@ -240,9 +226,7 @@ export function matchMessage(
}
}

// TODO: remove the non-null assertion operator
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await next!();
await next();
};
}

Expand All @@ -256,9 +240,7 @@ export function matchCommandName(pattern: string | RegExp): Middleware<SlackComm
return;
}

// TODO: remove the non-null assertion operator
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await next!();
await next();
};
}

Expand Down Expand Up @@ -294,9 +276,7 @@ export function matchEventType(pattern: EventTypePattern): Middleware<SlackEvent
}
}

// TODO: remove the non-null assertion operator
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await next!();
await next();
};
}

Expand Down Expand Up @@ -329,18 +309,14 @@ export function ignoreSelf(): Middleware<AnyMiddlewareArgs> {
}

// If all the previous checks didn't skip this message, then its okay to resume to next
// TODO: remove the non-null assertion operator
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await args.next!();
await args.next();
};
}

export function subtype(subtype1: string): Middleware<SlackEventMiddlewareArgs<'message'>> {
return async ({ message, next }) => {
if (message.subtype === subtype1) {
// TODO: remove the non-null assertion operator
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await next!();
await next();
}
};
}
Expand Down Expand Up @@ -376,9 +352,7 @@ export function directMention(): Middleware<SlackEventMiddlewareArgs<'message'>>
return;
}

// TODO: remove the non-null assertion operator
// eslint-disable-next-line @typescript-eslint/no-non-null-assertion
await next!();
await next();
};
}

Expand Down
3 changes: 1 addition & 2 deletions src/types/middleware.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,7 @@ export interface AllMiddlewareArgs {
context: Context;
logger: Logger;
client: WebClient;
// TODO: figure out how to make next non-optional
next?: NextFn;
next: NextFn;
}

// NOTE: Args should extend AnyMiddlewareArgs, but because of contravariance for function types, including that as a
Expand Down