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: init.ts周りのeslintエラーと型の修正 #10157

Conversation

k35o
Copy link
Contributor

@k35o k35o commented Mar 1, 2023

What

  • /packages/frontend/src/init.tsファイルの型修正とeslint修正
  • config.tslocal-storage.tsも修正(エラーの原因だと思って最初に修正してしまいました)

Why

misskeyの型安全性を保った状態で開発を進められるようにするため

Additional info (optional)

document.documentElementに定義するlangはこれまでnull の時lang="null"となっていましたが、nullには意味がないため何も設定しないようにしました(空文字列は別の意味を持つため指定しませんでした)。 が設定される可能性があったが、nullの場合は'en-US'にフォールバックするようにした

The lang attribute (in no namespace) specifies the primary language for the element's contents and for any of the element's attributes that contain text. Its value must be a valid BCP 47 language tag, or the empty string. Setting the attribute to the empty string indicates that the primary language is unknown. [BCP47]

https://html.spec.whatwg.org/multipage/dom.html#attr-lang

vueファイルを解釈できずにエラーが出ていたので/packages/frontend/src/types/vue-shim.d.tsに型定義をしました。

windowに特別なプロパティ($i$store)を渡す部分ですが、eslintの設定で一部anyを許容することで対応しました。開発環境でのみwindowに特別なメソッドを持たせているので、本番のwindowからも間違って扱わないようにwindow型の拡張ではなく、eslintの設定で一部anyを許容するようにしました。

@github-actions github-actions bot added the packages/frontend Client side specific issue/PR label Mar 1, 2023
@k35o k35o marked this pull request as ready for review March 1, 2023 10:54
@@ -114,8 +115,10 @@ if (['smartphone', 'tablet'].includes(deviceKind)) {
}

//#region Set lang attr
const html = document.documentElement;
html.setAttribute('lang', lang);
if (lang) {
Copy link
Member

Choose a reason for hiding this comment

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

langがnullになることはないはず?

Copy link
Contributor Author

@k35o k35o Mar 6, 2023

Choose a reason for hiding this comment

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

@syuilo
window.localStorage.getItemの戻り値がstring | nullなので、型としてnullであることを考慮する必要がありそうです(setAttributeの引数はstringしか許容していない)。
空文字列など何らかの初期値をセットするようにしますか?

Comment on lines 38 to 48
//#region account indexedDB migration
import { set } from '@/scripts/idb-proxy';

{
const accounts = miLocalStorage.getItem('accounts');
if (accounts) {
set('accounts', JSON.parse(accounts));
miLocalStorage.removeItem('accounts');
}
}
//#endregion
Copy link
Member

Choose a reason for hiding this comment

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

マイグレーションもう消しても良さそう

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@syuilo
このPRで消して問題ないですか?

@k35o k35o requested review from syuilo and removed request for EbiseLutica and acid-chicken March 8, 2023 09:07
@codecov
Copy link

codecov bot commented Mar 9, 2023

Codecov Report

Merging #10157 (76a6cd2) into develop (1c3d9a5) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff            @@
##           develop   #10157   +/-   ##
========================================
  Coverage    78.59%   78.59%           
========================================
  Files          161      161           
  Lines        19647    19649    +2     
  Branches       331      331           
========================================
+ Hits         15442    15444    +2     
  Misses        4205     4205           
Impacted Files Coverage Δ
packages/frontend/src/config.ts 90.90% <100.00%> (+0.43%) ⬆️
packages/frontend/src/local-storage.ts 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@tamaina
Copy link
Contributor

tamaina commented Mar 9, 2023

  • config.langをen-USにフォールバックしてみた
  • accountのマイグレーションは削除

Copy link
Member

@saschanaz saschanaz left a comment

Choose a reason for hiding this comment

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

👍

@k35o
Copy link
Contributor Author

k35o commented Apr 5, 2023

TypeScriptのバージョン5.0.2と@micorsoft/api-extractorのbundled compilerとバージョン合わないエラーなのでこのPRとは関係なさそうですかね?

@tamaina
Copy link
Contributor

tamaina commented Apr 13, 2023

関係ないはず

@tamaina tamaina merged commit 4634467 into misskey-dev:develop Apr 13, 2023
@tamaina
Copy link
Contributor

tamaina commented Apr 13, 2023

👍

na2na-p pushed a commit to na2na-p/misskey that referenced this pull request May 10, 2023
* refactor/miLocalStorageのメソッドに戻り値追加

* refactor/miLocalStorageのキーとしてdebugがconfig.tsに存在するので追加

* fix/JSON.parseにnullは入らないのでnullの時は分岐させてnullにする

* refactor/修正したファイルの型調整+記法の統一

* fix/型のためにlangがnullの時はhtmlの言語の設定をしない

* refactor/catchで何もしないと警告が出るので修正

* refactor/細かい点の修正

* refactor/変数の二重定義になっていた二重定義になっていたので修正

* refactor/importの整理(通常のimportは最初に処理されるので影響はない想定)

* fix/vueファイルに型を与えてインポート時の型エラーを防ぐ

* refactor/開発環境のみで利用するので,eslintの設定を変更する

* fix/vueの定義を最小限にする

* fallback language to 'en-US'

* remove accounts migration

* fix:vueの型定義ファイルを消す

---------

Co-authored-by: tamaina <[email protected]>
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.

4 participants