Allow passing additional types for Global and Middleware Context #1505

M1kep · 2022-06-26T04:55:19Z

Summary

This pr picks up from #1157 and allows users to pass custom types to the App initialization(Global Context) as well as individual middleware(Localized Context). This allows IDEs to provide more specific suggestions as well as better type checking.

I've included an example of the direction I'm thinking would work for testing this. Testing is a little weird because this is purely a type system addition. As it's just the type system, the only forms of tests I can think of will cause compilation errors rather than test failures. Additionally, as the Context type is still StringIndexed an IsAny check was used which was found via StackOverflow. If there are any suggestions for better way to test this, I'd be happy to give it a try, otherwise I can move forward with the test cases using the my current method.

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 · 2022-06-26T04:56:57Z

Codecov Report

Merging #1505 (95c8afa) into main (7eec438) will increase coverage by 0.06%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1505      +/-   ##
==========================================
+ Coverage   82.00%   82.07%   +0.06%     
==========================================
  Files          18       18              
  Lines        1495     1495              
  Branches      435      435              
==========================================
+ Hits         1226     1227       +1     
  Misses        172      172              
+ Partials       97       96       -1

Impacted Files	Coverage Δ
src/types/middleware.ts	`100.00% <ø> (ø)`
src/App.ts	`84.11% <100.00%> (+0.23%)`	⬆️

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 7eec438...95c8afa. Read the comment docs.

seratch

Looks good to me. Adding this should not bring any breaking changes. The only downside of the changes would be the types inside this framework can be even more complex but it is not due to this PR.

seratch · 2022-06-27T05:30:26Z

src/App-context-types.spec.ts

+
+    app.message('hello', async ({ context }) => {
+      // If the globalContextKey in the context is not explicitly typed, then it will be 'any'
+      // The IfAny check will then set 'check' to 'never', causing the check.valid call later to fail


This is a bit tricky but as long as the check.valid = true; statement fails in the compilation phase, it'd be fine.

filmaj

Left a question regarding the type of the private members (mainly for my own education!)

What about documentation? Wondering how we can communicate this to developers via our existing docs site? We have a small section, available by a 'Using Typescript' link in the bottom left of the site. I think we should add more information on this feature in there. I know that 'guide' is very light, but every small improvement helps.

Maybe it is also worth showing off how to use this feature in the TypeScript example in the repo?

filmaj · 2022-06-30T19:03:43Z

src/App.ts

+  public use<MiddlewareCustomContext extends StringIndexed = StringIndexed>(
+    m: Middleware<AnyMiddlewareArgs, AppCustomContext & MiddlewareCustomContext>,
+  ): this {
+    this.middleware.push(m as Middleware<AnyMiddlewareArgs>);


Should the private middleware property also be typed as Middleware<AnyMiddlewareArgs, AppCustomContext & MiddlewareCustomContext>[]?

Currently if it's typed with the Contexts the below compiler error is generated:

TS2345: Argument of type 'Middleware<AnyMiddlewareArgs, AppCustomContext & MiddlewareCustomContext>' is not assignable to parameter of type 'Middleware<AnyMiddlewareArgs, StringIndexed>'. Type 'StringIndexed' is not assignable to type 'AppCustomContext & MiddlewareCustomContext'. Type 'StringIndexed' is not assignable to type 'AppCustomContext'. 'StringIndexed' is assignable to the constraint of type 'AppCustomContext', but 'AppCustomContext' could be instantiated with a different subtype of constraint 'StringIndexed'.

In addition, I'm not sure the typing is as important on this line, as once the middlewares are in the private global middlewares, I don't think Bolt cares about the custom context type anymore? From my limited testing, if
(Line: 228)

/** Global middleware chain */ private middleware: Middleware<AnyMiddlewareArgs>[];

is changed to

/** Global middleware chain */ private middleware: Middleware<AnyMiddlewareArgs, AppCustomContext>[];

Then m as Middleware<AnyMiddlewareArgs, AppCustomContext> will work. But doing this, causes the processMiddleware function(https://github.com/slackapi/bolt-js/blob/main/src/middleware/process.ts) to want to be aware of the app context as well. Circling back to the question of whether Bolt cares about the context internally, I don't really see a scenario where the context will matter at that point as the consumer of the library only interacts with the middlewares via the use, shortcut, message, etc middleware registration points.

Thank you for the explanation! I appreciate it; my TS isn't great so this is helpful 🙇

M1kep · 2022-07-04T00:31:51Z

src/App.ts

+  public message<
+    MiddlewareCustomContext extends StringIndexed = StringIndexed,
+  >(
+    filter: MessageEventMiddleware,
+    ...listeners: MessageEventMiddleware<AppCustomContext & MiddlewareCustomContext>[]
+  ): void;


While building the tests I wasn't able to get TypeScript to use this overload. As filter and ...listeners share the same type I'm not sure this overload is necessary. If there's agreement, I feel like the first overloads docs could be updated to go into more detail about how a middleware can be used as a filter.

bolt-js/src/App.ts

Lines 567 to 571 in 7eec438

/**

*

* @param listeners Middlewares that process and react to a message event

*/

public message(...listeners: MessageEventMiddleware[]): void;

Good catch. As long as it still works without the overloaded method, I think that we can safely remove the unnecessary method with filter: MessageEventMiddleware argument.

seratch · 2022-07-04T04:43:32Z

Everything in this PR already looks great to @filmaj and me. We'll merge this PR within a few business days. If anyone has comments, please don't hesitate to write in before merging!

seratch · 2022-07-12T23:44:28Z

No responses from others. Let me merge this one now. @M1kep Thanks for the great improvement!

M1kep added 2 commits June 25, 2022 21:28

Allow passing additional types for Global and Individual Middleware

2011c38

Add initial attempt at testing context types

dc8b0c8

M1kep changed the title ~~Allow passing additional types for Global and Individual Middleware~~ Allow passing additional types for Global and Middleware Context Jun 26, 2022

seratch added enhancement M-T: A feature request for new functionality TypeScript-specific labels Jun 27, 2022

seratch added this to the 3.12.0 milestone Jun 27, 2022

seratch requested a review from filmaj June 27, 2022 05:37

seratch approved these changes Jun 27, 2022

View reviewed changes

filmaj reviewed Jun 30, 2022

View reviewed changes

filmaj requested review from misscoded, srajiang and stevengill June 30, 2022 19:08

M1kep added 2 commits July 3, 2022 17:26

Add tests

0aae114

Add App and Middleware context to Message Filter middleware

5b5b3c6

M1kep commented Jul 4, 2022

View reviewed changes

Refactor IfAny check to make it more intuitive

95c8afa

seratch merged commit 2e6e6c0 into slackapi:main Jul 12, 2022

seratch mentioned this pull request Jul 14, 2022

Feature: Type safe custom context properties in TypeScript #1157

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow passing additional types for Global and Middleware Context #1505

Allow passing additional types for Global and Middleware Context #1505

M1kep commented Jun 26, 2022 •

edited

Loading

codecov bot commented Jun 26, 2022 •

edited

Loading

seratch left a comment

seratch Jun 27, 2022

filmaj left a comment

filmaj Jun 30, 2022

M1kep Jun 30, 2022

filmaj Jun 30, 2022

M1kep Jul 4, 2022 •

edited

Loading

seratch Jul 4, 2022

seratch commented Jul 4, 2022

seratch commented Jul 12, 2022

	/**
	*
	* @param listeners Middlewares that process and react to a message event
	*/
	public message(...listeners: MessageEventMiddleware[]): void;

Allow passing additional types for Global and Middleware Context #1505

Allow passing additional types for Global and Middleware Context #1505

Conversation

M1kep commented Jun 26, 2022 • edited Loading

Summary

Requirements (place an x in each [ ])

codecov bot commented Jun 26, 2022 • edited Loading

Codecov Report

seratch left a comment

Choose a reason for hiding this comment

seratch Jun 27, 2022

Choose a reason for hiding this comment

filmaj left a comment

Choose a reason for hiding this comment

filmaj Jun 30, 2022

Choose a reason for hiding this comment

M1kep Jun 30, 2022

Choose a reason for hiding this comment

filmaj Jun 30, 2022

Choose a reason for hiding this comment

M1kep Jul 4, 2022 • edited Loading

Choose a reason for hiding this comment

seratch Jul 4, 2022

Choose a reason for hiding this comment

seratch commented Jul 4, 2022

seratch commented Jul 12, 2022

M1kep commented Jun 26, 2022 •

edited

Loading

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

codecov bot commented Jun 26, 2022 •

edited

Loading

M1kep Jul 4, 2022 •

edited

Loading