Extra strict method arguments

[clavin]

Description

On the topic of method arguments interfaces, thoughts on possibly making those interfaces extra strong by attempting to ensure that certain combinations of arguments are valid?

For example, one could say the following call to chat.postMessage is technically incorrect:

web.chat.postMessage({
    text: 'words, words, words',
    as_user: true,
    icon_url: 'https://lorempixel.com/48/50'
});

Because passing as_user as true means icon_url is ignored. With the right type signature, the above example can throw an error at static analysis.

This can be demonstrated in a smaller case. Say you have an object named shape and it either has the property size or radius, but not both. This this "exclusive or" operation comes at the cost of verbosity in TypeScript:

type Shape = { size: number; radius?: never } | { radius: number; size?: never };

const shape1: Shape = { // error!
    size: 12,
    radius: 13
};

const shape2: Shape = {}; // error!

const shape3: Shape = { // ok
    size: 12
};

const shape4: Shape = { // ok
    radius: 13
};

Going back to the example from before, the implementation would look something like this:

type ChatPostMessageArguments =
    {
        text: string;
        // ...
    } & (
        {
            as_user?: boolean;

            username?: never;
            icon_emoji?: never;
            icon_url?: never;
        } | (
            {
                username?: string;

                as_user?: never;
            } & (
                {
                    icon_emoji?: string;

                    icon_url?: never;
                } | {
                    icon_url?: string;

                    icon_emoji?: never;
                }
            )
        )
    );

The solution is mighty verbose, but the result is that method arguments can be statically checked to conform to the right shape:

Argument of type '{ text: string; as_user: boolean; icon_url: string; }' is
not assignable to parameter of type 'ChatPostMessageArguments'.

---

Obviously, this is a lot of work to do, but I feel like it might just be worth it (or, at least, a lot of fun).

Even more obvious, doing this would end up with bugs where correct combinations turn out to throw errors.

To mitigate this, the definition of Method at the top of method.ts could be altered so an optional type argument could be supplied to methods to be automatically unioned for method arguments, allowing callers to assert that their arguments are right by supplying any as the argument:

export default interface Method<MethodArguments extends WebAPICallOptions> {
    <MethodArgsOverload = {}>(options?: MethodArguments & MethodArgsOverload): Promise<WebAPICallResult>;
    <MethodArgsOverload = {}>(options: MethodArguments & MethodArgsOverload, callback: WebAPIResultCallback): void;
}

Then a consumer could just do:

web.chat.postMessage<any>({
    text: 'words, words, words',
    as_user: true,
    icon_url: 'https://lorempixel.com/48/50'
});

to assert that their arguments are correct.

And finally, this doesn't affect JavaScript users at all 👏 🙌 ✨

What type of issue is this? (place an x in one of the [ ])

• [ ] bug
• [x] enhancement (feature request)
• [ ] question
• [ ] documentation related
• [ ] testing related
• [x] discussion

Requirements (place an x in each of the [ ])

• [x] I've read and understood the Contributing guidelines and have done my best effort to follow them.
• [x] I've read and agree to the Code of Conduct.
• [x] I've searched for any related issues and avoided creating a duplicate issue.

[aoberoi]

This is quite the ambitious proposal, I like it! In spirit, I'm totally aligned with the vision of providing useful static analysis checks that save our users the round-trip to the docs in order to understand which combinations of arguments are valid, and which aren't. The intention to make the argument types as self-describing as possible is why I left comments like the following in the code: https://github.com/slackapi/node-slack-sdk/blob/c49ba78068c359e2a75951f69198256d20bfa895/src/methods.ts#L200.

I think the ideas outlined here serve that vision very well. I have a few concerns, but i think they are addressable:

1. Out of practicality, I think its more important to finish defining the existing argument types with some basic level of fidelity (all argument names specified, required/optional expressed, common "protocol" argument sets abstracted). #505

