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

Resurrect Service Worker #7108

Merged
merged 14 commits into from Feb 6, 2021
Merged

Resurrect Service Worker #7108

merged 14 commits into from Feb 6, 2021

Conversation

tamaina
Copy link
Member

@tamaina tamaina commented Jan 21, 2021

Summary

Resolve #7106

Service Workerへの情報共有を、indexedDBを使うのをやめてpostMessageで渡して変数として保持にすることにして解決した

$i.tokenも渡せるので、Service Woker系のIssueを進められるようになった

@syuilo
Copy link
Member

syuilo commented Jan 21, 2021

ServiceWorkerが起動した時にメインプロセスがいるかどうかはわからず(ブラウザ起動したけどMisskey開いてないとか)、したがってpostMessageされる保証もないので、一度postMessageされたら(= service worker register時?)その結果をService Workerから扱えるLocalStorageかなんかにキャッシュしておいて、以後はそれを参照するようにしないとだめそう
2回目以降postMessageが来たら都度キャッシュ更新すると良さそう

Copy link
Member

@EbiseLutica EbiseLutica left a comment

Choose a reason for hiding this comment

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

コメントしました

src/client/sw/compose-notification.ts Outdated Show resolved Hide resolved
src/client/sw/sw.ts Show resolved Hide resolved
@tamaina
Copy link
Member Author

tamaina commented Jan 21, 2021

ServiceWorkerが起動した時にメインプロセスがいるかどうか

盲点だった

LocalStorageはService Workerで使えないので、やっぱりindexedDBで保存しておく必要がありそう

@syuilo
Copy link
Member

syuilo commented Jan 21, 2021

postMessageでlocaleまるまるじゃなくて言語情報だけ渡して、その言語情報をもとにServiceWorkerがlocaleをAPI叩いてフェッチしてくる、とかでも良さそうと思った(思いつきで喋っているので実現できるかどうかは不明)

@syuilo
Copy link
Member

syuilo commented Jan 21, 2021

LocalStorageはService Workerで使えないので

アレそうだったっけまじか

@syuilo
Copy link
Member

syuilo commented Jan 21, 2021

元サイトのデータにアクセスできないだけで、localStorage自体は使用できると思い込んでたわ

@EbiseLutica
Copy link
Member

@syuilo MDN曰く

メモ: localStorageは Service worker キャッシュと同じように動作しますが、同期処理のため、Service worker 内では許可されていません。
メモ: 必要ならば、IndexedDBを Service worker 内でデータ保存のために使用できます。

だね

@tamaina
Copy link
Member Author

tamaina commented Jan 21, 2021

postMessageでlocaleまるまるじゃなくて言語情報だけ渡して、

できるからそうするほうがいいならそうするか…?
ただ、Service Worker自身のバージョンの変更ができないので、localeのバージョンの変更にも対応できないかも

@tamaina
Copy link
Member Author

tamaina commented Jan 21, 2021

(ちなみに、Service Workerのバージョン変更は、同じURLであっても更新を勝手に検知してくれる

@syuilo
Copy link
Member

syuilo commented Jan 21, 2021

ServiceWorkerが自身が己に必要な情報をフェッチしてくる設計の方が、関心が分離されてて良さそうな気はしている
localeのバージョン云々はよく理解できてないけど、ServiceWorkerが起動するたびに翻訳フェッチすればデータが古くなるのは避けられそう

@tamaina
Copy link
Member Author

tamaina commented Jan 21, 2021

(サーバーのバージョンが新しくなってlocaleのURLが新しくなっても、Service Workerのバージョンが古いと古いlocaleを探してしまう話

@syuilo
Copy link
Member

syuilo commented Jan 21, 2021

まあそうするにしても言語情報などをキャッシュしておく必要はあるから、やはりIndexedDBの利用は必須だなあ

@tamaina
Copy link
Member Author

tamaina commented Jan 21, 2021

通常のクライアントからごってり実装するのではなく、Service WorkerからindexedDBに簡単に保存する感じでいいと思う…

@tamaina
Copy link
Member Author

tamaina commented Jan 22, 2021

iを保存しておくのに複数アカウントをどうするのかという問題に気付いた

@tamaina tamaina marked this pull request as draft January 22, 2021 14:27
@tamaina
Copy link
Member Author

tamaina commented Jan 22, 2021

とりあえず今回は、アカウントの問題は考えずにlangだけを考えることにする

(langがidb-keyvalとlocalStorageで両方存在するのはよくなさそうだけど、boot.jsあたりをいじり始めるとキリがつかない…)

@tamaina tamaina marked this pull request as ready for review January 22, 2021 16:05
@tamaina
Copy link
Member Author

tamaina commented Jan 22, 2021

メモ: クライアントがない(バックグラウンド)状態のService Workerの挙動について

  • Service Worker再起動時のライフサイクルというものは存在しないので、実行時に必要な情報はベタ書きで読み込む
  • Service Workerは積極的に停止され、通知などの処理が必要な段階で起動される

@tamaina
Copy link
Member Author

tamaina commented Jan 22, 2021

localeはいちいちサーバーに取りに行かずともcacheで取得しておいてもいいかも

@rinsuki
Copy link
Contributor

rinsuki commented Jan 22, 2021

適当に IndexedDB をVue→SW用データ押し付けストアとして使うだけでうまくいきそうな気がする (Vueから見ずに書き込み失敗を無視すれば Firefox のプライベートブラウザでも動くはず)

@tamaina
Copy link
Member Author

tamaina commented Jan 22, 2021

localStorageと同じ情報をIndexedDBに保存するのはどうなのかの話か

Firefoxのプライベートでも使えるようにするのは、フォールバックでlocalStorageを使うようにするようなものを作ればいいんじゃないかなと

プッシュ通知のactionsの実装ではService Workerがtokenを必要とするので、accountsまわりはindexeddb(使えなければlocalStorage)に統一するようになると思われる

@tamaina
Copy link
Member Author

tamaina commented Jan 23, 2021

localeはいちいちサーバーに取りに行かずともcacheで取得しておいてもいいかも

やった

このPRはこれでおわり

}

if (typeof ev.data === 'object') {
const otype = Object.prototype.toString.call(ev.data).slice(8, -1).toLowerCase();
Copy link
Member

@syuilo syuilo Jan 23, 2021

Choose a reason for hiding this comment

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

sliceの 8 とか -1 がマジックナンバーに見えて何やってるかイマイチ把握できないので、コメントなりあると嬉しいかも

Copy link
Member Author

Choose a reason for hiding this comment

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

やったけどこんな感じでいいかしら

src/server/web/boot.js Outdated Show resolved Hide resolved
@tamaina tamaina mentioned this pull request Jan 27, 2021
8 tasks
@syuilo
Copy link
Member

syuilo commented Jan 30, 2021

yosasou

@syuilo
Copy link
Member

syuilo commented Jan 30, 2021

ちょっとテストしてみるか(ローカルでservice workerのテストできたっけ)

@tamaina
Copy link
Member Author

tamaina commented Jan 30, 2021

できるはず

@tamaina
Copy link
Member Author

tamaina commented Jan 30, 2021

一応c2鯖でうごいてる

Comment on lines 174 to 215
//if (this.store.state.instance.meta.swPublickey) this.registerSw(this.store.state.instance.meta.swPublickey);
if (instance.swPublickey &&
('serviceWorker' in navigator) &&
('PushManager' in window) &&
$i && $i.token) {
navigator.serviceWorker.register(`/sw.js`);

navigator.serviceWorker.ready.then(registration => {
registration.active?.postMessage({
msg: 'initialize',
lang,
});
// SEE: https://developer.mozilla.org/en-US/docs/Web/API/PushManager/subscribe#Parameters
registration.pushManager.subscribe({
userVisibleOnly: true,
applicationServerKey: urlBase64ToUint8Array(instance.swPublickey)
}).then(subscription => {
function encode(buffer: ArrayBuffer | null) {
return btoa(String.fromCharCode.apply(null, new Uint8Array(buffer)));
}

// Register
api('sw/register', {
endpoint: subscription.endpoint,
auth: encode(subscription.getKey('auth')),
publickey: encode(subscription.getKey('p256dh'))
});
})
// When subscribe failed
.catch(async (err: Error) => {
// 通知が許可されていなかったとき
if (err.name === 'NotAllowedError') {
return;
}

// 違うapplicationServerKey (または gcm_sender_id)のサブスクリプションが
// 既に存在していることが原因でエラーになった可能性があるので、
// そのサブスクリプションを解除しておく
const subscription = await registration.pushManager.getSubscription();
if (subscription) subscription.unsubscribe();
});
});
}
Copy link
Member

Choose a reason for hiding this comment

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

この辺別ファイルに切り出しても良いかも

@syuilo
Copy link
Member

syuilo commented Feb 6, 2021

c2鯖でテストしてみたけど通知来ないな(service workerのインスペクタにも何もエラーなどは出ない)

@tamaina
Copy link
Member Author

tamaina commented Feb 6, 2021

うーん?ちゃんとタブ閉じて検証してみたよね…?

OS・ブラウザは何でしょうか?

@syuilo
Copy link
Member

syuilo commented Feb 6, 2021

うーん?ちゃんとタブ閉じて検証してみたよね…?

yes

OS・ブラウザは何でしょうか?

Windows10 / Chrome 88.0.4324.104

@tamaina
Copy link
Member Author

tamaina commented Feb 6, 2021

通知の許可設定はどうなってますか?

@syuilo
Copy link
Member

syuilo commented Feb 6, 2021

許可してる

@syuilo
Copy link
Member

syuilo commented Feb 6, 2021

まあaqzが通知受け取れているならおま環だと思うので無視してもらってOK

@tamaina
Copy link
Member Author

tamaina commented Feb 6, 2021

うん、私は普通に受け取れているんですよね…なんでだろう…

@syuilo syuilo merged commit 40bfa3e into misskey-dev:develop Feb 6, 2021
@syuilo
Copy link
Member

syuilo commented Feb 6, 2021

👍👍👍

@syuilo
Copy link
Member

syuilo commented Feb 7, 2021

@tamaina
self.addEventListener('fetch', ev => { ... って必要だっけ?
開発時とかキャッシュされていろいろ不都合だから消したい

@tamaina
Copy link
Member Author

tamaina commented Feb 7, 2021

必要…だと思っていたけど必要ではないかも

PWAとして認識されるための条件としてオフラインでも200で応答することというのがあるんだけど、キャッシュではなく適当な文字列でResponse.Response()を返せばいいことに最近ようやく気がついた

@syuilo
Copy link
Member

syuilo commented Feb 7, 2021

消してみたけどまだPWAとしてインストールできそう
あと少なくともプッシュ通知受け取るのには fetch の listen は不要っぽい

@syuilo
Copy link
Member

syuilo commented Feb 7, 2021

消してみたけどまだPWAとしてインストールできそう

いやこれは嘘

foggy1 pushed a commit to foggy-llc/pluskey that referenced this pull request May 8, 2021
* Resolve misskey-dev#7106

* fix lint

* fix lint

* save lang in idb

* fix lint

* fix

* cache locale file

* fix lint

* ✌️

* wip

* fix [wip]

* fix [wip]

Co-authored-by: syuilo <Syuilotan@yahoo.co.jp>
@tamaina tamaina mentioned this pull request Aug 19, 2021
9 tasks
snk-lab pushed a commit to snk-lab/misskey-kusa-note that referenced this pull request Sep 24, 2021
* Resolve misskey-dev#7106

* fix lint

* fix lint

* save lang in idb

* fix lint

* fix

* cache locale file

* fix lint

* ✌️

* wip

* fix [wip]

* fix [wip]

Co-authored-by: syuilo <Syuilotan@yahoo.co.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Service Workerの復活
5 participants