search

watanavex / flutter_github_search

12 stars 1 forks source link

monoレビュー 🐶 #13

Open mono0926 opened 2 years ago

mono0926 commented 2 years ago

全体的に良い感じだと思いました 👍 ✨

Slackコメントに返答

画面の状態はStateNotifierでまるっと提供

関連する状態はStateNotifierでまとめるべきですが、直接関連しない状態は適切な小さめの粒度で分けるのが良いです。 https://github.com/mono0926/flutter_github_search/commit/1bf39adcc31f3adbd2f9655a122f0d650a38f151 と https://github.com/mono0926/flutter_github_search/commit/081d230bd70791493cd2bea8a6e62b9e6b6b4b27 のようにするのが良いと思います。

Page単位でまとめるより、関連する状態の単位でまとめる方が組みやすいはずです。 Pageまたがったり複数箇所で必要な状態もありますし。

詳細画面は↑をやるとオーバーエンジニアリングに感じたのでFutureProviderで ただし、規模の大きなアプリになったら統一させた方がいいんかな…

基本的には適切な種類のProviderを過不足なく組み合わせるのが良いと思っています。 アプリ自体の規模というより、メンバーの統制的に多少冗長なコードになったとしても何かしらの分かりやすい方針提示して画一的なコードにしたいとかなら、仕方なくStateNotifierProviderに寄せるとかありかもしれませんが。

その他気になった点箇条書き

全体的に良い感じな前提で、気になったところのみ列挙している感じです 🙏
※ 好みによるところや、僕が誤っているところが含まれている可能性もあります。

  • buildをメソッドで分割している箇所は、Widget分割に統一がベター(あるいは読みにくくない範囲でベタ書きで済ませると良い)

例えば、 https://github.com/watanavex/flutter_github_search/blob/dac08ff7a104ac9278ce02b4b13e83b5d4a799c3/lib/ui/page/search/search_page.dart#L23 は分断されているより、以下のように書かれている方がそのまま一目で読めてむしろ読みやすいと思います。 (もちろん全てベタ書きだとカオスになるので、各種要素を適切にWidget分割することとセット)

  @override
  Widget build(BuildContext context, WidgetRef ref) {
    final searchState = ref.watch(
        searchPageStateNotifierProvider.select((value) => value.searchState));
    return Scaffold(
      appBar: _SearchAppBar(),
      body: searchState.when(
        uninitialized: () {
          return Container();
        },
        searching: () {
          return const LoadingView();
        },
        success: (repositories, query, page, haxNext) {
          return _buildListView(context, ref, repositories, haxNext);
        },
        fetchingNext: (repositories, query, page) {
          return _buildListView(context, ref, repositories, true);
        },
        fail: () {
          return const ErrorView();
        },
        empty: () {
          return const ErrorView(
            message: '検索結果は0件です',
          );
        },
      ),
    );
  }
watanavex commented 2 years ago

@mono0926

Page単位でまとめるより、関連する状態の単位でまとめる方が組みやすいはずです。 Pageまたがったり複数箇所で必要な状態もありますし。

buildをメソッドで分割している箇所は、Widget分割に統一がベター(あるいは読みにくくない範囲でベタ書きで済ませると良い)

上記2点について、半信半疑ではあったものの feature/apply-feedback で実装してみたところ、非常にしっくり来ました。 コードの変更にも強そうです。。。ありがとうございます。

.freezed.dart・.g.dartなどは、コミットに含めた方が諸々利便性高く、目立ったデメリットもなく、個人的にはそうしている

このメリットはまだ理解できず...

API_TOKEN の件については、デバッグ用途と言う意図でした (READMEに書いておけば良かったです🙇‍♂️) VPN環境などで、API rate limitに達し動作確認ができない場合などがよくあるので、 flutter run --debug --dart-define=API_TOKEN=$GITHUB_ACCESS_TOKEN っと実行して動作確認をしてもらうことを想定していました🙇‍♂️

それ以外は実際に手を動かしてみて、全て納得感のあるものでした。 丁寧なフィードバックありがとうございました！

mono0926 commented 2 years ago

@watanavex

このメリットはまだ理解できず...

このあたりです:

watanavex commented 2 years ago

@mono0926 なるほど 🤔

私は

のですが、

パッケージが上がった時のコード生成結果の差分や異変に気付ける(普段あまり見ないもののたまに変な時にチェックすることはあります)

このあたりはメリットに感じました。 ありがとうございます！

mono0926 commented 2 years ago

@watanavex

git worktreeを常用しているためブランチ変更の影響を受けない

ただ、その場合でも初回はbuild_runner必要ですよね？

コード量多くなると実行に数分かかることも多く、その時間を最小化できるに越したことはないと思います。 トレードオフがあるならともかく、デメリット感じたことないので個人的にはいつも迷うことなくコミットに含めてます。

mono0926 commented 2 years ago

pub get と build_runner と IDE開く一連の操作をコマンド化している

こういうプロジェクトや個人での工夫無しに、デフォルトで何も考えずにそのまま実行できるに越したことないと思います。

watanavex commented 2 years ago

@mono0926

ただ、その場合でも初回はbuild_runner必要ですよね？

ですです。必要です。

こういうプロジェクトや個人での工夫無しに、デフォルトで何も考えずにそのまま実行できるに越したことないと思います。

それはもう、おっしゃる通りですね。

自動生成コードをリポジトリに含めるのはアンチパターンとしてきたので若干抵抗はありますが 😅 次のプロジェクトでは試してみたい と思いました 💭

mono0926 commented 2 years ago

@watanavex

自動生成コードをリポジトリに含めるのはアンチパターンとしてきたので若干抵抗はありますが 😅

生成すれば同じ成果物得られる場合は元ソースのみ管理するというのがセオリーではありますが、絶対に従うべき指針でもなく、かつケースバイケースで一部含めるのもアンチパターンではないと思います。

iOSのCocoaPodsも、成果物をコミットしてもどちらでも良い(ややコミット推奨)というのが公式の見解です。 https://qiita.com/mono0926/items/636819c42e96a8c4e12d#podsディレクトリはバージョン管理に含めるべきか

freezedでもコミットしてますね: https://github.com/rrousselGit/freezed/tree/master/packages/freezed/example/lib

ただ、Riverpodはgitignoreしてますね。 https://github.com/rrousselGit/river_pod/blob/5f6616f6b76596be3a7b50559376348b439cbc2f/.gitignore#L3

確かにコメントに書いてある通り、コンフリクトは少しだけ煩わしいかもしれませんが、後勝ち・再生成で解決するだけなので、ネックに感じたことなかったです🤔 (多数が関わるOSSだと煩わしいのかもしれません🤔)

nerd0geek1 commented 2 years ago

@mono0926 一連のレビューありがとうございました、傍から読んでいても勉強になりました。

アプリにソースコードとして含まれている場合についても、dart-defineで渡した場合も読めてしまう、というのは知りませんでした(dart-defineは大丈夫だと勘違いしていた) https://medium.com/@rondalal54/reverse-engineering-flutter-apps-5d620bb105c0 https://twitter.com/OrestesGaolin/status/1326550096540889088?s=20&t=7AIUjLXV0ctFUOabTKLWdA

仮にどうしてもモバイル側でAPI Keyを保持する必要がある場合、Firebase Remote Configを使うのが一般的でしょうか？ それとも、他に良い方法があるのでしょうか？

mono0926 commented 2 years ago

@nerd0geek1

こういう認識です。

仮にどうしてもモバイル側でAPI Keyを保持する必要がある場合、Firebase Remote Configを使うのが一般的でしょうか？

Firebase Remote Configなどでもその値を利用する処理・キャッシュ・通信経路などから、多少苦労しながらも値を入手できてしまうはずです。

値を完全に隠したい場合、その処理をクライアントサイドでやるのではなく、キー情報を置いたサーバーサイド経由にするしか無いはずです。

一般的なAPI Keyは漏れてもあまり問題ない前提で、想定外の利用(アプリ経由ではなくスクリプトでの実行など)をなるべく抑えるために、特定のアプリIDからしか使えないような制限をKeyに設定したりしますね。

API Keyではなく秘密鍵系は普通絶対に漏れてはいけないものなので、そういう工夫をしてもクライアントサイドに渡すべきではなく、先述の通り外部から絶対にアクセスできないようにサーバーサイドに隠蔽するべきなはずです。

nerd0geek1 commented 2 years ago

確かに、おっしゃる通りです。。

ありがとうございます！すっきり疑問を解決できました。