2. We should also think about how we might be able to machine-generate type information. Another part of our vision is to get out of the business of hand-tweaking these types, but rather using a process to generate them from the OpenAPI Spec. If we take a look at chat.postMessage's arguments, to follow the example above, you'll notice that there's no metadata about which combinations of arguments are acceptable (except in the description strings). I'm not expert on the specification, but from my read, I don't see how this even could be expressed. I'm not trying to say that the OpenAPI Spec should be a limiting factor, but we should plan on how to build a process that is in harmony with it. This could mean the first pass of the process generates the basic information, while we maintain a smaller set of hand-tweaked enhanced types that apply over those in a second pass. This is just an early idea.

3. We need to give a nod to readability of the source. I don't consider myself a sophisticated TypeScript developer, but one with an intermediate level of experience. I will admit that the conditions (&s, |s, and ()s) are non-trivial for me to parse. If that's the case for me, it could be potentially much worse for others. Are there other means of expressing the same information in a syntax that is more readable? i explored the got library definitions (not perfect) and noticed the technique of using generics to help expand the type for options. I don't think that would work for these argument types, but I just wanted to spark some creativity. I do think using the optional type argument as you mentioned above is a step in the right direction.

4. How might these input types relate to a future output type that we could provide? right now, all properties of the return value (aside from the generic ones in WebAPICallResult) have to be asserted to exist before they can be used, and this is annoying. This might be a place where we need to introduce generics.

Happy to continue this discussion! When we have a specification for the work we actually want to merge, lets start a new enhancement issue. A change this large would definitely warrant a minor version increment.

[clavin]

MessageAttachment Sample

To foster discussion, here's a sample typing for MessageAttachment:

`MessageAttachment` sample

export type MessageAttachment = {
  color?: string;
  pretext?: string;
  actions?: MessageAttachmentAction[]; // actions are complicated, so they get their own type
} & (
  // message text: at least one of `fallback` or `text`
  {
    fallback: string;
    text?: string;
  } | {
    text: string;
    fallback?: string;
  }
) & (
  // author properties: `author_link` and `author_icon` depend on `author_name`
  {
    author_name: string;
    author_link?: string;
    author_icon?: string;
  } | {
    author_name?: string;
    author_link?: never;
    author_icon?: never;
  }
) & (
  // title properties: `title_link` depends on `title`
  {
    title: string;
    title_link?: string;
  } | {
    title?: string;
    title_link?: never;
  }
) & {
  // fields: at least one of `title` or `value`
  fields?: {
    short?: boolean;
  } & (
    {
      title: string;
      value?: string;
    } | {
      value: string;
      title?: string;
    }
  )[];
} & (
  // thumbnail/image: `image_url` takes precedence over `thumb_url`
  {
    image_url?: string;
    thumb_url?: never;
  } | {
    thumb_url?: string;
    image_url?: never;
  }
) & (
  // footer: `footer_icon` depends on `footer`
  {
    ts?: string | number;
  } & (
    {
      footer: string;
      footer_icon?: string;
    } | {
      footer?: string;
      footer_icon?: never;
    }
  )
);

Mind you, this code doesn't really take into account the suggestions mentioned in the last comment.

Feel free to use this code as a starting point to explain (with specific examples) possible issues, solutions, and insights.

---

Overload Modification

Also, I'd recommend altering the overload signature previously proposed to be a union (|) rather than an intersection (&):

export default interface Method<MethodArguments extends WebAPICallOptions> {
    <MethodArgsOverload = never>(options?: MethodArguments | MethodArgsOverload): Promise<WebAPICallResult>;
    <MethodArgsOverload = never>(options: MethodArguments | MethodArgsOverload, callback: WebAPIResultCallback): void;
}

This is because it actually fulfills the idea that users can overload types by specifying their own type. The definition in the original issue message wouldn't work when a user to specified { key: type; } as the overload would be intersected (thus the provided arguments had to match both the original args [bad] and the overload) rather than one, the other, or both (the intended behavior).

Specifying any as the overload still works, but specific overloads also work. For example:

const attachment: Overload<MessageAttachment, { image_url: string; thumb_url: string; }> = {
    text: 'hi',
    image_url: 'https://cats.images/random',
    thumb_url: 'https://dogs.images/random'
};

...can be written to assert that certain arguments are correct, while still providing the ability to check other arguments, i.e.:

