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

chore: add tiny definition for redis-lock #9971

Merged
merged 10 commits into from
Feb 22, 2023

Conversation

dojineko
Copy link
Contributor

What

ref: https://misskey.io/notes/9bbq6h1afw

src/core/AppLockService.tsredis-lock の型定義がない事に起因する型エラーを調整しました。

Why

Misskey が使用している箇所のみをひとまず解決したかったので @types に定義を追加しました。

constructor(
@Inject(DI.redis)
private redisClient: Redis.Redis,
) {
this.lock = promisify(redisLock(this.redisClient, retryDelay));
}

Additional info (optional)

@github-actions github-actions bot added the packages/backend Server side specific issue/PR label Feb 17, 2023
@codecov
Copy link

codecov bot commented Feb 17, 2023

Codecov Report

Merging #9971 (381c96a) into develop (980bf13) will decrease coverage by 1.25%.
The diff coverage is 0.00%.

❗ Current head 381c96a differs from pull request most recent head 4ed60a2. Consider uploading reports for the commit 4ed60a2 to get more accurate results

@@             Coverage Diff             @@
##           develop    #9971      +/-   ##
===========================================
- Coverage    24.39%   23.14%   -1.25%     
===========================================
  Files          700      700              
  Lines        64959    64793     -166     
  Branches      2241     1985     -256     
===========================================
- Hits         15844    14995     -849     
- Misses       49115    49798     +683     
Impacted Files Coverage Δ
packages/backend/src/@types/redis-lock.d.ts 0.00% <0.00%> (ø)
packages/backend/src/misc/truncate.ts 45.45% <0.00%> (-54.55%) ⬇️
.../backend/src/core/activitypub/ApAudienceService.ts 33.64% <0.00%> (-37.39%) ⬇️
...end/src/core/activitypub/models/ApPersonService.ts 26.29% <0.00%> (-36.16%) ⬇️
...kages/backend/src/core/FederatedInstanceService.ts 45.00% <0.00%> (-31.67%) ⬇️
...ckend/src/core/activitypub/models/ApNoteService.ts 24.42% <0.00%> (-30.03%) ⬇️
...nd/src/core/activitypub/models/ApMentionService.ts 68.29% <0.00%> (-26.83%) ⬇️
packages/backend/src/core/NoteCreateService.ts 23.18% <0.00%> (-25.43%) ⬇️
...d/src/core/activitypub/models/ApQuestionService.ts 37.50% <0.00%> (-23.22%) ⬇️
packages/backend/src/core/chart/charts/notes.ts 59.67% <0.00%> (-20.97%) ⬇️
... and 29 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

declare module 'redis-lock' {
import type Redis from 'ioredis';

type lock = (lockName: string, timeout?: number, taskToPerform?: function) => void;
Copy link
Member

Choose a reason for hiding this comment

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

  • 型エイリアスはパスカルケースで宣言されるべきです(すなわち Lock
  • function というプリミティブ型はなく (arg: type) => type というように引数と返値の型をそれぞれ指定する必要があります

Copy link
Contributor Author

Choose a reason for hiding this comment

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

レビューありがとうございますー

型エイリアスはパスカルケースで宣言されるべきです

ESLintに @typescript-eslint がいたので、 @typescript-eslint/naming-convention を設定し、typeLike を PascalCase とするルールを追記しました。同様のケースを防止できる見込みです。

https://github.com/typescript-eslint/typescript-eslint/blob/main/packages/eslint-plugin/docs/rules/naming-convention.md#group-selectors

(arg: type) => type というように引数と返値の型をそれぞれ指定する

function が unresolved で any 相当になっていたので redis-lock の実装で promisify している部分を参考に () => Promise<void> と改めてみました。doneで値をcallbackしていないのでおそらくこれで必要十分そうです。

https://github.com/errorception/redis-lock/blob/dcfa15f/index.js#L40-L50

redis-lock 1.0.0 で該当の仕組みがなくなっているのでバージョンが上がったら不要な定義になりそうですねー

packages/backend/src/@types/redis-lock.d.ts Outdated Show resolved Hide resolved
@saschanaz
Copy link
Member

saschanaz commented Feb 18, 2023

I filed errorception/redis-lock#36, perhaps it's more useful to add this directly to the package?

errorception/redis-lock#36 を登録しましたが、そのパッケージに直接寄与するのはどうでしょう。

@dojineko
Copy link
Contributor Author

@saschanaz

I think it is a very good suggestion.
I checked it, thanks for registering the Issue.

However, the version of redis-lock used by misskey is 0.1.4.
The current version of redis-lock is 1.0.0, and retroactively providing type definitions for older versions may affect the maintenance of redis-lock.

Therefore, assuming that misskey currently uses redis-lock 0.1.4, I would like to complete this addition of type definitions for redis-lock, and then realize the addition of type definitions for redis-lock 1.0.0 in redis-lock itself.

I am assuming that misskey will use redis-lock 1.0.0 if it goes in order, so I think that might be better as a result.


素晴らしい提案だと思います。
Issueの登録をいただきありがとうございます。

一方で misskey が使用している redis-lock のバージョンは 0.1.4 です。
現在の redis-lock の最新バージョンは 1.0.0 であり、古いバージョンに対して型定義を遡って提供することはもしかすると redis-lock のメンテナンスに影響を与えるかもしれません。

そのため、現状の misskey が redis-lock 0.1.4 を使う前提で、私は今回の redis-lock の型定義の追加を完了し、その上で redis-lock 1.0.0 用の型定義の追加を redis-lock 自体に実現しようと考えています。

私は misskey が順当に行けば redis-lock 1.0.0 を使用するようになることを想定しているため、結果的にその方が良いのかもしれないと考えています。

@dojineko
Copy link
Contributor Author

うう~む frontend の lint 落ちちゃうな。
naming-convention を適用する範囲を限定するかなにかしたほうが良さそうかしら。
image

ちょっとルール調整してみよう。

@dojineko
Copy link
Contributor Author

dojineko commented Feb 18, 2023

📝

  • frontend は naming-convention に関する Lint はパスするようになった
    • @typescript-eslint/prefer-nullish-coalescing などが見られるが元からのようなので置く
  • backend と sw は Lint をパスした

追記: sw のここ で naming-convention の検知対象が上がってるけど sw の eslint で通過してるっぽさがあった

> sw@ eslint /home/dojineko/misskey/packages/sw
> eslint --quiet src/**/*.ts

@@ -9,6 +9,7 @@ import { MetaService } from '@/core/MetaService.js';
import { bindThis } from '@/decorators.js';

// Defined also packages/sw/types.ts#L13
// eslint-disable-next-line @typescript-eslint/naming-convention
type pushNotificationsTypes = {
Copy link
Member

Choose a reason for hiding this comment

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

名前直した方がいい気もしますが👀

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ホントだ export してないから変えちゃって大丈夫そうですね。

core/activitypub/type.ts の1件だけ利用箇所見て検討しますねー

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • core/activitypub/type.ts の objIObject および ApObject に使用されている
  • 直接 obj が参照されている箇所は存在しないので名前を直すことにする
find . -type f -name '*.ts' -o -name '*.js' -o -name '*.vue' | grep -v node_modules | xargs grep -E 'type(\.[tj]s|)' | grep import | grep -w obj | wc -l
0

@dojineko
Copy link
Contributor Author

あっ acid-chicken さんがデフォルトに戻してくださってた すまねぇ・・・

@dojineko dojineko requested review from acid-chicken and saschanaz and removed request for acid-chicken and saschanaz February 18, 2023 18:01
@syuilo syuilo merged commit a6fb615 into misskey-dev:develop Feb 22, 2023
@syuilo
Copy link
Member

syuilo commented Feb 22, 2023

🙏🏻🙏🏻🙏🏻

@dojineko dojineko deleted the dts-redis-lock branch February 27, 2023 03:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/backend Server side specific issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants