Leverage unionization #1195
Leverage unionization #1195
Conversation
There was a problem hiding this comment.
Impressive.
I'm not sure how this typechecks without updating corresponding static methods, but I expect inference to break when you do.
Minor thing: ideally, for example in composer.on('message', ctx => ...), ctx.editedMessage and others should be undefined (currently they're left as optional).
telegrafsupports switching onchannelMode. [...] It is technically feasible to makemessageoptional every time and also providechannel_postas an optional property.
That's what I did in my failed attempt.
Updating these methods is both required and not a problem. Note that there is one difference to a previous approach: it is not possible to infer from previous middleware arguments the types of the next. This is available for
Good idea! I would not have thought about it. I added that now. |
|
I do have to add that we should write more documentation about how to use @wojpawlik feel free to play around with the example bots, the types are working well there IMO. Looking forward to hearing your opinion about it. We also need to decide on the problem with |
src/composer.ts
Outdated
| C extends Context, | ||
| T extends tt.UpdateType | tt.MessageSubType | ||
| > = C & | ||
| // TODO: support subtypes for: callback query, channel post, maybe more? |
There was a problem hiding this comment.
Do we want to support type inference in even more cases, not just for subtypes of message?
There was a problem hiding this comment.
Currently, when using a MessageSubType, we infer subtypes for the message property only, even though it could come from a different update type.
Take this code:
bot.on('text', ctx => {
const text = ctx.message.text // works
})We correctly infer ctx.message.text: string as well as ctx.update.message.text: string.
However, mount not only checks message texts, but also the content of all update types, including callback queries, inline queries, channel posts, etc.
Take this code:
bot.on('channel_chat_created', ctx => {
const chat = ctx.channelPost.chat // error
})We infer both ctx.message.channel_chat_created and ctx.update.message.channel_chat_created as true and ctx.channelPost as undefined, all of which is incorrect.
I'm suggesting that we should also do a similar thing for other subtypes that we provide. It may make sense to do this generically, but I believe that this will create too complex types again, so we may have to come up with something smarter.
Note: the TODO comment in the code is not quite correct about callback queries as neither 'data' nor 'game_short_name' are valid message subtypes, so callback queries should in fact not be the issue here. It's about channel posts.
There was a problem hiding this comment.
Now that I think about it, it basically boils down to supporting channel mode also in this place.
|
Tbh., I'd suggest to remove |
Sounds like a perfect fit for template literal types. I like the idea actually. This should make it much easier to add types and to explain the lib. I remember well how confused I was in the beginning that I could do both I suggest in turn that we leave this PR as it is, get a review done, and then open a third PR that does the rest before releasing 4.0. |
|
Actually, the |
|
I see the point that reworking how to listen to update types should be done thoroughly. This opens two questions:
|
|
I put every breaking change that comes up into the milestone. |
|
One possible solution here is to exclude After applying this change, the types are still a bit incorrect for |
|
Nothing really. It makes things less readable and in some cases more maintainable. I try to avoid them for complex type signatures. If I do advanced stuff, I don't want things to be more implicit than necessary. Why? |
|
DRY? |
|
Yes. That's what I mean with more maintainable. It's useful and I like it for what it can do, but it comes at a cost and sometimes I think it's better not to pay that. |
|
But does the PR work? Do the voodoo magic types do their job? I'm looking forward to your |
|
Anything else, or can I merge? |
You can merge. |
|
Now it's my time to shine |
|
Looking forward to it! Also, you've already been shining pretty bright so far with very good reviews. (Not to forget the rest.) |
Description
typegramallows to narrow down certain types (most notablyUpdateandMessage) to its different subtypes. However,telegrafdoes not use this feature yet (reason).We therefore have to check if
message.textis present even in the case where we register the middleware only fortextevents and hence know in advance that it is always truthy.This PR adds a number of new types to the library that allow for type inference on the context object in middleware functions. As a result, a context object is correctly assumed to provide a
messageproperty when it is the argument of a middleware function that is being passed tobot.on('message'). The same is true for subtypes of messages.Furthermore, it is possible to provide a list of update types and message subtypes. The type inference will then guarantee that all of the relevant properties are available, but optional. Thus, we get autocompletion for the properties that make sense, while still having to check explicitly which of them is actually present. Even here it is possible to narrow down the type of update, i.e. when registering for
['callback_query', 'text']and then checking if either of the update types is present, the other option will be discarded. (Callback queries and text messages are mutually exclusive in an update.)Registering middleware on
forwardcorrectly assures thatforward_dateis present, even though the propertyforwardwill never exist on the message. In other words, the type mapping oftelegrafthat happens under the hood is regarded.Even though this PR is good as it is, there is still one open question that we may want to discuss before merging.
telegrafsupports switching onchannelMode. When activated, all channel posts are handled as messages. What happens in this case? It is technically feasible to makemessageoptional every time and also providechannel_postas an optional property. In the current state,channel_postis said to be unavailable when registering middleware fortextwhilemessageis guaranteed to be present. This happens regardless ofchannelMode(which is obviously wrong). It may be possible to do this by adding another type variable toComposer.Type of change
Only changes type annotations. Might require elaborations on how to use this project with TypeScript, depending on how we decide on the aforementioned open question.
How Has This Been Tested?
Checklist: