Chore: remove dead code in CommonDB#18107
Conversation
|
🔍🌐 Suite Web test results: View in Currents |
|
🔍🖥️ Suite Desktop test results: View in Currents |
| "@trezor/env-utils": "workspace:*", | ||
| "@trezor/eslint": "workspace:*", | ||
| "@trezor/utils": "workspace:*", | ||
| "broadcast-channel": "^7.0.0", |
There was a problem hiding this comment.
- This lib is what led me to find this obsolete code at all (I was about to bump it)
- It' super weird because
BroadcastChannelis natively offerred by browsers, and we rely on the native implementation at multiple places! - only here we use this polyfill lib for old browsers – meaning Chrome & Firefox < 2016 & Safari < 2022, lol
| r.onsuccess = () => { | ||
| indexedDB.deleteDatabase('test'); | ||
| resolve(true); | ||
| }; |
There was a problem hiding this comment.
This workaround is not needed anymore. The bug linked in comment was marked resolved 5 yrs ago.
I also tested it myself in Firefox Private window at any https page, not about:blank or file:/// etc.:
const r = indexedDB.open('test');
r.onerror = () => console.log('NOT SUPPORTED');
r.onsuccess = () => {indexedDB.deleteDatabase('test'); console.log('WORKS'); };
| onChange = (handler: (event: StorageMessageEvent<TDBStructure>) => any) => { | ||
| // listens to the channel. On receiving a message triggers the handler func | ||
| this.broadcastChannel.onmessage = handler; | ||
| return Promise.resolve(isSupported && !this.blocking && !this.blocked); |
There was a problem hiding this comment.
At first I thought that removing the FF workaround can reduce this whole chain of function from async to sync.
Yes it can, but it's not practical!
In storageActions there are 37 occurrences of await db.isAccessible(). These functions have to consistently return Promise, but without await you'd have to remove async, and it would be a lot of boilerplate code to Promise.resolve() everything to ensure they always return a Promise 🤦
| this.notify(store, keys); | ||
| }); | ||
| ); | ||
| const aggregatedPromise: Promise<void> = promisesOfItems.then(() => {}); |
There was a problem hiding this comment.
this fn is expected to return Promise<void>, not Promise<void[]> (would fail downstream in suite).
Naming the function makes this clear without code commentary
|
Failing test is confirmed to be broken (slack) |
WalkthroughThe pull request removes dependencies and simplifies functionality across the storage package. In the package configuration, the dependencies on 📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (8)
💤 Files with no reviewable changes (6)
🧰 Additional context used🪛 Biome (1.9.4)packages/suite-storage/src/web/index.ts[error] 192-192: void is confusing outside a return type or a type parameter. Unsafe fix: Use undefined instead. (lint/suspicious/noConfusingVoidType) ⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (8)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
2 participants
Description
Related Issue
Resolve #18106