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

hikyaru-suzuki · 2020-04-08T11:23:04Z

👏 解決する issue / Resolved Issues

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

📝 関連する issue / Related Issues

非機能要件（可読性・拡張性・設計）問題点・改善点の整理 #2790

⛏ 変更内容 / Details of Changes

メイン

新たに作成したlibrariesディレクトリにビルド後参照されるTypeScriptファイルを集約（utilsディレクトリを移動）
JSONファイルの型や変換コードを quicktype で自動生成しlibraries/auto_generatedに配置
JSONファイルとVueの間に型注釈を行いデータ変換を行うRepository層を実装

その他

ESLintのキャッシュを有効化
~/ を @/ に統一
package.jsonのdependenciesを整理
- expressは必要ないと思われるので削除
- ビルド後に必要のないものはdevDependenciesに移動

設計について

Vue -> RepositoryのInterface <- Repositoryの実装 -> JSONファイルという依存関係にすることでVue側からはRepositoryの実装変更やマスターデータの実態を意識することなくVue側の実装を行うことが可能となります
Repository実装のDependencyInjectionについてはlibraries/Registry.tsにて一元管理しています。このクラス内でpublic static readonlyなInterfaceの型で宣言したプロパティにインスタンス化したオブジェクトをDIしています

ポイント

変更が多いのでコミット単位で見て頂けるとわかりやすいかと思います🙇
一応、Vue側はRepositoryのInterfaceに依存していますが、そのInterfaceに指定するジェネリクスパラメータは自動生成された型に依存しているので完全に分離したとは言えません
もっと分離するならInterfaceのファイルを分けたり基底クラスやインタフェースを作らないようにするべきですが、今回はそこまで大規模ではないので扱いやすさに焦点を当てました
現在のRepositoryは動的にデータが変更されることを考慮していません。インスタンス化された際にマスターデータをメモリ上にReadOnlyの形で保持しているだけなので、そのあたりを変更するならキャッシュなどの点でもう少し考慮するべきことがあるので注意が必要です

nard-tech

気になる点，いくつかコメントしました．（コメントしていないけれどコメントした箇所と同様のところもあります）
本来はこの PR とは別に対処すべきかもしれませんので，お手すきであれば直して頂いて，そうでなければ私の方で後ほどリファクタリングします．

components/cards/InspectionPersonsNumberCard.vue

components/cards/ChiyodaVisitorsCard.vue

components/cards/TestedCasesDetailsCard.vue

libraries/auto_generated/data_converter/convert13101DailyVisitors.ts

components/cards/ConfirmedCasesAttributesCard.vue

hikyaru-suzuki · 2020-04-09T05:03:42Z

@nard-tech
次の理由のためインポートパスだけ修正しました！

本来はこの PR とは別に対処すべきかもしれませんので，お手すきであれば直して頂いて，そうでなければ私の方で後ほどリファクタリングします．

恐らく #3008 (comment) あたりのことだと思いますが、どのプロパティをどこで使っているか把握した上で最適化する必要があってその上数が多いので、このPRに混ぜるとややこしそうですし別PRでのリファクタリングをお願い致します！（ちゃんと書くならキャメルケースにしたほうが良いと思うのでconst { inspection_persons: inspectionPersons } = Registry.DataRepository.dataのような感じでしょうか？）
修正対象はDataRepositoryに限らずRegistryが使われている場所なので 8b36fe1 のコミットにあるファイルですかね

ただ、そのスコープ内の計算に使うだけなら分割代入をしてしまうとシャローコピーが走ってしまうので対象のプロパティを変数に格納して参照は維持したままにしたほうが良いかもしれません。dataの戻り値も新規にオブジェクトを作成しますが参照は持っている（はず。Vueがdataの戻り値をどう処理しているかによります）のでシャローコピーが走ると無駄になっちゃいます。でもシャローコピーなら重くないのでこのあたりの計算コストと可読性をどうするかはお任せ致します！

nard-tech · 2020-04-10T16:15:13Z

@hikyaru-suzuki お疲れさまです．

どのプロパティをどこで使っているか把握した上で最適化する必要があってその上数が多いので、このPRに混ぜるとややこしそうですし別PRでのリファクタリングをお願い致します！

そうですね．別PRで着手することにします．

ちゃんと書くならキャメルケースにしたほうが良いと思う

スネークケースとキャメルケースが混在してしまっているのは気持ち悪いですよね．(cf. #3108)
JS のお作法としてはキャメルケースの方が望ましいはずですので，こちらも後ほど issue を立てておこうと思います．

修正対象はDataRepositoryに限らずRegistryが使われている場所なので 8b36fe1 のコミットにあるファイルですかね

8b36fe1 に限らず，components 以下のファイルは全体的に荒れている感があると思っています．

計算コストと可読性

難しいところですね．大勢が編集に参加するのであれば可読性と「美しさ」重視の方がいいのかなと思いますが，実際に修正を入れるときにバランスを考慮したいと思います．

nard-tech · 2020-04-10T16:15:33Z

@hikyaru-suzuki そして，conflict してしまっているので修正をお願いします…

（追記）#2916 の merge の後の方がいいかもしれません．

hikyaru-suzuki

（追記）#2916 の merge の後の方がいいかもしれません．

承知いたしました！ #2916 のマージ後にコンフリクト解決を行ってまたご連絡します！

スネークケースとキャメルケースが混在してしまっているのは気持ち悪いですよね．(cf. #3108)

こちらについてですが、quicktypeのオプションを使うことでキャメルケースへ自動変換することが出来ます。当然Vue側の修正も必要となってきますが、今回型付けによって以前よりは楽に対応できるのではないでしょうか
今回自動生成の際に入れようか迷いましたがVue側も大掛かりな変更になるので今回のPRの責務外かなと思い有効にしませんでした
こういうことを出来るのがデータ実態とアプリコードの間に一枚挟む設計の良さですよね〜

$  yarn -s run quicktype --help

Synopsis

  $ quicktype [--lang LANG] [--out FILE] FILE|URL ... 

~~中略~~

Options for TypeScript

 --[no-]just-types                                 Interfaces only (off by default)                            
 --[no-]nice-property-names                        Transform property names to be JavaScripty (off by default) 
 --[no-]explicit-unions                            Explicitly name unions (off by default)                     
 --[no-]runtime-typecheck                          Verify JSON.parse results at runtime (on by default)        
 --acronym-style original|pascal|camel|lowerCase   Acronym naming style                                        
 --converters top-level|all-objects                Which converters to generate (top-level by default)         

~~中略~~

--[no-]nice-property-names Transform property names to be JavaScripty (off by default)

このオプションを有効にすることでキャメルケースに変換されます。

hikyaru-suzuki

上記、対応するならこちらとは別PRの方が良さそうですね

#3008 (comment)

そうですね．別PRで着手することにします．

hikyaru-suzuki

実際に手元で実行してみました
libraries/auto_generated/data_converter/convertMetro.tsを例にするとこんな感じとなります

hikyaru-suzuki · 2020-04-10T20:17:10Z

tool/generateDataConverter.ts

+      lang: 'ts',
+      out: path.resolve(outputPath, `convert${pascalFileName}.ts`),
+      topLevel: pascalFileName,
+      src: [path.resolve(inputPath, sourceFileName)]


オプションを有効化すると

Suggested change

src: [path.resolve(inputPath, sourceFileName)]

src: [path.resolve(inputPath, sourceFileName)],

rendererOptions: { 'nice-property-names': 'true' }

hikyaru-suzuki · 2020-04-10T20:18:11Z

libraries/auto_generated/data_converter/convertMetro.ts

+export interface Metro {
+    date:        string;
+    datasets:    Dataset[];
+    labels:      string[];
+    base_period: string;
+}


Interfaceの定義がキャメルケースに変換されて

Suggested change

export interface Metro {

date: string;

datasets: Dataset[];

labels: string[];

base_period: string;

}

export interface Metro {

date: string;

datasets: Dataset[];

labels: string[];

basePeriod: string;

}

hikyaru-suzuki · 2020-04-10T20:18:47Z

libraries/auto_generated/data_converter/convertMetro.ts

+        { json: "date", js: "date", typ: "" },
+        { json: "datasets", js: "datasets", typ: a(r("Dataset")) },
+        { json: "labels", js: "labels", typ: a("") },
+        { json: "base_period", js: "base_period", typ: "" },


実際に変換を行う関数にその情報が伝えられます

Suggested change

{ json: "base_period", js: "base_period", typ: "" },

{ json: "base_period", js: "basePeriod", typ: "" },

hikyaru-suzuki · 2020-04-10T20:32:20Z

@nard-tech （メンション忘れていました）

ここで議論すべきではないと思いますが

Vueテンプレート内のコンポーネントの記法はケバブケース（or パスカルケース。スタイルガイドでは割とパスカルケース押しっぽい？）
TypeScript内は全てキャメルケース（Vueテンプレート内で使用する変数や関数も含む）
jsonファイルのプロパティ名の記法は何でも良い（quicktypeによる変換が入るため。現在はごちゃごちゃです）

のように統一していけたらきれいで見やすいコードになるかと！
あとESLintのルールもしっかり設定すれば事前に弾けそうですね！

nard-tech · 2020-04-13T09:25:14Z

@hikyaru-suzuki 丁寧に解説頂きありがとうございます．ESLint の設定も，不慣れですがやってみようと思っています．

nard-tech · 2020-04-13T09:32:06Z

@hikyaru-suzuki
#2916 が

@nard-tech お待たせして申し訳ありません。グラフ・表に関する大きめのPRのマージを控えていることと、都から新規グラフ・表の追加の依頼がきている現状から、こちらのPRについてはタイミングを見計っている状況です。

という状況なので，こちらもしばらく置いておいた方がよさそうですね．

cf. a0a9e95 Tokyo-Metro-Gov#3008 (review) Tokyo-Metro-Gov#3008 (review)

nard-tech · 2021-04-18T14:59:03Z

@kaizumaki こちらの close もお願いします．

hikyaru-suzuki added 16 commits April 8, 2020 15:40

add: install quicktype, lodash, types

0c7fe0f

add: data type generator

a0a9e95

add: tsconfig exclude tool directory

37b542c

add: generate data type

7f935b8

add: .eslintignore

587339f

add: Repository layer

13e10b7

add: Instance Registry

eb4fba2

update: use Repository

8b36fe1

fix: alias

d60cd8c

fix: lint

e79f95f

update: use generated type

4197bf3

add: enable ESLint cache

1017d50

update: move directory

edcb529

update: create directory for auto generated files

1aa691c

update: README

5322d94

update: remove express and move to devDependencies

c96f628

nard-tech reviewed Apr 8, 2020

View reviewed changes

fix: use import path alias

e1f206f

hikyaru-suzuki commented Apr 10, 2020

View reviewed changes

nard-tech mentioned this pull request Apr 12, 2020

component のディレクトリ構成を変更 #2916

Closed

nard-tech mentioned this pull request Mar 14, 2021

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

Merged

nard-tech added a commit to nard-tech/covid19-tokyo that referenced this pull request Mar 14, 2021

tool/generateDataConverters.ts: 追加

86404a0

cf. a0a9e95 Tokyo-Metro-Gov#3008 (review) Tokyo-Metro-Gov#3008 (review)

nard-tech added a commit to nard-tech/covid19-tokyo that referenced this pull request Mar 14, 2021

tool/generateDataConverters.ts: 追加

3bb8ba4

cf. a0a9e95 Tokyo-Metro-Gov#3008 (review) Tokyo-Metro-Gov#3008 (review)

nard-tech added a commit to nard-tech/covid19-tokyo that referenced this pull request Mar 14, 2021

tool/generateDataConverters.ts: 追加

a220240

cf. a0a9e95 Tokyo-Metro-Gov#3008 (review) Tokyo-Metro-Gov#3008 (review)

soutaito merged commit 03116f2 into Tokyo-Metro-Gov:development Apr 19, 2021

nard-tech mentioned this pull request Apr 27, 2021

component に対する型の指定と Vuex による JSON データの読み込み nard-tech/covid19-tokyo#9

Open

nard-tech mentioned this pull request Jun 7, 2021

component に対する型の指定と Vuex による JSON データの読み込み #6388

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

hikyaru-suzuki commented Apr 8, 2020 •

edited

Loading

nard-tech left a comment

hikyaru-suzuki commented Apr 9, 2020

nard-tech commented Apr 10, 2020

nard-tech commented Apr 10, 2020 •

edited

Loading

hikyaru-suzuki left a comment

hikyaru-suzuki left a comment •

edited

Loading

hikyaru-suzuki left a comment

hikyaru-suzuki Apr 10, 2020

hikyaru-suzuki Apr 10, 2020

hikyaru-suzuki Apr 10, 2020

hikyaru-suzuki commented Apr 10, 2020

nard-tech commented Apr 13, 2020

nard-tech commented Apr 13, 2020

nard-tech commented Apr 18, 2021

	src: [path.resolve(inputPath, sourceFileName)]
	src: [path.resolve(inputPath, sourceFileName)],
	rendererOptions: { 'nice-property-names': 'true' }

	{ json: "base_period", js: "base_period", typ: "" },
	{ json: "base_period", js: "basePeriod", typ: "" },

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

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

Conversation

hikyaru-suzuki commented Apr 8, 2020 • edited Loading

👏 解決する issue / Resolved Issues

📝 関連する issue / Related Issues

⛏ 変更内容 / Details of Changes

ポイント

nard-tech left a comment

Choose a reason for hiding this comment

hikyaru-suzuki commented Apr 9, 2020

nard-tech commented Apr 10, 2020

nard-tech commented Apr 10, 2020 • edited Loading

hikyaru-suzuki left a comment

Choose a reason for hiding this comment

hikyaru-suzuki left a comment • edited Loading

Choose a reason for hiding this comment

hikyaru-suzuki left a comment

Choose a reason for hiding this comment

hikyaru-suzuki Apr 10, 2020

Choose a reason for hiding this comment

hikyaru-suzuki Apr 10, 2020

Choose a reason for hiding this comment

hikyaru-suzuki Apr 10, 2020

Choose a reason for hiding this comment

hikyaru-suzuki commented Apr 10, 2020

nard-tech commented Apr 13, 2020

nard-tech commented Apr 13, 2020

nard-tech commented Apr 18, 2021

hikyaru-suzuki commented Apr 8, 2020 •

edited

Loading

nard-tech commented Apr 10, 2020 •

edited

Loading

hikyaru-suzuki left a comment •

edited

Loading