Feature/ts improvements #1084
Feature/ts improvements #1084
Conversation
…eature/ts-improvements
| import http from 'http' | ||
| import https from 'https' |
There was a problem hiding this comment.
What's the benefit of doing this? The drawback is obvious -- it'll need to be changed back if esModuleInterop is disabled.
There was a problem hiding this comment.
I prefer to make it as tidy as possible for now, esModuleInterop changes later are easy to fix
| telegram: {}, | ||
| retryAfter: 1, | ||
| handlerTimeout: 0, | ||
| channelMode: false, | ||
| contextType: Context, | ||
| } | ||
|
|
||
| const noop = () => {} | ||
| const noop: () => void = () => undefined |
There was a problem hiding this comment.
This is just more verbose way of doing the same thing, isn't it?
There was a problem hiding this comment.
No, this fixes a type error further down, where another function expects a void callback, and not an undefined callback
There was a problem hiding this comment.
How? () => {} is already a void callback.
There was a problem hiding this comment.
Weird, NOW it is for me too, previously this wasn't a void callback but the type turned into () => undefined.. uhh
| // eslint-disable-next-line @typescript-eslint/restrict-template-expressions | ||
| debug(`Launching @${botInfo.username}`) | ||
| debug( | ||
| `Launching ${botInfo.username == null ? 'bot' : '@' + botInfo.username}` |
There was a problem hiding this comment.
Dead code, bots always have a username as far as I'm aware. The real solution here is to tighten the type returned by getMe and stored in botInfo to something like
interface BotUser {
id: number
is_bot: true
first_name: string
username: string
can_join_groups: boolean
can_read_all_group_messages: boolean
supports_inline_queries: boolean
}There was a problem hiding this comment.
Sure, we'll fix that later, I'd enjoy seeing some extending here, since BotUser would just be an extension of User in this case
| console.error( | ||
| ('stack' in err ? err.stack : err.toString()).replace(/^/gm, ' ') | ||
| ) |
There was a problem hiding this comment.
Nothing wrong with this, but it also doesn't seem to fix anything.
There was a problem hiding this comment.
It checks that err.stack is a property on err, instead of checking if it's nullish
Co-authored-by: Wojciech Pawlik <woj.pawlik@gmail.com>
…eature/ts-improvements
…elegraf into feature/ts-improvements
|
Any ETA for this? What is the current state? I'm waiting for this because it is blocking #1074. |
|
I don't think it's blocking, as long as you don't touch |
|
@wojpawlik would it be possible for me to give you write access to my fork? I'm not very interested in arguing about these tiny things I really don't care about, but I'm fìne with letting you have final say on all of these as long as my code gets merged. I enjoy making larger contributions than these changes |
|
Save your time you two. I'll just pull your work into my fork and then continue the migration journey from there. I'll most likely need to touch most lines of the codebase anyway. After all, half of it is still JS. I applied a reasonably tight TS config for a project of this size and I'm at nearly 300 compilation errors now. It's not just gonna take a week before this is done. I'll submit a separate PR once I have a first proper step to share. |
|
@trgwii please check out if something slipped through on my side or if you're missing something else in my PR, I'll happily add it |
Description
Adds improved typings to src/telegraf.ts
How Has This Been Tested?
All tests pass
Test Configuration:
Checklist: