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

フロントエンドのデータ取得の流れを整備 #476

Merged
merged 6 commits into from
Oct 17, 2024

Conversation

tosuke
Copy link
Collaborator

@tosuke tosuke commented Sep 17, 2024

  • connect でデータを取ってきて表示するまでの流れ(ルータでデータ取得する場合)を整備した
  • vitest でのテストの雛形を整備した
  • MSW と connect を連携させるためのユーティリティを整備した

これによりpnpm devで動かなくなる(エラーが表示される)が仕様(API がないので)

Copy link
Contributor

github-actions bot commented Sep 17, 2024

The latest Buf updates on your PR. Results from workflow Proto CI Pipeline / breaking (pull_request).

BuildFormatLintBreakingUpdated (UTC)
✅ passed✅ passed✅ passed✅ passedOct 17, 2024, 12:56 PM

Comment on lines +8 to +10
const transport = createConnectTransport({
baseUrl: "/api",
});
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

とりあえず/apiの下にバックエンドがありそこに Connect Protocol でアクセスするということにする

Copy link
Member

Choose a reason for hiding this comment

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

本番環境でもそのようにする予定
ワンチャン/connect/apiで分けるかも
(ファイル関係はConnectだとキツいので、一応OpenAPIを書いていて、それのパスを/apiとして切り分けるのであればConnectと混ぜるのは微妙かなぁという悩みがある)

Comment on lines +144 to +165
export const connect = {
rpc: <M extends DescMethod>(
method: M,
impl: MethodImpl<M>,
options?: ConnectHandlerOptions,
) =>
new ConnectHandler(
{ kind: method.kind, name: `${method.parent.typeName}/${method.name}` },
(router) => router.rpc(method, impl),
options,
),
service: <S extends DescService>(
service: S,
impl: ServiceImpl<S>,
options?: ConnectHandlerOptions,
) =>
new ConnectHandler(
{ kind: "service", name: service.typeName },
(router) => router.service(service, impl),
options,
),
};
Copy link
Collaborator Author

@tosuke tosuke Sep 17, 2024

Choose a reason for hiding this comment

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

[zatsu] なぜかなくて人々が困っている様子が見える(connectrpc/connect-es#825) のでライブラリとして外に切り出してもいいかもね

Copy link
Member

Choose a reason for hiding this comment

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

自分のリポジトリで公開して、それを利用する形にしてしまって良いのでは
ICTSCとしては全然OKですよ

<>
<AppShell>
<AppShell me={me}>
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

常にmeを引き回すかは怪しい。Context にするかも

Copy link
Member

Choose a reason for hiding this comment

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

データのライフサイクル管理・フェッチタイミング管理も視野に入れて決めてもらえればという感じ

Comment on lines +47 to +48
"@connectrpc/connect": "2.0.0-alpha.1",
"@connectrpc/connect-web": "2.0.0-alpha.1",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

これを実用する頃には beta にはなっているだろうという読み。なっててくれ

@tosuke tosuke marked this pull request as ready for review September 17, 2024 15:00
logica0419
logica0419 previously approved these changes Oct 9, 2024
Copy link
Member

@logica0419 logica0419 left a comment

Choose a reason for hiding this comment

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

フロントエンドにわかだけど、自分がみた感じ問題なさそうです
大感謝

問題とかの他のデータも合わさってきた時に、異なるライフサイクル・フェッチタイミングのデータが混在してくると思うから、それを統一的に管理できるとわかりやすくて嬉しいかも...という思いがあります
(そこに関してもお任せしますが)

@@ -36,3 +36,25 @@ jobs:

- name: Lint
run: pnpm run --recursive --parallel --aggregate-output lint
test:
Copy link
Member

Choose a reason for hiding this comment

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

自分の中ではフロントエンドのビルドもCIで確認するようなイメージがあるんだけど、それはなくても大丈夫?

Comment on lines +144 to +165
export const connect = {
rpc: <M extends DescMethod>(
method: M,
impl: MethodImpl<M>,
options?: ConnectHandlerOptions,
) =>
new ConnectHandler(
{ kind: method.kind, name: `${method.parent.typeName}/${method.name}` },
(router) => router.rpc(method, impl),
options,
),
service: <S extends DescService>(
service: S,
impl: ServiceImpl<S>,
options?: ConnectHandlerOptions,
) =>
new ConnectHandler(
{ kind: "service", name: service.typeName },
(router) => router.service(service, impl),
options,
),
};
Copy link
Member

Choose a reason for hiding this comment

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

自分のリポジトリで公開して、それを利用する形にしてしまって良いのでは
ICTSCとしては全然OKですよ

Comment on lines +8 to +10
const transport = createConnectTransport({
baseUrl: "/api",
});
Copy link
Member

Choose a reason for hiding this comment

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

本番環境でもそのようにする予定
ワンチャン/connect/apiで分けるかも
(ファイル関係はConnectだとキツいので、一応OpenAPIを書いていて、それのパスを/apiとして切り分けるのであればConnectと混ぜるのは微妙かなぁという悩みがある)

<>
<AppShell>
<AppShell me={me}>
Copy link
Member

Choose a reason for hiding this comment

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

データのライフサイクル管理・フェッチタイミング管理も視野に入れて決めてもらえればという感じ

@logica0419
Copy link
Member

モジュール関係のコンフリクト直したらセルフマージで大丈夫です
よろしく

@tosuke tosuke disabled auto-merge October 17, 2024 12:57
@tosuke tosuke merged commit 79bacfb into main Oct 17, 2024
16 checks passed
@tosuke tosuke deleted the frontend-connect branch October 17, 2024 12:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants