-
Notifications
You must be signed in to change notification settings - Fork 2k
component のディレクトリ構成を変更する #5404
component のディレクトリ構成を変更する #5404
Conversation
@nard-tech 整理していただきありがとうございます!現在 #5293 がマージ待ちでして、そちらが終わったらこのPRを運営側で検討したいと思います。少しお時間ください 🙏 |
6ac2078
to
ea2e813
Compare
@kaizumaki #5293 が merge されましたので,最新の development ブランチに rebase し,force push しました. #5632, #5654, #5656 などと conflict が発生するかと思います.申し訳ないですが,そのことを気にしすぎるとこの PR は永遠に merge されないと思いますので,いいタイミングで早めに merge して頂きたいです. 概して,荒れた設計を立て直すときには大きな変更をまとめて行わなければならないことがあると思います. |
@nard-tech お待たせしてしまい申し訳ありません。ご対応ありがとうございます。
これはまったくその通りだと思います。実際のところ、当サイトが立ち上がってからオールシーズンをまだ経ていないので、正直今後どうなるかわかりません。 |
68ef7fc
to
e9dfb0b
Compare
53d7923
to
a2bb406
Compare
ディレクトリ構成については Vue.js 公式のスタイルガイド(BETA版)が存在し、ガイドの密結合コンポーネントの名前 - 強く推奨によれば、
とあります。Nuxtでは |
@MaySoMusician
ことです. 単一階層に component をまとめてしまうと,コンポーネント間の依存関係,階層が明確になりませんし,
ことになり,開発のパフォーマンスを損ねることになると思っています.新しい機能を追加するときに既存の component が乱用される恐れもあります.
であったとしても,よい設計,わかりやすいコードで開発のパフォーマンスを上げる観点からは無視できる程度かと思います. エディタに関しては,VSCode のようなモダンなエディタであれば,
また,TypeScript を導入して型のチェックをしているのでバグを仕込むリスクも少ないはずです. |
単一階層での依存関係の表現については、先程のガイドの
で解決可能ですがいかがでしょうか |
components/README.md
Outdated
|
||
`components/index/_shared/DataView.vue` に関連して用いられるファイル群を配置している. | ||
|
||
**注意**: 旧 `components/DataViewCustomInfoPanel.vue` は「発症日別による陽性者数の推移」の描画のみに用いられているため,`components/index/CardsReference/PositiveNumberByDevelopedDate` 以下に配置した. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
この注意についてですが、ちょっとわかりにくいような気がしますね。
たしかに現在 DataViewCustomInfoPanel.vue
は「発症日別による陽性者数の推移」にしか使っていませんが、コンポーネントの性格としては汎用に使用できるように作られていますから、このディレクトリの下にあることに少し違和感を感じます。
ご提案いただいているコードで以下のようなimportの書き方がありますが、これって一般的なのでしょうか...?
もちろん正常に動作することは理解できるのですが、コンポーネント名とimportしている名前が一致しない書き方はみなさん一般的にされているのだろうか?という素朴な疑問です。 |
@MaySoMusician
という考えに素直に従って正確に階層を表現しようとするとファイル名が極端に長くなり不健全になることがあると思います. 極端な例では,この PR 内で component の数が多いので,適切にディレクトリを区切り,
という階層関係が見えやすくする方が保守性,可読性がよいのではないでしょうか. 例えば,VSCode のファイル一覧で
と表示されるのと,
と表示されるのとでは,後者の方が視認性が優れているはずです. 折衷案として, 補足: |
どの component で何が使われている(何が使われていないか)かを明確にするために,汎用的に使われている component もあえて現状のコードに即して rename しました. DataViewCustomInfoPanel.vue と同様の表示をする component が今後どんどん増加するのであれば汎用性を考慮しておくべきかと思いますが,そうは思いません.あまり汎用性を考慮すると,いわゆる「早すぎる最適化」となってしまって結局意味がなかったということにもなると思いますので,必要なときに refactor すればよいのではないでしょうか. |
これはご指摘の通りあまり一般的ではないので, import Chart from '@/components/index/CardsReference/TestedNumber/Chart.vue' となるように修正したいと思います. 当方の都合でこちらの PR も長く放置してしまい,conflict も発生しているので,#5690 の対応が済んだ頃にまた改めて対応します. |
ご提案の内容だと、Chart.vueというファイルが大量にできているのを懸念しています。エディタでのサジェストが上手くいきません。 |
たしかに仰るとおりなのですが、このプロジェクトのソースコードは多くの人々に参照されているということも考慮に入れていただければと思います。 ディレクトリ構造で言えば、nuxtを使用している時点でディレクトリ構成が決められているのですから、すでに最適化されていると言えると思います。 |
こんな興味深いスレ、もとい PR があったんですね。てんで気づきませんでした。 で、イキナリで恐縮ですけれども、私は 本 PR 最新コミット内容を、 |
@kaizumaki @mcdmaster こちらの PR も長らく放置してしまいましたが,
多くの方が VSCode を使われていると思うのですが,VSCode では command + P でファイル名を名前で検索するときはディレクトリ名も表示されるので,間違ったディレクトリのものを選ばない限りは問題ありません. この PR の目的は,ディレクトリで component 間の階層関係を表現し,開発者の理解を助けることにあります.「エディタでのサジェスト」は主に新しく component を追加するときにだけ使うことになると思いますが,「間違ったディレクトリのものを選ぶリスク」「最初の1回限りのエディタでのサジェストが効くこと」と「今後保守運用,機能追加をするときの開発者の理解のしやすさ」を比べると後者の方のメリットの方がが開発全体に与える影響が大きいのではないでしょうか.
多くの人々に参照されているからこそ理解のしやすさは重要だと思います.
いくつかドキュメントに目を通しましたが,設定が複雑になって新しく参加する開発者の参入障壁が上がってしまう気がしました. 私自身,このプロジェクトに参加するまで Vue の経験はほとんどなく,一般的な JavaScript, TypeScript の知識や React の経験があったので何とか追いつけたのですが,Vue の知識の他に |
ああ、いつの間にか 横に長いファイル名を避けるのは視認性向上につながって良いと思いました。一方で縦向きの視認性も気になります。すなわち、多くのエディタ(とGitHub)ではディレクトリとファイルを分けて一覧化するので、依存されているコンポーネントファイルと依存しているコンポーネント群のディレクトリが離れてしまいます(コンポーネントが増えれば増えるほど)。 ここで、
を部分的に適用すると
が達成でき、 また、ディレクトリは「開くまで何が入っているかわからない」「階層の上り下り自体に1操作を消費する」という課題があります(Windows Explorerやターミナル、GitHubのWebインターフェイスでも)。ディレクトリ内のファイル数が1~数個程度なら、よりフラットな階層構造を目指してもよいのではないでしょうか。 上図を例にすると、 他方で、フラットな構造にした場合、 もうひとつ気になっているのは、 import時に import UntrackedRateChart from '@/components/index/CardsMonitoring/UntrackedRate/Chart.vue' と別名をつけるなら import UntrackedRateCardChart from '@/components/index/CardsMonitoring/UntrackedRateCardChart.vue' でも大差ないと感じます。 |
よくよく考えてみると,以下のようなディレクトリ構造が
の両方を満たしていてよいのではないかと思いました.
具体的なコードを示すと, // 検査陽性者の状況
const ConfirmedCasesDetailsCard = () =>
import('@/components/index/CardsMonitoring/ConfirmedCasesDetails/Card.vue')
// 報告日別による陽性者数の推移
const ConfirmedCasesNumberCard = () =>
import('@/components/index/CardsMonitoring/ConfirmedCasesNumber/Card.vue') のように,「 import Table from '@/components/index/CardsMonitoring/ConfirmedCasesDetails/Table.vue' とする形となります. 複数の
上記の案では,依存されているコンポーネントファイルと依存しているコンポーネントファイルを同じディレクトリに入れておくことになります. component の依存関係は表現できませんが,「1つの図表の描画に必要な component が同じディレクトリの中に入っている」という状態が実現でき,「どのディレクトリも開いてみれば同じような名前のファイルが入っている(けれど,ところどころ違う)」という状態にもなるので,各図表(に必要な component)の共通点と相違点が明確になります. 1つのディレクトリの中で
先のコメントのスクリーンショットで示して頂いたように,ディレクトリ分けを
とできるものが
のようになってしまうので,「横向きの視認性」の点で劣ると思います. 上記の案では, ここで, import { Chart } from 'chart.js' となっている箇所との衝突が懸念されるかもしれませんが,これは type Computed = {
displayOption: Chart.ChartOptions
displayOptionHeader: Chart.ChartOptions
...
} といった形の型定義で使われているものがほとんど( import { ChartOptions } from 'chart.js' のように書き換えることができ,上記の案で発生することになる import Chart from '@/components/index/CardsMonitoring/MonitoringConfirmedCases/Chart.vue' などの import と名前が衝突することはなくなります.(余談ですが,chart という言葉と graph という言葉が混在しているので,各 card で使用している図表描画用の component は
<template>
<chart>
</chart>
</template> と表記したときにどの component が自動で import されるかが保証できません.
import するファイルを指定するとき, // 例1
import Chart from '@/components/index/CardsMonitoring/MonitoringConfirmedCases/Chart.vue'
// 例2
import MonitoringConfirmedCasesChart from '@/components/index/CardsMonitoring/MonitoringConfirmedCases/Chart.vue' のどちらが分かりやすいかは議論が分かれるところではありますが,import しているファイルについて誤解の余地がないという点では自動 import より優れているはずです. |
@nard-tech そうですね。同じディレクトリにCard.vueとTable.vueを入れるのはわかりやすそうですね。 |
私はそれでいいんじゃないかなあと思いました。
という構成にして、自動importする、という感じでしょうか。 |
ご教示ありがとうございます.
という構成だと,nuxt/components で何も設定しないと template 内で nuxt/components に設定を加えることで
といったことが条件になるかと思います. 自動 import を使わず愚直に import を書いていくスタイルだと,#5404 (comment) の末尾に書いたように,
と思われます. たたき台として,nuxt/components に設定を加えて template 内で |
ああーっと、
これは避けたいですねー。そもそも、私が極端な深い階層になるのを避けたかった理由がここだったりします。 importの扱いについては @MaySoMusician さんの意見も伺いたく 🙏 |
横割り失礼します。 個人的には、 |
…rt.vue to components/index/_shared/MixedBarAndLineChart.vue
…hared/OpenDataLink.vue
…sultation/StaticInfo.vue
…ndex/_shared/DataView/ExpantionPanel.vue
…ared/DataView/Share.vue
…d/DataViewDataSetPanel.vue
b1f0580
to
fdd9668
Compare
@kaizumaki @MaySoMusician 他の関連 PR が merge されたので,この PR の確認をお願いします. |
@@ -52,7 +52,9 @@ export type TableDateType = { | |||
* | |||
* @param data - Raw data | |||
*/ | |||
export function formatTable(data: DataType[]): TableDateType { | |||
export function formatConfirmedCasesAttributesTable( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
formatTable
では汎用的に使える印象を与えてしまうので,実態に合わせて改名した.
<div class="InfectionMedicalcareprovisionStatus"> | ||
<div class="InfectionMedicalcareprovisionStatus-heading"> | ||
<h3 class="InfectionMedicalcareprovisionStatus-title"> | ||
<div class="InfectionMedicalCareProvisionStatus"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
可読性を考慮し InfectionMedicalcareprovisionStatus
ではなく InfectionMedicalCareProvisionStatus とした.
import StayingPopulation from '@/components/StayingPopulation.vue' | ||
import WhatsNew from '@/components/WhatsNew.vue' | ||
import Consultation from '@/components/index/SiteTopUpper/Consultation.vue' | ||
import InfectionMedicalCareProvisionStatus from '@/components/index/SiteTopUpper/InfectionMedicalCareProvisionStatus.vue' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
InfectionMedicalCareProvisionStatus を明示的に import した.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ありがとうございます!LGTMです!
ここに至るまで粘り強くご対応いただいたことに感謝です 🙏
</client-only> | ||
</v-col> | ||
</template> | ||
|
||
<script> | ||
import DashedRectangleTimeBarChart from '@/components/DashedRectangleTimeBarChart.vue' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM後にすみません、一部カードのimport時のネーミングルールがミスっている気がします....
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
フォローありがとうございます!ここは削除行のようですが、どうなんでしょう?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
すみません、範囲選択ミスなんですが、Chart
としてimport、使用されています
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
私の理解では Card
で Chart
を配置する、というルールかと思っているのですが、どうなんでしょう? @nard-tech
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
他では
import MonitoringConfirmedCasesNumberChart from '@/components/index/CardsMonitoring/MonitoringConfirmedCasesNumber/Chart.vue'
と言った形でimportされている箇所があるようです。
Chart
と言った形でimportされているところと、このようにフルネームでimportされているところで、ネーミングルールのばらつきがあるのはどうなんだろう?といった指摘でした。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
このコメントでは、区別する必要がなければChart
としてimportすれば良いというように結論づいているように見えましたが、どうなんでしょうか...(つまり、このコメントに従うなら私が指摘すべき所をミスってるわけ(Chart
としてimportしていない所を指摘すべき)ですが...)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
そうですね!おそらく後者が修正もれですね。
また後日の修正でも大丈夫なので、いったんここでマージしますね!
@y-chan フォロー感謝です!!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@y-chan @kaizumaki キャッチアップできずすみません.merge ありがとうございます.後ほど確認して必要なら修正します.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nard-tech ぜんぜん後日で大丈夫です!よろしくお願いします 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@y-chan @kaizumaki #6368 を作成しました.ご確認ください.
👏 解決する issue / Resolved Issues
📝 関連する issue / Related Issues
⛏ 変更内容 / Details of Changes
components/README.md
に記載の通り