Feature: Type safe custom context properties in TypeScript

[szydlovski]

Summary

This pull request enables developers to maintain type safety when adding custom properties to the context middleware arg.

Description

This is partially related to #897. That issue was closed with a pull request which added built-in context fields to the type definitions, however it did not address the issue of using custom context properties.

Right now Bolt does not offer any way to add TypeScript typings to the context object. Consider this example:

const app = new App();

app.use(async ({ context, next }) => {
  context.foo = { bar: 'baz' };
  await next();
})

app.command('/test', async ({ ack, respond, context }) => {
  await ack();
  await respond(context.foo.bar);
})

This code would work, but there is no way to record type information about context.foo. The only workaround is something like this:

const app = new App<MyContext>();

interface MyContext {
  foo: { bar: string };
}

app.use(async ({ context, next }) => {
  const myContext = context as MyContext;
  myContext.foo = { bar: 'baz' };
  await next();
})

app.command('/show-user-id', async ({ ack, respond, context }) => {
  const myContext = context as MyContext;
  await ack();
  await respond(`Your custom user id is ${myContext.foo.bar}`);
})

This does work but is very clunky, verbose and overall causes the developer experience to suffer. Casting like this should be reserved for rare, special cases, not for something as basic as middleware.

Background

How do other frameworks handle this? Koa takes a type parameter ContextT which is then added to the build-in context type definition in middleware listeners:

interface MyContext {
  foo: { bar: string }
}

const app = new Koa<any, MyContext>();

app.use(async (ctx) => {
  ctx.foo = { bar: 'baz' };
})

Solution

My implementation is similar to Koa and works like this:

interface MyContext {
  foo: { bar: string };
}

const app = new App<MyContext>();

app.use(async ({ context, next }) => {
  context.foo = { bar: 'baz' };
  await next();
})

app.command('/show-user-id', async ({ ack, respond, context }) => {
  await ack();
  await respond(`Your custom user id is ${context.foo.bar}`);
})

This could be accomplished in a number of ways, I decided that the following is the cleanest option:

1. The AllMiddlewareArgs interface is the "root" of middleware args, so I added a CustomContext type parameter to it, constrained and defaulting to StringIndexed, and modified its type definition to define the context property as Context & CustomContext

export interface AllMiddlewareArgs<CustomContext = StringIndexed> {
  context: Context & CustomContext;
  logger: Logger;
  client: WebClient;
  next: NextFn;
}

2. The link between listener args and AllMiddlewareArgs is the Middleware interface, so I added a CustomContext type parameter to it as well, constrained and defaulting to StringIndexed, ​and modified its type definition to pass that type argument to AllMiddlewareArgs.

export interface Middleware<Args, CustomContext = StringIndexed> {
  (args: Args & AllMiddlewareArgs<CustomContext>): Promise<void>;
}

3. Finally, I added a CustomContext type parameter to the App class, constrained and defaulting to StringIndexed, and modified its methods' type definitions to pass that type parameter to Middleware

export default class App<CustomContext extends StringIndexed = StringIndexed> {
  // ...
  public command(
    commandName: string | RegExp,
    ...listeners: Middleware<SlackCommandMiddlewareArgs, CustomContext>[]
  ): void {
    this.listeners.push([onlyCommands, matchCommandName(commandName), ...listeners] as Middleware<AnyMiddlewareArgs>[]);
  }
  // ...
}

Because all added type parameters default to StringIndexed and Context already extends StringIndexed they can still be used in other parts of the codebase without a type parameter or any changed to the typings. Other parts of the codebase don't need information about custom context properties because those are by definition added by the end developer and hence not needed in internal framework logic.

These changes do not modify any typings outside of this rather narrow scope, do not introduce any changes to the logic of the framework, and should work with any existing codebase.

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.

[srajiang]

@szydlovski - 🎉 Very excited to see this contribution! Let's give a few days to let folks take a look and add their comments and feedback.

[codecov]

Codecov Report

Merging #1157 (a799388) into main (a55ec4c) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1157   +/-   ##
=======================================
  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% <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 a55ec4c...a799388. Read the comment docs.

[szydlovski]

I've been doing some background processing on this and I think I've found a more robust solution to this problem. Extending context is fine, but true typed middleware support should probably extend middleware args themselves. Another upside is that this approach is even simpler and less invasive, in my opinion. Here's some code:

interface EmptyMiddlewareArgs {}

export default class App<AppMiddlewareArgs extends EmptyMiddlewareArgs = EmptyMiddlewareArgs> {
  // ...
  public command(
    commandName: string | RegExp,
    ...listeners: Middleware<AppMiddlewareArgs & SlackCommandMiddlewareArgs>[]
  ): void {
    // ...
  }
  // ...
}

This way middlewares are not limited to only extending context, and changes are limited to the App class.

This could be improved even futher by recording the middlewares attached to an app:

export default class App<AppMiddlewareArgs extends EmptyMiddlewareArgs = EmptyMiddlewareArgs> {
  // ...
  private middleware: Middleware<AnyMiddlewareArgs>[];
  // ...
  public use<T extends EmptyMiddlewareArgs>(middleware: Middleware<AppMiddlewareArgs & T>): App<AppMiddlewareArgs & T> {
    this.middleware.push(middleware as Middleware<AnyMiddlewareArgs>);
    return this as unknown as App<AppMiddlewareArgs & T>;
  }
  // ...
}

Then we can define some typed middleware:

interface Example1MiddlewareArgs { customArg: string }
const example1Middleware = (value: string): Middleware<Example1MiddlewareArgs> =>
  async (middleware) => void (middleware.customArg = value)

interface Example2MiddlewareArgs { anotherCustomArg: number }
const example2Middleware = (value: number): Middleware<Example2MiddlewareArgs> =>
  async (middleware) => void (middleware.anotherCustomArg = value)

And attach them to the app in chained calls:

const app = new App()
  .use(example1Middleware('foo'))
  .use(example2Middleware(1))

app.command('/test', async ({ ack, respond, customArg, anotherCustomArg}) => {
  await ack();
  await respond(`${customArg}, ${anotherCustomArg}`)
});

customArg and anotherCustomArg will be typed correctly.

Finally, this approach could enable another somewhat common pattern - a single middleware attached to a specific listener. For example, I've been using a command args parser middleware (to parse command.text into separate values). Ideally I should be able to call app.command like this:

// this is obviously a simplified version :)
const parseCommandArgs = (): Middleware<SlackCommandMiddlewareArgs & {
  parsedCommandArgs: string[];
}> => async (middleware) => {
  middleware.parsedCommandArgs = middleware.command.text.split(' ');
}

app.command('/test', parseCommandArgs(), async ({ack, respond, parsedCommandArgs }) => {
  const firstArg = parseCommandArgs[0];
  await ack();
  await respond(`First argument: ${firstArg}`);
})

Obviously right now this cannot be typed correctly, but it could - with this overload on the command method. Similar overloads could be added for other listeners (action, message etc.):

export default class App<AppMiddlewareArgs extends EmptyMiddlewareArgs = EmptyMiddlewareArgs> {
  // ...
  public command<T>(commandName: string | RegExp, middleware: Middleware<SlackCommandMiddlewareArgs & AppMiddlewareArgs & T>, listener: Middleware<SlackCommandMiddlewareArgs & AppMiddlewareArgs & T>): void
  public command(commandName: string | RegExp, ...listeners: Middleware<AppMiddlewareArgs & SlackCommandMiddlewareArgs>[]): void
  public command(commandName: string | RegExp, ...listeners: Middleware<AppMiddlewareArgs & SlackCommandMiddlewareArgs>[]): void {
    // ...
  }
  // ...
}

[seratch]

@szydlovski Thanks for your great suggestion here!

I like your approach to give more types in context because it does not bring any breaking changes to existing ones. Also, I agree that it could be useful in the situations like context enhancement here: #1109 (comment) (In the issue, I'm working on the context enhancement on the ReceiverEvent side).

Although your pull request does not need to resolve this issue, one thing I wanted to mention for others here is that, even with the changes here, still the context object is not type-safe in TS. The reason is that the Context type is StringIndexed based and inheriting StringIndexed makes the types in its subclasses useless. We may want to remove StringIndexed in future major versions for better TS support but we haven't decided to do so. It's an obvious breaking change to existing apps. Thus, the benefit that we can receive at this moment is that an editor (perhaps, VS Code for most people) tells you the available properties in the context as well. Right, that's not a small benefit.

Finally, this approach could enable another somewhat common pattern - a single middleware attached to a specific listener.

In the first place, the bolt-js project does not encourage developers to add additional properties to middleware arguments (although it works fine in many cases). The documents do not have a warning about it (because this is a general issue in JS) but the biggest issue is the risk of naming conflicts with the built-in properties in the future. In the case where you unexpectedly modify built-in properties, troubleshooting of the issue can be hard. Instead, we recommend putting any additional properties into context object. For this reason, I am not positive about the alternative approach in the comment. Other maintainers may have a bit different views, though.

Overall, if we go with the changes only for context, this proposal looks good to me.

@szydlovski If you have more time, adding some unit tests covering the use cases would be appreciated.

[szydlovski]

still the context object is not type-safe in TS. The reason is that the Context type is StringIndexed based and inheriting StringIndexed makes the types in its subclasses useless. We may want to remove StringIndexed in future major versions for better TS support but we haven't decided to do so. It's an obvious breaking change to existing apps

StringIndexed could be the default value for CustomContext, in which case - correct me if I'm wrong - removing it as a base for Context would not break existing apps.

As for the middleware args and their namespacing issues, have you considered defining the built in properties as non-configurable getters? Obviously that could still be overwritten by a developer with enough dedication, but it would make accidentally breaking things singificantly less likely (if custom middleware args become a supported feature at some point).

[seratch]

Thanks for your reply.

removing it as a base for Context would not break existing apps.

If we completely remove StringIndexed from context objects, it should break existing TS apps. Some code accessing custom properties will need type casting to any. Thus, when we make this change, it requires a major version release and we should provide a corresponding part in its migration guide.

have you considered defining the built in properties as non-configurable getters?

Although the framework code can be a bit more complex, this may be a good improvement for safety.

[filmaj]

Hey, thanks for the PR / ideas @szydlovski!

My humble opinion: given what @seratch mentioned about how Slack wants to keep the middleware arguments "out of bounds" for developers so that we can change its structure down the road, perhaps better to keep the suggested change to your original approach of specifying the context's type rather than extending the middleware args?

As an app author, I also don't particularly mind the approach of casting the context to some interface or type I can define on my own (as you described in your original post) - though fair warning, I don't consider myself a typescript power user so perhaps that is due to my own inexperience / ignorance!

[raycharius]

Wonderful feature! 💯

[raycharius]

I played around with this a bit and wanted to share another use case that the current implement of this really great feature doesn't cover.

A bit of context – in all of my apps, before a payload is passed into the final listener, it is passed into another middleware where I programmatically parse:

• Any values passed back to the backend through a block action payload
• Form values from a submission payload

I do this to keep from having to access those deeply nested values in the payload for every single value. And if there is a clear contract between the action IDs/block IDs/callback IDs/private metadata and that middleware – it works really well and saves a lot of time and code.

And I pass those parsed values to the listener as a part of context. So the shape of context can differ from listener to listener. As @seratch mentioned, this obviously isn't type-safe (as is often the case with TS), but there is the added value of having the property hints in the IDE and type checking being applied to any functions those (hopefully correctly typed) values are passed into.

The proposed implementation works great when the shape of context is the same globally, but doesn't allow for there to be different properties across different listeners.

A very simplified example, in which there are no globally applicable differences in the context object (so I don't pass a type to App):

// handle-foo.ts

interface Foo {
  foo: string;
}

export const handleFoo: Middleware<SlackActionMiddlewareArgs<BlockAction>, Foo> = async ({ context }) => {
  console.log(context.foo);
};

// handle-bar.ts

interface Bar {
  bar: string;
}

export const handleBar: Middleware<SlackActionMiddlewareArgs<BlockAction>, Bar> = async ({ context }) => {
  console.log(context.bar);
};

// app.ts

import { App } from '@slack/bolt';
import { parseValues } from './parse-values';
import { handleFoo } from './handle-foo';
import { handleBar } from './handle-bar';

const app = new App({ token: 'xoxb-xxxx' });

app.action({ type: 'block_actions', action_id: 'foo' }, parseValues, handleFoo); // TS error
app.action({ type: 'block_actions', action_id: 'bar' }, parseValues, handleBar); // TS error

The same also goes for passing in a type to App and extending the global custom context:

// handle-foo.ts
import type { GlobalCustomContext } from './types';

interface Foo extends GlobalCustomContext {
  foo: string;
}

export const handleFoo: Middleware<SlackActionMiddlewareArgs<BlockAction>, Foo> = async ({ context }) => {
  console.log(context.foo);
};

// handle-bar.ts
import type { GlobalCustomContext } from './types';

interface Bar extends GlobalCustomContext {
  bar: string;
}

export const handleBar: Middleware<SlackActionMiddlewareArgs<BlockAction>, Bar> = async ({ context }) => {
  console.log(context.bar);
};

// app.ts

import { App } from '@slack/bolt';
import { parseValues } from './parse-values';
import { handleFoo } from './handle-foo';
import { handleBar } from './handle-bar';

import type { GlobalCustomContext } from './types';

const app = new App<GlobalCustomContext>({ token: 'xoxb-xxxx' });

app.action({ type: 'block_actions', action_id: 'foo' }, parseValues, handleFoo); // TS error
app.action({ type: 'block_actions', action_id: 'bar' }, parseValues, handleBar); // TS error

I'm not sure how often developers using Bolt are implementing similar patterns, where things are parsed in earlier middleware or context differs from listener to listener, but felt it was a good case to bring up.

In an ideal world, it would be great to:

• Have the ability to declare a globally applicable custom context object.
• Have the ability to declare a locally applicable custom context object (that extends the global one, if it is declared).

Really love this feature, think it's a great improvement, but wanted to share my two cents

[seratch]

@raycharius Thanks for your comment! I like your approach to have the context type in the middleware type. As you mentioned, it should be even more useful for many use cases. Would having the custom context type in both App and Middleware be possible?

[raycharius]

@seratch I definitely think it could be, looking at the changes in this PR.

@szydlovski What are your thoughts? If you're currently at full capacity, I'm also happy to take a look at this.

[raycharius]

I'm going to check out @szydlovski branch and play around with this a bit this week

[szydlovski]

Unfortunately I didn't have the time to work on this further, but I really like the suggestions posted here. I think this would be a great improvement to Bolt.

[M1kep]

@seratch I believe this can be closed now?

[seratch]

@M1kep Yeah, you are right!

To: all the contributors in this PR and its discussion
Thank you very much for taking the time to improve the type safety of bolt context objects. #1505 will resolve this in a similar way and the change will be included in the next release 🎉

Let us close this PR now.