フロントエンドからのGraphQLクエリを最適化した #134

vvakame · 2018-09-07T10:26:37Z

次のルールがあったほうがよさそうだなーと思ったので、それに沿って直してみている。

Queryを持っていいのは pages/ 配下のComponentのみ
- 狙い: 画面あたりのクエリ数を1にする
- それ以外のcontainersとかはFragmentのみ
Componentは直下の子Componentが要求するFragmentを自分のQuery or Fragmentに取り込む
- 狙い: Componentをツリー構造として管理するため
- 子孫の要求はFragmentとして集約できる
Componentは自分のJSX内で使うフィールドのみ要求する
- 狙い: 子のためのフィールド要求をしはじめると管理が難しくなるため
- filter を使うとより厳密にできそう(このPRではやらない
Fragmentの命名は (自分を使うComponent名)Fragment とする
- 狙い: Fragment名からComponentにたどり着きやすくするため
Componentが要求するクエリ範囲は可能な限り最小化する
- 狙い: 後から広くするのは簡単だが狭めるのは苦痛が大きいため
- TimetableRowFragment が参考になるかも
親Componentの型を参照しない
- 狙い: ツリー構造でComponentを管理する場合、関心事は子Componentのみで完結する
- 親Component関連の型を使ってしまうと責任範囲が明瞭でなく、わかりにくくリファクタリングが難しそうだと感じた

判断に悩む 🤔

単なる土管的なComponentにもいちいちFragmentを用意するべきか？
- 例: ContentSection は Top と ContentGrid を繋いでいるが、ContentSectionFragmentを定義するべきか？
- Topの実装を見た時、なぜ ContentGridFragment が必要なのかパット見で判断できない
- ContentSectionのコードにContentGridFragmentの型定義が出てきて微妙
  - 孫の型が出てくるのはかなり難解…
- 僕の意見: いちいちFragment定義すればいいと思う
- 現状の実装: 子孫Fragmentを直接参照している
  - VSCode上で該当子孫にジャンプはできる
  - 間に何が挟まっているのかは自明ではない←辛い気もする

vvakame · 2018-09-07T10:29:00Z

web/containers/Top/TimetableSection/TimetableRow.tsx

+} from '../../../graphql/generated/TimetableRowFragment';
+
+export const TIMETABLE_ROW_FRAGMENT = gql`
+  fragment TimetableRowFragment on SessionConnection {


ここで on Query として sessionList { ... } としなかったのは、取得件数の指定が上位Componentの責務であると考えたため。
そうすると、このComponentの責任範囲としては sessionList の返り値である SessionConnection に対する要求から書き始めるのが正しいはず。

細かいですがConnectionはページングの責務も含んでるんで、このRowに対しては Session そのものが責務として合ってるのかなと思いました

わかる。しかしsessionListをフィルタして妥当なデータを見つける処理がここに入っている構造上仕方ないのだ…(その構造が悪いのでリファクタリングしたほうがよさはある

vvakame · 2018-09-07T10:30:20Z

web/containers/Top/TimetableSection/index.tsx

+
+export const TIMETABLE_SECTION_FRAGMENT = gql`
+  fragment TimetableSectionFragment on Query {
+    sessionList {


ここで件数を指定したりできます。
将来的に ContentGrid と件数がズレた場合、aliasを設定する必要があるかも(めんどいね

これはQueryの関数（？）自体をFragmentにしてるときに起きるケースとして面白いですね！！
今はいい考えが湧いてこないけどこういうことも起こるんだなぁなるほどなるほど

vvakame · 2018-09-07T10:31:09Z

web/package.json

@@ -9,7 +9,7 @@
    "lint": "tslint -c tslint.json -p tsconfig.json",
    "lint:fix": "npm run lint -- --fix",
    "precommit": "lint-staged",
-    "codegen": "apollo codegen:generate graphql/generated --schema ../schema.graphql --addTypename --target typescript --outputFlat"
+    "codegen": "rm -rf graphql/generated && apollo codegen:generate graphql/generated --schema ../schema.graphql --addTypename --target typescript --outputFlat"


rm -rf しておかないと、消したFragmentやらのファイルが残ってて意図せずコンパイルが通ってしまう場合があったので型定義を生成するたびに一回消すようにしました。

sawa-zen

めっちゃ勉強なりました 🙇 🙇

adwd

知見が得られるコードレビューで面白かったです 😂

adwd · 2018-09-07T14:22:28Z

web/pages/index.tsx

+                  <NewsSection data={data} />
+                  <AboutSection />
+                  <ContentSection data={data} />
+                  <StyledTimetableSection data={data} />


ここはdataがまるごと渡ってて違和感あるような。
でも子コンポーネントのPropsを見ると自分のFragmentになってて型としてはOKってことなんすね〜。
graphql-anywhereのfilterはこういうときに使うのかな？

まぁまぁわかる

adwd · 2018-09-07T14:27:33Z

web/containers/Top/NewsSection/index.tsx


-const NewsSection: React.SFC = ({ ...props }) => (
+interface Props {
+  data: NewsListFragment;


💭 一瞬ん？ってなったのはこれがバケツリレーしてるだけのコンポーネントだからか。。なるほどなるほど

adwd · 2018-09-07T14:40:31Z

web/containers/Top/NewsSection/NewsList.tsx

+  }
+`;
+
+const NewsList: React.SFC<Props> = ({ ...props }) => (


nit: ({ data, ...props }) もしくは ({ data: { newsList }, ...props }) すればよいかも

まかせた！(なげやり

adwd · 2018-09-07T14:45:26Z

web/containers/Top/TimetableSection/TimetableRow.tsx

+} from '../../../graphql/generated/TimetableRowFragment';
+
+export const TIMETABLE_ROW_FRAGMENT = gql`
+  fragment TimetableRowFragment on SessionConnection {


細かいですがConnectionはページングの責務も含んでるんで、このRowに対しては Session そのものが責務として合ってるのかなと思いました

adwd · 2018-09-07T14:48:35Z

web/containers/Top/TimetableSection/index.tsx

+
+export const TIMETABLE_SECTION_FRAGMENT = gql`
+  fragment TimetableSectionFragment on Query {
+    sessionList {


これはQueryの関数（？）自体をFragmentにしてるときに起きるケースとして面白いですね！！
今はいい考えが湧いてこないけどこういうことも起こるんだなぁなるほどなるほど

adwd · 2018-09-07T14:51:08Z

Componentは直下の子Componentが要求するFragmentを自分のQuery or Fragmentに取り込む

これはFragmentを使う上でやらないとデータが子に渡らないのでルールと言うか必須なのでは？ 🤔

単なる土管的なComponentにもいちいちFragmentを用意するべきか？

ContentSectionでいうと、現状のComponent分割構成を見直すべきなのかも？と思った。 Section でラップするだけの責務を負っているのが ContentSection でその関係でいうと pages/index.tsx に

<Section title="NEWS" id="news">
  <News data={filter(NewsFragment, data))} />
</Section>
<Section title="ABOUT" id="about">
  <About />
</Section>
<Section title="CONTENTS" id="contents">
  <Contents data={filter(ContentsFragment, data)} />
</Section>
<Section title="TIME TABLE" id="time_table">
  <TimeTable data={filter(TimeTableFragment, data)} />
</Section>

こうなってるとか。孫Fragment問題はやらない案に同意です。

vvakame added 3 commits September 7, 2018 17:54

GraphQLクエリを利用するcomponentに近い位置に移した

4d5f09d

NewsQueryをNewsListFragmentに変更

79aa5c2

QueryやFragmentをリファクタリングしてどこで何を必要としているかわかりやすくした

f01a3dc

vvakame requested review from sawa-zen and adwd September 7, 2018 10:26

vvakame commented Sep 7, 2018

View reviewed changes

sawa-zen approved these changes Sep 7, 2018

View reviewed changes

adwd approved these changes Sep 7, 2018

View reviewed changes

vvakame merged commit 402cba7 into develop Sep 8, 2018

vvakame deleted the refactor-frontgql branch September 8, 2018 01:08

kazegusuri mentioned this pull request Sep 10, 2018

Revert "フロントエンドからのGraphQLクエリを最適化した" #140

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

フロントエンドからのGraphQLクエリを最適化した #134

フロントエンドからのGraphQLクエリを最適化した #134

vvakame commented Sep 7, 2018

vvakame Sep 7, 2018

adwd Sep 7, 2018

vvakame Sep 8, 2018

vvakame Sep 7, 2018

adwd Sep 7, 2018

vvakame Sep 7, 2018

sawa-zen left a comment

adwd left a comment

adwd Sep 7, 2018

vvakame Sep 8, 2018

adwd Sep 7, 2018

adwd Sep 7, 2018

vvakame Sep 8, 2018

adwd Sep 7, 2018

adwd Sep 7, 2018

adwd commented Sep 7, 2018 •

edited

Loading

フロントエンドからのGraphQLクエリを最適化した #134

フロントエンドからのGraphQLクエリを最適化した #134

Conversation

vvakame commented Sep 7, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sawa-zen left a comment

Choose a reason for hiding this comment

adwd left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adwd commented Sep 7, 2018 • edited Loading

adwd commented Sep 7, 2018 •

edited

Loading