Skip to content
This repository has been archived by the owner on Jun 1, 2023. It is now read-only.

JSON ファイルと Vue の間に中間層を設ける #6102

Conversation

nard-tech
Copy link
Contributor

@nard-tech nard-tech commented Mar 14, 2021

👏 解決する issue / Resolved Issues

📝 関連する issue / Related Issues

背景

#3008 は 2020年4 - 5月のもので,そのまま merge することはできないため,#3008 を引き継ぐ形でこの PR を作成した.

⛏ 変更内容 / Details of Changes

  • JSONファイルの型や変換コードを quicktype で自動生成し,libraries/auto_generated/data_converter 以下に配置

📸 スクリーンショット / Screenshots

見た目の変更はなし


issueの関連付けを修正 — @MaySoMusician (2021/03/14 22:15)

hikyaru-suzuki and others added 19 commits April 8, 2020 15:40
…ro-Gov#1100-create-repository-layers

Conflicts:
  README.md
  components/AgencyBarChart.vue
  components/DataView.vue
  components/MetroBarChart.vue
  components/PageHeader.vue
  components/TimeBarChart.vue
  components/TimeStackedBarChart.vue
  components/VisitorsBarChart.vue
  components/WhatsNew.vue
  components/cards/AgencyCard.vue
  components/cards/ChiyodaVisitorsCard.vue
  components/cards/ConfirmedCasesAttributesCard.vue
  components/cards/ConfirmedCasesDetailsCard.vue
  components/cards/ConfirmedCasesNumberCard.vue
  components/cards/ConsultationDeskReportsNumberCard.vue
  components/cards/InspectionPersonsNumberCard.vue
  components/cards/MetroCard.vue
  components/cards/ShinjukuVisitorsCard.vue
  components/cards/TelephoneAdvisoryReportsNumberCard.vue
  components/cards/TestedCasesDetailsCard.vue
  components/cards/TestedNumberCard.vue
  components/flow/FlowSpElder.vue
  components/flow/FlowSpGeneral.vue
  components/flow/FlowSpPast.vue
  components/flow/FlowSpSuspect.vue
  layouts/default.vue
  libraries/utils/formatGraph.ts
  libraries/utils/formatTable.ts
  nuxt.config.ts
  package.json
  pages/cards/_card.vue
  pages/index.vue
  utils/formatConfirmedCases.ts
  utils/formatTestedCases.ts
  yarn.lock
@nard-tech
Copy link
Contributor Author

nard-tech commented Mar 14, 2021

#3008 の commit との対応関係

@nard-tech nard-tech force-pushed the feature/#1100-create-repository-layers branch 4 times, most recently from 6f430c5 to 4896d56 Compare March 14, 2021 13:08
@nard-tech nard-tech marked this pull request as draft March 14, 2021 13:10
@nard-tech nard-tech force-pushed the feature/#1100-create-repository-layers branch from 4896d56 to 217199a Compare March 14, 2021 13:49
@nard-tech
Copy link
Contributor Author

@MaySoMusician

quicktype で自動生成されたファイルについては、冒頭に「自動生成されたファイルであり、直接編集してはいけない」みたいな(ちょうど yarn.lock の冒頭にあるようなやつ)注意文を付け足すことは可能なのでしょうか?

直接編集することで注意文を追加し,後に更新するときは注意文が削除されないように,更新された行だけ git で add, commit すればよいと思いますが,もっと仕組み化したいところです.

もし quicktype のオプションなどで独自の文言を追加できそうならやっておきたいところです

ちょっと厳しそうですので,GitHub Actions で danger を回すとよいと思います.
libraries/auto_generated/data_converter 以下のファイルが更新された場合は pull request 上に警告を出すことができます.

@MaySoMusician
Copy link
Contributor

@MaySoMusician

statuses() に対する型アノテーションなどは一律不要になります

確かにそうなのですが,エディタ上で

  computed: {
    statuses(): IInfectionMedicalCareProvisionStatusData {
      return this.infectionMedicalCareProvisionStatus.data

IInfectionMedicalCareProvisionStatusData の部分から直接 libraries/auto_generated/data_converter 以下に書かれている interface の定義に飛べる利便性を考慮してこのようにしています.

(略)

string のようなプリミティブな型については書かなくてもよいと思いましたが,全体の統一性を考慮し,すべてに型を付けています.

@nard-tech 確かに利便性はありますが、 type Computed = の方で指定している型と computed 内のアノテーションの型で同じ型を2度書くのは冗長だと思います。片方を更新し忘れるとTSエラーこそ出ますが(Vetur導入時)、エラーメッセージもあまり直感的ではない(原理的には正しいメッセージなのですが)のでちょっと大変そうです。VSCodeなら type Computed = の方の型アノテーションから定義に飛べますが、それだとまずいですかね?

@MaySoMusician
Copy link
Contributor

#6102 (comment) の件以外については、僕としては問題ないと思います。
@nard-tech お手数ですがコンフリクトの解決をしていただけると助かります

@nard-tech
Copy link
Contributor Author

nard-tech commented Apr 18, 2021

@MaySoMusician conflict は解消しました (e1c56a2)

エラーメッセージもあまり直感的ではない(原理的には正しいメッセージなのですが)

スクリーンショット 2021-04-18 19 54 01

上の画像のようなメッセージのことを仰っているのだと思いますが,直感的でないことはいずれにしろ変わらなさそうです.

型については,わりと好みの問題だと思っていて,Java のようなガチガチの型アノテーションが必要な言語に慣れていると,TypeScript であっても冗長なのを承知の上でしっかり型を記述したくなります.

VSCodeなら type Computed = の方の型アノテーションから定義に飛べます

好みの問題はさておき,結局,問題は「type Computed = の方にカーソルを移動して定義に飛ぶ手間と,重複した型アノテーションを付けてメンテナンスする手間どちらが大きいか」になるかと思います.

機能拡張で新しく computed, data などを追加する場合,既に書かれているコードとその型に注意しながら実装するケースが多くなると思うので,コードから型定義に飛びやすいというのはメリットではないでしょうか.


サンプルとして c5d406c を追加しました.

@nard-tech
Copy link
Contributor Author

この PR が merge されたら #5404 を rebase して作り直したいと思います.

@kaizumaki
Copy link
Collaborator

@nard-tech @MaySoMusician フォローありがとうございます 🙏
つまるところ、個々人のコーディングスタイルや使用しているエディタによるのかなと思いましたが、私個人的にはエディタの機能も含め、とても勉強になりました。
とりあえず、この状態でこちらのPRを採用し、またご意見などあればその都度ディスカッションの上、取り入れていくこととしたいと思います。

Copy link
Collaborator

@kaizumaki kaizumaki left a comment

Choose a reason for hiding this comment

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

長いことissueをフォローしていただいてありがとうございます!LGTMです!

@kaizumaki kaizumaki added the ready-for-merge コードレビューが終わり、マージができるもの label Apr 18, 2021
@nard-tech
Copy link
Contributor Author

@kaizumaki こちらこそ,ありがとうございました.

@soutaito soutaito merged commit 293c461 into Tokyo-Metro-Gov:development Apr 19, 2021
@nard-tech nard-tech deleted the feature/#1100-create-repository-layers branch April 19, 2021 04:27
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ready-for-merge コードレビューが終わり、マージができるもの
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants