Typecheck examples #1177
Typecheck examples #1177
Conversation
218a3e0 to
6f8d32f
Compare
|
Unsure what to do with the rest, please take over @KnorpelSenf |
|
Depends on KnorpelSenf/typegram#11. @wojpawlik how is it even possible that sending files works? We're passing |
e9d3896 to
6f8d32f
Compare
| @@ -13,9 +12,9 @@ const invoice = { | |||
| { label: 'Working Time Machine', amount: 4200 }, | |||
| { label: 'Gift wrapping', amount: 1000 } | |||
| ], | |||
| payload: { | |||
| payload: JSON.stringify({ | |||
There was a problem hiding this comment.
@wojpawlik can you confirm that this is correct?
|
|
||
| const bot = new Telegraf(process.env.BOT_TOKEN) | ||
| bot.start(({ replyWithInvoice }) => replyWithInvoice(invoice)) | ||
| bot.command('buy', ({ replyWithInvoice }) => replyWithInvoice(invoice, replyOptions)) | ||
| bot.on('shipping_query', ({ answerShippingQuery }) => answerShippingQuery(true, shippingOptions)) | ||
| bot.on('shipping_query', ({ answerShippingQuery }) => answerShippingQuery(true, shippingOptions, undefined)) |
There was a problem hiding this comment.
We may need to redefine that function's signature. It would be nice if undefined is not required here.
There was a problem hiding this comment.
BY adding true and false overloads?
There was a problem hiding this comment.
How are you going to make true and false overloads for string | undefined?
There was a problem hiding this comment.
Nvm, you mean overloads for ok. Yes, that makes a lot of sense.
| export interface InputFileByPath { | ||
| source: string | ||
| } | ||
| export interface InputFileByReadableStream { | ||
| source: NodeJS.ReadableStream | ||
| } | ||
| export interface InputFileByBuffer { | ||
| source: Buffer | ||
| } | ||
| export interface InputFileByURL { | ||
| url: string | ||
| filename?: string | ||
| } |
There was a problem hiding this comment.
Is this correct? I'm not sure if this accurately describes the InputFiles that telegraf can handle.
|
|
||
| /** @deprecated */ |
There was a problem hiding this comment.
This is a partial redefinition of the arguments of sendInvoice in typegram. Should this be removed?
| @@ -269,7 +269,7 @@ class Telegram extends ApiClient { | |||
| */ | |||
| sendSticker( | |||
| chatId: number | string, | |||
| sticker: tt.InputFile, | |||
| sticker: tt.Opts<'sendSticker'>['sticker'], | |||
There was a problem hiding this comment.
It is important to note here that string must not be part of InputFile. It is not possible to use a URL or a file_id when sending a sticker, and typegram already models this correctly. The correct type is simply InputFile as it is defined.
There was a problem hiding this comment.
Also confer KnorpelSenf/typegram#15
| @@ -282,7 +282,7 @@ class Telegram extends ApiClient { | |||
| */ | |||
| sendVideo( | |||
| chatId: number | string, | |||
| video: tt.InputFile, | |||
| video: tt.Opts<'sendVideo'>['video'], | |||
There was a problem hiding this comment.
typegram makes sure that string is included in this definition. Videos can be sent via file_id.
src/core/network/client.ts
Outdated
| const hasMedia = includesMedia(payload) | ||
| let buildConfig: Promise<RequestInit> | ||
| const configPayload: { [key: string]: unknown } = payload | ||
| if (hasMedia) { | ||
| configPayload[method] = method | ||
| buildConfig = buildFormDataConfig(configPayload, options.agent) | ||
| } else { | ||
| buildConfig = buildJSONConfig(payload) | ||
| } |
There was a problem hiding this comment.
...what does it improve?
There was a problem hiding this comment.
It makes the code compile. I have no idea why it suddenly didn't anymore. Do let me know what the issue was in case you decide to investigate this.
There was a problem hiding this comment.
Type '{ method: M; } & Opts<M>' is not assignable to type '{ [key: string]: unknown; }'.ts(2322) was the error, perhaps related to https://devblogs.microsoft.com/typescript/announcing-typescript-3-5/#k-string-unknown-is-no-longer-a-wildcard-assignment-target
Co-authored-by: Wojciech Pawlik <woj.pawlik@gmail.com>
832e107 to
11b240d
Compare
|
Everything's alright @KnorpelSenf? I'll merge this (without squashing) if yes, even tho last 2 examples aren't checked. |
#1074 (comment)
I decided that the best way to verify that examples are correct is to typecheck them. This revealed several typing issues, some of which I fixed.
Type of change