Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor: adapt for telegraf v4.0 and v3.38 #107

Merged
merged 7 commits into from
Jan 30, 2021
Merged

refactor: adapt for telegraf v4.0 and v3.38 #107

merged 7 commits into from
Jan 30, 2021

Conversation

EdJoPaTo
Copy link
Collaborator

@EdJoPaTo EdJoPaTo commented Jan 16, 2021

The good thing about this: it is not breaking compatibility with 3.38

closes #106

Explanation:

  • The example works with both telegraf 3.38 and 4.0 now
    • const {Telegraf} = require('telegraf') already works in 3.38
    • bot.launch() already works in 3.38 (bot.startPolling() was somewhat deprecated)
  • the Typescript Types are kind of a hack to not be breaking:
    The import Path of MiddlewareFn changed with the update to v4 but Middleware is exported from the main export in v3.38 and v4.0. This results in slightly less good typings but as you normally only use bot.use(sessionLocal) you wont notice it.

Keep in mind that telegraf v4 requires NodeJS 12. But this is not a requirement of this library on its own so no (breaking) change needed there.

this is not breaking compatibility with 3.38
telegraf typings arnt considered breaking in v3 and 3.38 works for sure
@wojpawlik
Copy link

  • The import Path of MiddlewareFn changed with the update to v4

You can simply define it in the file:

type MiddlewareFn<C extends Context> = (ctx: C, next: () => Promise<void>) => Promise<void>

It's compatible with 3.38 and 4.x, and it should be forward-compatible with 5.x.

@EdJoPaTo
Copy link
Collaborator Author

You can simply define it in the file

Thank you for the suggestion @wojpawlik. Haven't thought about that.

Normally I would be against redefining types as this might end up as a potential bug later on. On the other side: MiddlewareFn is a very stable type. Not sure what it best here.

As bot.use(sessionLocal) is the only place where this is needed (which is a place where typings aren't that helpful as that's mostly never changes) I would just stick with Middleware and accept the slightly less good typings in a mostly never looked at place.

@wojpawlik
Copy link

I don't think you have a choice: making return type less specific is a breaking change.

Anyway, why did you change

const LocalSession = require('telegraf-session-local')

to

const LocalSession = require('../lib/session') // require('telegraf-session-local')

in README?

redefining something from a lib is bad but as the import changed it
would be breaking otherwise
@@ -1,7 +1,8 @@
declare module 'telegraf-session-local' {
import { AdapterSync, AdapterAsync, BaseAdapter } from 'lowdb'
import { Context } from 'telegraf'
import { MiddlewareFn } from 'telegraf/typings/composer'

type MiddlewareFn<C extends Context> = (ctx: C, next: () => Promise<void>) => Promise<void>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I meant

Suggested change
type MiddlewareFn<C extends Context> = (ctx: C, next: () => Promise<void>) => Promise<void>
type MiddlewareFn<C extends Context> = (ctx: C, next: () => Promise<void>) => Promise<unknown>

Promise<void> should work too, but void's interactions are inconsistent.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

void is more precise than unknown?

@TemaSM
Copy link
Contributor

TemaSM commented Jan 21, 2021

Thank you guys for all of your work & conversation ❤️
I think we need also to adapt tests and examples to newer Telegraf version, and then I will merge this PR (after successfully running Travis CI tests)

PS> Happy New 2021 year!

@EdJoPaTo
Copy link
Collaborator Author

About the tests and Travis CI, why isnt travis listed in the checks?

@coveralls
Copy link

coveralls commented Jan 22, 2021

Coverage Status

Coverage remained the same at 100.0% when pulling a248c83 on EdJoPaTo:telegraf-v4 into 0de45f6 on RealSpeaker:master.

@TemaSM
Copy link
Contributor

TemaSM commented Jan 23, 2021

Figured out, that Travis-CI moved from .org to .com, so I migrated project to .com and now it should works fine as before.

@TemaSM TemaSM merged commit aeb2d62 into RealSpeaker:master Jan 30, 2021
@wojpawlik
Copy link

Please release this on npm. Until then, I cannot recommend this library in Telegraf 4's readme.

@TemaSM
Copy link
Contributor

TemaSM commented Feb 6, 2021

@wojpawlik gonna release in 2 hours. How do you think, do we need to bump minor version to 2.1.0, or simply bump patch version to 2.0.1?

@EdJoPaTo
Copy link
Collaborator Author

EdJoPaTo commented Feb 6, 2021

Technically there is nothing new, just some fixes for the typings. So in that way, its a bugfix release.

Practically its a feature (which is not breaking) so I would tend to a minor version bump.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants