Skip to content

chore: add tiny definition for redis-lock - #9971

Merged
syuilo merged 10 commits into
misskey-dev:developfrom
dojineko:dts-redis-lock
Feb 22, 2023
Merged

chore: add tiny definition for redis-lock#9971
syuilo merged 10 commits into
misskey-dev:developfrom
dojineko:dts-redis-lock

Conversation

@dojineko

Copy link
Copy Markdown
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

codecov Bot commented Feb 17, 2023

Copy link
Copy Markdown

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
Copy Markdown
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
Copy Markdown
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 で該当の仕組みがなくなっているのでバージョンが上がったら不要な定義になりそうですねー

Comment thread packages/backend/src/@types/redis-lock.d.ts Outdated
@saschanaz

saschanaz commented Feb 18, 2023

Copy link
Copy Markdown
Member

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
Copy Markdown
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
Copy Markdown
Contributor Author

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

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

@dojineko

dojineko commented Feb 18, 2023

Copy link
Copy Markdown
Contributor Author

📝

  • 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


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

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

syuilo commented Feb 22, 2023

Copy link
Copy Markdown
Member

🙏🏻🙏🏻🙏🏻

@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