Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor(frontend): router.ts解きほぐし #12907

Merged
merged 9 commits into from
Jan 8, 2024

Conversation

samunohito
Copy link
Member

@samunohito samunohito commented Jan 5, 2024

What

#12602 の対応の一環です(書いてないけど)
この対応により、大部分のページでリロードなしのHMRが可能となります。

対応内容としては下記となります。

  • ①ルーティングの定義を切り出し
    循環参照の巨大な温床となっていたルーティング定義を起動シーケンス(boot/common.ts)でのみ読み込むように切り出し。また、mainRouterの初期化もここで明示的に行うようにした(いままではrouter.tsが評価されたタイミングで初期化していたと思われるので)
  • mainRouterの保持を切り出し
    ①で作成されたmainRouterのインスタンスを持つためのファイルを切り出し。①を一切参照していないため、mainRouterをimportしても循環参照が発生しないようになっている。
  • inject経由でのインスタンス取得方法提供
    mainRouterのインスタンスそのものや、Routerを作成するためのファクトリ関数(※)をinject経由で取得するためのユーティリティ。
    MkPageWindowsにて都度Routerのインスタンスを作る必要があるため、そこでの使用を想定

Why

フロントエンドの開発効率向上および開発負担軽減

Additional info (optional)

  • デフォルト表示中、サイドバーをクリックしてページ遷移が出来ること、アドレスバーのアドレスが変わること
  • チャンネルのリンクをコピー→別タブに貼り付け&移動で該当チャンネルのTLを表示できること
  • サイドバーの項目を右クリック→ウィンドウで開くを選択→選択したメニュー項目に相当するウィンドウが表示されること
  • デッキ表示中、サイドバーをクリックするとページ遷移せずにウィンドウが表示されること。表示されるウィンドウもメニュー項目に相当するものであること。

Checklist

  • Read the contribution guide
  • Test working in a local environment
  • (If needed) Add story of storybook
  • (If needed) Update CHANGELOG.md ※動作そのものには影響がないので記載していません
  • (If possible) Add tests

@github-actions github-actions bot added the packages/frontend Client side specific issue/PR label Jan 5, 2024
Copy link

codecov bot commented Jan 5, 2024

Codecov Report

Attention: 99 lines in your changes are missing coverage. Please review.

Comparison is base (0ed2a22) 80.02% compared to head (ae4d4ea) 79.72%.

Files Patch % Lines
packages/frontend/src/global/router/main.ts 44.17% 91 Missing ⚠️
packages/frontend/src/global/router/supplier.ts 73.33% 8 Missing ⚠️
Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #12907      +/-   ##
===========================================
- Coverage    80.02%   79.72%   -0.30%     
===========================================
  Files          959      957       -2     
  Lines       109563   109114     -449     
  Branches      8522     8487      -35     
===========================================
- Hits         87676    86995     -681     
- Misses       21887    22119     +232     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

Choose a reason for hiding this comment

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

defineというよりdefinition?

Copy link
Member Author

Choose a reason for hiding this comment

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

definitionは名詞のようで、"定義"という言葉そのものを示すようです。
一方、defineは動詞のようなので、こちらの方が適していると考えています。

Copy link
Member

Choose a reason for hiding this comment

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

見た感じdefineを行う関数があるわけでもなくdefinition自体が書かれているように見えた

Copy link
Member Author

Choose a reason for hiding this comment

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

特にこだわりは無いので、definitionにしました

@samunohito samunohito marked this pull request as ready for review January 5, 2024 10:46
@samunohito samunohito changed the title [wip] refactor(frontend): router.ts解きほぐし refactor(frontend): router.ts解きほぐし Jan 5, 2024
@samunohito
Copy link
Member Author

openにしました 🙏

Copy link
Member

Choose a reason for hiding this comment

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

useRouter.tsの方がいいかも

@syuilo syuilo merged commit 04f9147 into misskey-dev:develop Jan 8, 2024
@syuilo
Copy link
Member

syuilo commented Jan 8, 2024

🙏🙏🙏

@samunohito samunohito deleted the refactor/separate_router_ts branch January 11, 2024 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
packages/frontend Client side specific issue/PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants