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

seratch · 2021-08-30T03:05:24Z

Summary

This pull request fixes #1098 by updating the type of next property in AllMiddlewareArgs. As we already have so many changes in v3.7, we can have this one in v3.8 or later.

Requirements (place an `x` in each `[ ]`)

I've read and understood the Contributing Guidelines and have done my best effort to follow them.
I've read and agree to the Code of Conduct.

codecov · 2021-08-30T03:07:12Z

Codecov Report

Merging #1099 (a2b8b39) into main (a24e3b8) will not change coverage.
The diff coverage is 100.00%.

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

@@           Coverage Diff           @@
##             main    #1099   +/-   ##
=======================================
  Coverage   71.71%   71.71%           
=======================================
  Files          15       15           
  Lines        1354     1354           
  Branches      402      402           
=======================================
  Hits          971      971           
  Misses        312      312           
  Partials       71       71

Impacted Files	Coverage Δ
src/App.ts	`83.64% <ø> (ø)`
src/WorkflowStep.ts	`90.69% <100.00%> (ø)`
src/conversation-store.ts	`100.00% <100.00%> (ø)`
src/middleware/builtin.ts	`83.84% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update a24e3b8...e8933b7. Read the comment docs.

seratch · 2021-08-30T03:09:18Z

src/WorkflowStep.spec.ts

@@ -291,12 +291,12 @@ describe('WorkflowStep', () => {

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


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.

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.

seratch · 2021-08-30T03:10:18Z

src/App.ts

@@ -886,7 +887,8 @@ export default class App {
                context,
                client,
                logger: this.logger,
-              }),
+                // `next` is already set in the outer processMiddleware


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.

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

@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.

Updated the comment

filmaj

Awesome, nice improvement!

filmaj · 2021-08-30T15:22:33Z

src/WorkflowStep.spec.ts

@@ -291,12 +291,12 @@ describe('WorkflowStep', () => {

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

      const fn1 = sinon.spy((async ({ next: continuation }) => {
        // eslint-disable-next-line @typescript-eslint/no-non-null-assertion


I think we can now safely remove this line as well.

Thanks! Fixed

srajiang

Just had one question - this is my first time reviewing the middleware handling, and I want to make sure I understand what's happening when the listener middleware chain last is evoked.

seratch · 2021-08-31T21:51:34Z

src/App.ts

@@ -886,7 +887,8 @@ export default class App {
                context,
                client,
                logger: this.logger,
-              }),
+                // `next` is already set in the outer processMiddleware


@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.

src/WorkflowStep.spec.ts

seratch added the TypeScript-specific label Aug 30, 2021

seratch added this to the 3.8.0 milestone Aug 30, 2021

seratch requested review from mwbrooks, filmaj, misscoded and srajiang August 30, 2021 03:05

seratch commented Aug 30, 2021

View reviewed changes

filmaj approved these changes Aug 30, 2021

View reviewed changes

mwbrooks approved these changes Aug 30, 2021

View reviewed changes

srajiang reviewed Aug 30, 2021

View reviewed changes

seratch commented Aug 31, 2021

View reviewed changes

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

e8933b7

seratch force-pushed the issue-1098-optional-next branch from a2b8b39 to e8933b7 Compare October 6, 2021 11:08

seratch self-assigned this Oct 6, 2021

seratch merged commit a86909a into slackapi:main Oct 6, 2021

seratch deleted the issue-1098-optional-next branch October 6, 2021 11:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

seratch commented Aug 30, 2021

codecov bot commented Aug 30, 2021 •

edited

Loading

seratch Aug 30, 2021

misscoded Aug 30, 2021

seratch Aug 30, 2021

srajiang Aug 30, 2021

seratch Aug 31, 2021

seratch Oct 6, 2021

filmaj left a comment

filmaj Aug 30, 2021

seratch Oct 6, 2021

srajiang left a comment

seratch Aug 31, 2021

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

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

Conversation

seratch commented Aug 30, 2021

Summary

Requirements (place an x in each [ ])

codecov bot commented Aug 30, 2021 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

filmaj left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

srajiang left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Requirements (place an `x` in each `[ ]`)

codecov bot commented Aug 30, 2021 •

edited

Loading