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

enhance: PizzaxデータをindexedDBに保存するように #9225

Merged
merged 13 commits into from
Feb 2, 2023

Conversation

tamaina
Copy link
Contributor

@tamaina tamaina commented Nov 24, 2022

#8098 を間違ってマージしてしまったので再度PR立てます

Related to #5439

Required #8511

What

  • pizzaxをindexedDBに保存するように
  • BroadcastChannelを使ってタブ間で状態を共有するように
  • メインストリームをpizzaxインスタンスを超えて使いまわすように(disposeはきついと思うので)
  • ちょっとだけリファクター

Why

TODOだったのと #5439 に示した理由もある

@github-actions github-actions bot added the packages/frontend Client side specific issue/PR label Nov 24, 2022
@tamaina tamaina requested a review from syuilo November 24, 2022 09:53
@codecov
Copy link

codecov bot commented Jan 2, 2023

Codecov Report

Merging #9225 (a002b21) into develop (b2ed4c9) will not change coverage.
The diff coverage is n/a.

❗ Current head a002b21 differs from pull request most recent head 8e3dd5c. Consider uploading reports for the commit 8e3dd5c to get more accurate results

@@           Coverage Diff            @@
##           develop    #9225   +/-   ##
========================================
  Coverage    22.53%   22.53%           
========================================
  Files          735      735           
  Lines        68917    68917           
  Branches      2022     2022           
========================================
  Hits         15527    15527           
  Misses       53390    53390           

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

@syuilo
Copy link
Member

syuilo commented Jan 28, 2023

p1.aqz.devで動作中?

@tamaina
Copy link
Contributor Author

tamaina commented Jan 28, 2023

50年前ぐらいから動いてる

@syuilo
Copy link
Member

syuilo commented Feb 1, 2023

正しく値を読み出すためにはreadyを待つ必要があるということは、device-kind.tsみたいに同期的にdefaultStore.stateを読んでる箇所は何らかの改修なしには正しく動かなくなりそうかも

@syuilo
Copy link
Member

syuilo commented Feb 1, 2023

何らかの改修

top-level awaitが使えば解決するかも

@tamaina
Copy link
Contributor Author

tamaina commented Feb 1, 2023

top-level awaitって一度導入したけど非難轟々だからやめたんだっけ

@syuilo
Copy link
Member

syuilo commented Feb 1, 2023

そういった記憶はない

@tamaina
Copy link
Contributor Author

tamaina commented Feb 1, 2023

ここら辺

#8931

#8603

@syuilo
Copy link
Member

syuilo commented Feb 1, 2023

まあiOS15切ったし心置きなく使える

@tamaina
Copy link
Contributor Author

tamaina commented Feb 1, 2023

top-level awaitってiOS 15では問題ないはずだけど、ちょっとメジャーじゃないChromium/WebKit/Geckoの追従が遅れているブラウザで問題があるという話だったはず

@syuilo
Copy link
Member

syuilo commented Feb 1, 2023

iOS15という割と新しめの実装を切ったくらいだからその他の遅れてるブラウザも切ったも同然だから良いんじゃねというニュアンスだった

@tamaina
Copy link
Contributor Author

tamaina commented Feb 1, 2023

device-kind.tsみたいに同期的にdefaultStore.stateを読んでる箇所は何らかの改修なしには正しく動かなくなりそう

init.tsでVueがmountされる前にreadyになるのでさほど改修範囲は大きくない気がする

@syuilo
Copy link
Member

syuilo commented Feb 1, 2023

device-kindがinit.tsでimportされてたとすると、ready前の時点でのdevice-kindのexport値がキャッシュされると思うからその後Vueコンポーネント内でimportしたとしてもキャッシュされた値が読まれることになりそう

@tamaina
Copy link
Contributor Author

tamaina commented Feb 1, 2023

うん、init.tsでreadyする前にimportされるもので即時にストアが参照される場合はawaitが必要

@tamaina
Copy link
Contributor Author

tamaina commented Feb 1, 2023

え、top-level awaitにしたらなんか起動早くなったんだが

@syuilo
Copy link
Member

syuilo commented Feb 2, 2023

diff見やすくするためにinit.tsのtop-level awaitだけdevelopに入れた

@syuilo syuilo merged commit 8a6f73c into misskey-dev:develop Feb 2, 2023
@syuilo
Copy link
Member

syuilo commented Feb 2, 2023

🙏

AyumuNekozuki pushed a commit to AyumuNekozuki/misskey that referenced this pull request Feb 13, 2023
* Revert "Revert misskey-dev#8098"

This reverts commit 8b9dc96.

* fix

* use deepClone instead of deepclone

* defaultStore.loaded

* fix load

* wait ready

* use top-level await, await in device-kind.ts
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