-
Notifications
You must be signed in to change notification settings - Fork 0
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
Minor corrections #2
Conversation
832ab4b
to
17efa93
Compare
17efa93
to
46003aa
Compare
408626a
to
9110ba7
Compare
9110ba7
to
9688ece
Compare
src/FlagOverrides.ts
Outdated
@@ -91,7 +89,7 @@ export interface IQueryStringProvider { | |||
} | |||
|
|||
class DefaultQueryStringProvider implements IQueryStringProvider { | |||
get currentValue() { return window?.location.search; } | |||
get currentValue() { if (typeof location !== "undefined") return location?.search; } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe an else could be good here just for the eye.
const transaction = db.transaction(OBJECT_STORE_NAME, "readwrite"); | ||
const store = transaction.objectStore(OBJECT_STORE_NAME); | ||
store.put(value, key); | ||
transaction.oncomplete = () => resolve(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm not 100% sure how the transaction works here, but maybe if the store.put completes before this line, the transaction.oncomplete = () => resolve(); will not fire? Just a guess, but maybe it would be safer to put the transaction.oncomplete and onerror before the store.put
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All the JS runtime implementations I know of use an event loop-based, single-threaded execution model. So put
doesn't have a chance to fire the complete event before the currently processed JS code finishes.
Regardless, I'll change it, just in case.
src/shared/IndexedDBConfigCache.ts
Outdated
const store = transaction.objectStore(OBJECT_STORE_NAME); | ||
const storeRequest = store.get(key); | ||
let value: string | undefined; | ||
storeRequest.onsuccess = event => value = (event.target as IDBRequest<any>).result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here? not sure, just a feeling
|
Describe the purpose of your pull request
Bug fixes:
browser
andchrome-extension
platforms to work correctly in web workers too.DefaultQueryStringProvider
to make it work in web workers (aswindow
global variable is not available web workers.)Breaking changes:
SettingKeyValue
to a plain object type. (Very low impact expected. It's unlikely that consumers create instances of this type but even then they may not use the ctor.)FlagOverrides
to a plain object type. (Internal breaking change only.)LocalStorageCache
toLocalStorageConfigCache
andChromeLocalStorageCache
toChromeLocalStorageConfigCache
. (Internal breaking change only.)Related issues (only if applicable)
n/a
Requirement checklist (only if applicable)