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 to TypeScript and Telegraf v4 #40

Open
wants to merge 44 commits into
base: develop
Choose a base branch
from

Conversation

EdJoPaTo
Copy link
Contributor

@EdJoPaTo EdJoPaTo commented Oct 7, 2020

This Pull Request refactors the code to TypeScript.
This has the benefit of not having separate typings which are maybe wrong. This should also improve code readability and maintainability.

While this should be a simple refactoring in general I decided to introduce two changes to simplify code and have more strict typings, one change is breaking.

The first change is the internal structure of the locale repositories. Before this change the Repository is structured with the object tree structure. When the resourceKey a.b.c in language en is requested the following value is read: {en: {a: {b: {c: value}}}}.
With this change the dot notation is already applied when loading into the repository and not every time a key is used. This results in a simpler structure of the repository: {en: {'a.b.c': value}}. Having that simplifies internal typings (and has an insignificant performance benefit). This change is not breaking and shouldn't be noticed by users.

The breaking change is the way to import/require and use this library.
Before this you could simply use any name you liked and use that:

const whatever = require('telegraf-i18n')
const i18n = new whatever()

With these changes you always have to use it like this:

const {I18n} = require('telegraf-i18n') // CommonJS
import {I18n} from 'telegraf-i18n'      // ES6 modules

const i18n = new I18n()

Allowing for the default import is not as easy with ES6 and CommonJS style imports and having the I18n class and functions like reply. After some testing it wasnt worth my time: There is a way to use it and its not hard to adapt to that.
If someone has some suggestions how to solve that elegantly: I'm curious :)

As NodeJS 8 is not supported anymore and telegraf now requiring NodeJS 12 this is also another breaking change. But as you need NodeJS 12 or higher for telegraf anyway users probably wont notice.

fixes #30
fixes #33
fixes #35
fixes #36
fixes #37
fixes #38

Feel free to comment on the changes. I'm sure someone can spot something to improve or has better ideas how to do something.

I tested the changes in multiple of my TypeScript Bots. I do not have any running JavaScript bot left so it might be nice if someone else is willing to test on that. Especially If the import works well.

BREAKING CHANGES:
`const I18n = require()`
has to be rectored to
`const {I18n} = require()`

NodeJS 10 required now
The method loads the template for this resourceKey.
Not having the resourceKey then seems useless.

BREAKING CHANGE: resourceKey requires resourceKey
If someone has a reason why thats needed it can be added again.
@EdJoPaTo
Copy link
Contributor Author

This Pull Request is now available as @edjopato/telegraf-i18n on npm.

If you test it please report back to this Pull Request (like approving it) to increase confidence in a good end result.

EdJoPaTo added a commit to EdJoPaTo/telegram-typescript-bot-template that referenced this pull request Nov 8, 2020
@EdJoPaTo EdJoPaTo changed the title Refactor to TypeScript Refactor to TypeScript and Telegraf v4 Jan 11, 2021
@aqemi
Copy link

aqemi commented Jan 30, 2021

@EdJoPaTo Using the following expression:

this.bot.hears(match('subscribe'), ctx => this.subscribe(ctx));

throws TS error:

Error TS2345: Argument of type '(text: string, ctx: TelegrafContextWithI18n) => string[] | null' is not assignable to parameter of type 'MaybeArray<string | RegExp | ((value: string, ctx: TelegrafContext) => RegExpExecArray | null)>'.
  Type '(text: string, ctx: TelegrafContextWithI18n) => string[] | null' is not assignable to type '(value: string, ctx: TelegrafContext) => RegExpExecArray | null'.
    Type 'string[] | null' is not assignable to type 'RegExpExecArray | null'.
      Type 'string[]' is missing the following properties from type 'RegExpExecArray': index, input

Using "telegraf": "^4.0.2" and "@edjopato/telegraf-i18n": "^0.2.0"

@EdJoPaTo
Copy link
Contributor Author

EdJoPaTo commented Feb 3, 2021

@svmn nice catch. I kinda bodged it so it works. Its released in @edjopato/telegraf-i18n v0.2.1. Thanks for reporting!

EdJoPaTo and others added 5 commits February 3, 2021 13:27
Bumps [xo](https://github.com/xojs/xo) from 0.37.1 to 0.38.1.
- [Release notes](https://github.com/xojs/xo/releases)
- [Commits](xojs/xo@v0.37.1...v0.38.1)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
use typegram directly
dependabot bot and others added 19 commits May 1, 2021 10:11
Bumps [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node) from 14.14.43 to 15.0.1.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [ts-node](https://github.com/TypeStrong/ts-node) from 9.1.1 to 10.0.0.
- [Release notes](https://github.com/TypeStrong/ts-node/releases)
- [Commits](TypeStrong/ts-node@v9.1.1...v10.0.0)

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [del-cli](https://github.com/sindresorhus/del-cli) from 3.0.1 to 4.0.0.
- [Release notes](https://github.com/sindresorhus/del-cli/releases)
- [Commits](sindresorhus/del-cli@v3.0.1...v4.0.0)

---
updated-dependencies:
- dependency-name: del-cli
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node) from 15.14.1 to 16.0.0.
- [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
- [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node)

---
updated-dependencies:
- dependency-name: "@types/node"
  dependency-type: direct:development
  update-type: version-update:semver-major
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Bumps [xo](https://github.com/xojs/xo) from 0.41.0 to 0.42.0.
- [Release notes](https://github.com/xojs/xo/releases)
- [Commits](xojs/xo@v0.41.0...v0.42.0)

---
updated-dependencies:
- dependency-name: xo
  dependency-type: direct:development
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
BREAKING CHANGE: this removes the helper match and reply.
They arnt used that often anyway and would have made compatibilty with
grammY harder.
@EdJoPaTo
Copy link
Contributor Author

EdJoPaTo commented Aug 4, 2021

This pull request is now available as https://github.com/grammyjs/i18n and works with both grammY and Telegraf.

I will leave it open so its easier to see for everyone coming by.

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