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

enhance(misskey-js): miauth checkの型を追加 #14885

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

Conversation

kakkokari-gtyih
Copy link
Contributor

@kakkokari-gtyih kakkokari-gtyih commented Nov 3, 2024

What

書いてあるとおり
API /miauth/{sessionId}/check の型を追加した

Why

Additional info (optional)

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md
  • (If possible) Add tests

Copy link

codecov bot commented Nov 3, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 41.18%. Comparing base (6718a54) to head (a4e4799).
Report is 3 commits behind head on develop.

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.
📢 Have feedback on the report? Share it here.

@kakkokari-gtyih kakkokari-gtyih changed the title fix(misskey-js): miauth checkの型を追加 enhance(misskey-js): miauth checkの型を追加 Nov 3, 2024
@kakkokari-gtyih
Copy link
Contributor Author

バックエンドのCIが落ちてるけど関連しそうな場所は一切変更してないので謎

Comment on lines 109 to 112
[ep: `miauth/${string}/check`]: {
req: EmptyRequest;
res: MiAuthCheckResponse;
}
Copy link
Member

@anatawa12 anatawa12 Nov 3, 2024

Choose a reason for hiding this comment

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

ここ消して治るならOverwriteの検査でtypescriptの回数制限に達した可能性あるかも(よく知らないけどなんか型チェックが一定の複雑度を超えると打ち切りが発生することがあるらしい?よくわからないけど。メモ化とかの都合もあって色々複雑らしいというのを聞いたことがある)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

足してるだけのもの(自動生成で生成されないとわかっているもの)は単に|でつなげるようにすればいい?

Copy link
Member

@anatawa12 anatawa12 Nov 7, 2024

Choose a reason for hiding this comment

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

その方が良い可能性が高いと思います

Copy link
Member

Choose a reason for hiding this comment

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

あーでもSwitchCaseResponseTypeの適用はここじゃないのか。場合によっては変わらないかも

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Overwriteから出したけど変わらなかった

Copy link
Member

Choose a reason for hiding this comment

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

SwitchCaseResponseType側をいい感じにする必要がありそうか....

ちょっと調べてみる

Copy link
Member

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')にする事もできる。

Copy link
Member

@anatawa12 anatawa12 Nov 7, 2024

Choose a reason for hiding this comment

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

Copy link
Member

@anatawa12 anatawa12 Nov 8, 2024

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` みたいなのは動かなくなるかもしれないよって

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

Successfully merging this pull request may close these issues.

2 participants