const attachment: Overload<MessageAttachment, { image_url: string; thumb_url: string; }> = {
    text: 'hi',
    image_url: 'https://cats.images/random',
    thumb_url: 'https://dogs.images/random',
    footer_icon: 'https://animals.images/random' // causes error !!!
};

(Overload<A, B> is just A | B)

---

OpenAPI Compatibility

Just from taking a quick look at the linked OpenAPI spec, it seems there is a supported schema field which can define the (modified JSON) Schema of an object. Since this schema field is based off the JSON schema spec, I feel it's likely possible to describe many of these combinations via this field.

This does, however, make the process of writing the OpenAPI Slack spec steeply more difficult, and would be a lot to be expected of.

[aoberoi]

some thoughts:

Overload Modification

This is an interesting proposal. I like it, but I have a competing idea. Here's the challenge:

// first form
web.chat.postMessage(argument); // this should work only if the argument is strictly of type ChatPostMessageArguments

// second form
web.chat.postMessage<MyInterface>(argument); // this should work only if argument is strictly of type MyInterface, and MyInterface extends ChatPostMessageArguments

// third form
web.apiCall('chat.postMessage', argument); // this should work only if argument extends WebAPICallOptions, and since this is an empty interface it basically means it cannot be a scalar (number, string, boolean) or an array

I think these could be all the use cases we want. for an ordinary user, you'll just use the first form. If you're an expert you'll either use the second or third form. The second form works if you have some knowledge of a hidden argument but you still want the safety of the type system to do some work for you. The third form works if you have some knowledge of a hidden method so basically all bets are off.

OpenAPI Compatibility

Even if we utilize the schema options, it sounds like we'd have to "expand" all the conditional dependencies into a set of schema for all the valid combinations (e.g. MessageAttachmentWithHeader, MessageAttachmentWithHeaderWithFooter, etc). I say this because the only mechanism I see that can do the sorts of conditions we need is in the composition and inheritance section, and i think it might be too simple to describe our formats. Again, I'm not an expert on this, so I could be wrong.

What I am pretty sure of, is that we are highly unlikely to get that kind of detailed specification out of Slack's official OpenAPI spec. It's just too intricate and it would be a nightmare to maintain (given the knowledge I have about how that schema is built). So I think the logical next step is to explore/prototype a sort of 2-pass process where we could start with the OpenAPI spec and decorate/modify the types it gives us with as much information as we can about those object shapes.

MessageAttachment sample

Wow, the MessageAttachment is probably one of the most complicated structures in the entire API. Kudos to you for taking it on to describe it.

I still think the way you've implemented the type is hard to read, and therefore error prone. Let's take this part for example:

(
  // author properties: `author_link` and `author_icon` depend on `author_name`
  {
    author_name: string;
    author_link?: string;
    author_icon?: string;
  }
)

I don't think this is accurate. The docs say author_name must be present for author_link or author_icon to also be present. I have to start expanding this in my head:

{ author_name: string, author_link: undefined | string, author_icon: undefined | string } | { author_name: undefined | string, author_link: undefined | never, author_icon: undefined | never }

simplify, which requires memorizing the identity rules for the type system. i believe SomeType | never => SomeType

{ author_name: string, author_link: undefined | string, author_icon: undefined | string } | { author_name: undefined | string, author_link: undefined, author_icon: undefined }

well now these two types completely overlap and it reduces further

{ author_name: undefined | string, author_link: undefined | string, author_icon: undefined | string }

I think we could fix this by instead using:

(
  // author properties: `author_link` and `author_icon` depend on `author_name`
  {
    author_name: string;
    author_link?: string;
    author_icon?: string;
  }
)

I can't even be sure this is right until i evaluate it with the rest of the expression. The concern is that it took that many mental steps for a reader to figure just this part out, with quite a bit of set theory and type system knowledge memorized (or looked up). And I'm not even done.

On top of that concern, do we even know what the Intellisense completion offers when the structure is that complicated? If the real-time type hints show me that entire blob on top of my cursor, I'm likely to be more confused about what is valid than the structure we currently have. Maybe this can be mitigated by giving the sub-expressions (things inside the ()) a name, and joining the names with the logical operators.

[clavin]

Generics

I actually originally wanted a way to express these constraints as generics originally (possibly different than what @aoberoi meant by "generics") but I couldn't remember a way to use the keys of a generic type.

Well, I just remembered how (there's this lovely operator called keyof), so I quickly got to work on making a few generics to convey some of the more common constraints I used in my MessageAttachment example:

// Makes all keys of T of type `never`
type Not<T> = { [K in keyof T]: never; };

// Requires matching exactly one of A or B
type OneOf<A, B> = (A & Partial<Not<B>>) | (B & Partial<Not<A>>);

// Requires matching at most one of A or B
type MaxOneOf<A, B> = Partial<OneOf<A, B>>;

// Requires matching at least one of A or B
type MinOneOf<A, B> = (A & Partial<B>) | (B & Partial<A>);

// Requires all properties of Required to be present before any properties of Dependent can be present.
type Requires<Required, Dependent> = (Required & Partial<Dependent>) | Partial<Required & Not<Dependent>>;

Here's a rewrite of my previous MessageAttachment sample using these generic types, which heavily shows off their (wonderful ✨) powers:

export type MessageAttachment = {
  color?: string;
  pretext?: string;
  actions?: MessageAttachmentAction[];
  fields?: (
    { short?: boolean; } &
      MinOneOf<{ title: string; }, { value: string; }>
  )[];
  ts?: string | number;
} &
  MinOneOf<{ fallback: string; }, { text: string; }> &
  Requires<{ author_name: string; }, { author_link: string; author_icon: string; }> &
  Requires<{ title: string; }, { title_link: string; }> &
  MaxOneOf<{ image_url: string; }, { thumb_url: string; }> &
  Requires<{ footer: string; }, { footer_icon: string; }>;

fields is defined here for the sake of example; in actuality, it should be made its own type in order to appear more simply in intellisense.

I think these generics (which I wrote yesterday, funny enough) should help the point of readability, so long as the names of these generics make sense. Names are hard; feel free to change the ones I chose if there's something you feel makes more sense (especially Requires, which I feel is misleading).

Intellisense

I'm at a bit of a crossroads on this one.

When hovering over the MessageAttachment type, intellisence will show whatever that attachment's expanded source is. Oddly, the generics I defined above aren't preserved, but Partial is. This means that the definition given to the user when they hover over MessageAttachment is akin to the source of a compiled executable: gibberish that shouldn't be read by humans.

On the other hand, the auto-complete intellisense essentially shows the real type of all arguments:

I feel like this outcome would be my choice as a user as I'd like to be able to see all arguments and their types here without having to check the API docs.

Side note: even if color is defined as 'x' | 'y' | 'z' | string (as it currently is in the repo), intellisense will always simplify this to just string, including auto-complete and hovering over the type. This is why I dropped the verbose options of color in my definitions thus far.

I can see having those options defined being useful in one case: when a user reads the source of the type. Thus, I feel the best place for these options are in comments, somewhere next to the argument--especially a doc-comment, if those ever come along.

---

I'd also like to draw some attention to the TypeScript Playground. This tool is super useful for testing out these types and making sure they do/don't throw errors. Further, this playground is built on Monaco Editor, the web-based editor that powers VS Code (complete with TypeScript intellisense already set up), which is where I expect most (obviously not all) TypeScript development to happen.

[aoberoi]

Wow, the generics example is very interesting, and does go a long way to improve the readability. They seem so useful that I'd almost suggest publishing them in a dedicated package, with tests to prove their properties, and then we may take that package as a dependency.

Also, thanks for investigating the Intellisense capabilities. I agree that the hover-over hints are not ideal, so I think we should provide some feedback to Microsoft about that. (Partial is probably treated differently because its a built-in, but I'm not sure I like that.) I also agree that the completion hints representing the fully expanded type makes the most sense. I haven't given it a shot yet, but I'd also like to know how useful the error messages are when you don't get something right. I believe the compiler might give you every step of the expansion in the error message, so the generics would be helpful there too.

[stevengill]

We are exploring the possibility of solving this by generating types from a common schema or specification.

[github-actions[bot]]

👋 It looks like this issue has been open for 30 days with no activity. We'll mark this as stale for now, and wait 10 days for an update or for further comment before closing this issue out.

[filmaj]

web-api v7 should have addressed this issue.