-
-
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
enhance(misskey-js): miauth checkの型を追加 #14885
base: develop
Are you sure you want to change the base?
enhance(misskey-js): miauth checkの型を追加 #14885
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #14885 +/- ##
===========================================
+ Coverage 39.47% 41.18% +1.70%
===========================================
Files 1559 1563 +4
Lines 196948 202717 +5769
Branches 3563 2598 -965
===========================================
+ Hits 77750 83481 +5731
- Misses 118592 118664 +72
+ Partials 606 572 -34 ☔ View full report in Codecov by Sentry. |
バックエンドのCIが落ちてるけど関連しそうな場所は一切変更してないので謎 |
packages/misskey-js/src/api.types.ts
Outdated
[ep: `miauth/${string}/check`]: { | ||
req: EmptyRequest; | ||
res: MiAuthCheckResponse; | ||
} |
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.
ここ消して治るならOverwrite
の検査でtypescriptの回数制限に達した可能性あるかも(よく知らないけどなんか型チェックが一定の複雑度を超えると打ち切りが発生することがあるらしい?よくわからないけど。メモ化とかの都合もあって色々複雑らしいというのを聞いたことがある)
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.
その方が良い可能性が高いと思います
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.
あーでもSwitchCaseResponseTypeの適用はここじゃないのか。場合によっては変わらないかも
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.
Overwriteから出したけど変わらなかった
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.
SwitchCaseResponseType側をいい感じにする必要がありそうか....
ちょっと調べてみる
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.
ちょっといろいろ調べた感じ、複雑性は関係なかった。
miauth/${string}/check
が${string}timeline
(timeline testで使われてるタイムラインエンドポイントの表現)とマッチする扱いをtypescript compilerに受けて、戻り値の方がcomponents['schemas']['Note'][] | MiAuthCheckResponse
になる扱いを受けてるっぽいので、miauth/${string}/check
の${string}
をいい感じに変えるといいかもしれないけどmiauthの仕様的には変えられないか。
またはテスト側の型の宣言を('antennas/notes' | 'notes/global-timeline' | 'notes/timeline' | 'notes/hybrid-timeline' | 'notes/local-timeline' | 'roles/notes' | 'notes/search-by-tag' | 'notes/user-list-timeline')
にする事もできる。
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.
もう少し調べてみたら代入チェックではちゃんと互換性ないと判断されるのにContainer[Key]
では互換性があるって認識されるっぽい。
一貫性の無い動作ではある気がするしtypescriptにバグ報告しても良い可能性はあるかも(修正されるかは置いといて)
追記: templete literal typeのintersection typeがneverにならないのがroot causeっぽい気がしました
https://www.typescriptlang.org/play/?#code/GYVwdgxgLglg9mABGApgNxQJwBQEoBcy6WiA3olABaZwDuyIANowNyIC+AUJxAgM5QKKAQFZCAAwC2MAIYgqAegAkpAZhhgA5uwURKKCAGtxiAGSJxKtRu0LYklIw0oTAXiIYcuFpwULEAQEAegD8nEA
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.
typescriptの方針的に`miauth/${string}/check` & `${string}timeline`
を never にするかは不透明なので、
'antennas/notes' | 'notes/global-timeline' | 'notes/timeline' | 'notes/hybrid-timeline' | 'notes/local-timeline' | 'roles/notes' | 'notes/search-by-tag' | 'notes/user-list-timeline'
に書き換えたほうが良さそう。
また、Notesとしてmisskey jsのchangesとして書いておいたほうが良さそう。keyof Misskey.Endpoints & `${string}timeline`
みたいなのは動かなくなるかもしれないよって
What
書いてあるとおり
API
/miauth/{sessionId}/check
の型を追加したWhy
Additional info (optional)
Checklist