-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Conversation
Codecov Report
@@ 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
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; |
There was a problem hiding this comment.
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
というように引数と返値の型をそれぞれ指定する必要があります
There was a problem hiding this comment.
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 とするルールを追記しました。同様のケースを防止できる見込みです。
(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 で該当の仕組みがなくなっているのでバージョンが上がったら不要な定義になりそうですねー
Co-authored-by: Acid Chicken (硫酸鶏) <[email protected]>
I filed errorception/redis-lock#36, perhaps it's more useful to add this directly to the package? errorception/redis-lock#36 を登録しましたが、そのパッケージに直接寄与するのはどうでしょう。 |
I think it is a very good suggestion. However, the version of redis-lock used by misskey is 0.1.4. 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. 素晴らしい提案だと思います。 一方で misskey が使用している redis-lock のバージョンは 0.1.4 です。 そのため、現状の misskey が redis-lock 0.1.4 を使う前提で、私は今回の redis-lock の型定義の追加を完了し、その上で redis-lock 1.0.0 用の型定義の追加を redis-lock 自体に実現しようと考えています。 私は misskey が順当に行けば redis-lock 1.0.0 を使用するようになることを想定しているため、結果的にその方が良いのかもしれないと考えています。 |
📝
追記: 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 = { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
名前直した方がいい気もしますが👀
There was a problem hiding this comment.
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件だけ利用箇所見て検討しますねー
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- core/activitypub/type.ts の
obj
はIObject
および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
あっ acid-chicken さんがデフォルトに戻してくださってた すまねぇ・・・ |
🙏🏻🙏🏻🙏🏻 |
What
ref: https://misskey.io/notes/9bbq6h1afw
src/core/AppLockService.ts
のredis-lock
の型定義がない事に起因する型エラーを調整しました。Why
Misskey が使用している箇所のみをひとまず解決したかったので
@types
に定義を追加しました。misskey/packages/backend/src/core/AppLockService.ts
Lines 17 to 22 in 9ce13d4
Additional info (optional)