-
Notifications
You must be signed in to change notification settings - Fork 3
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
feat! (stores): ✨ Async Stores #129
Conversation
Signed-off-by: Nishant Arora <[email protected]>
Signed-off-by: Nishant Arora <[email protected]>
Signed-off-by: Nishant Arora <[email protected]>
Signed-off-by: Nishant Arora <[email protected]>
Signed-off-by: Nishant Arora <[email protected]>
Signed-off-by: Nishant Arora <[email protected]>
Signed-off-by: Nishant Arora <[email protected]>
Signed-off-by: Nishant Arora <[email protected]>
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.
self-review.
@@ -22,7 +22,6 @@ dist/ | |||
npm-debug.log* | |||
yarn-debug.log* | |||
yarn-error.log* | |||
typings/ |
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.
🤷🏽 Is this an issue 🤔
|
||
export class BrowserStorageProvider implements StorageProvider { | ||
export class BrowserStorageProvider implements StorageProviderInterface { |
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.
this makes it simpler to understand what the interface looks like, otherwise the type changes when we add methods to StorageProvider
@@ -20,8 +20,9 @@ export interface MetricsProviderConstructorOptions<T> { | |||
metricsService: T | |||
queue_size?: number | |||
session_update?: number | |||
storage?: 'none' | 'localStorage' | 'sessionStorage' | 'cookie' |
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.
src/MetricsProvider.ts
Outdated
private readonly storageProvider: StorageProvider | null | ||
private readonly initDone: boolean = false | ||
private readonly storageProvider: StorageProviderInterface | null | ||
public initDone: boolean = false |
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.
this now needs to be checked.
src/MetricsProvider.ts
Outdated
@@ -58,9 +59,13 @@ export default class MetricsProvider<T extends CountlyWebSdk | CountlyNodeSdk> { | |||
this.setupAutoTrack() | |||
} | |||
|
|||
const existingConsent = this.getConsentStore() | |||
void this.initExistingConsent() |
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.
because this call is now async.
tsconfig.json
Outdated
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.
just auto-fixed 😵💫
Signed-off-by: Nishant Arora <[email protected]>
Signed-off-by: Nishant Arora <[email protected]>
src/MetricsProvider.ts
Outdated
private readonly storageProvider: StorageProvider | null | ||
private readonly initDone: boolean = false | ||
private readonly storageProvider: StorageProviderInterface | null | ||
public initDone: boolean = false |
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.
we should probably add _initDone
and make initDone
a function that returns a promise.
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 fine with us handling this later.
while (!telemetry.initDone) { | ||
await new Promise(resolve => setTimeout(resolve, 10)) | ||
} |
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.
not a big fan of needing a while loop, but im okay with it for now.
Can we add a followup action item for exposing a promise that folks can await to determine when init is done?
src/MetricsProvider.ts
Outdated
private readonly storageProvider: StorageProvider | null | ||
private readonly initDone: boolean = false | ||
private readonly storageProvider: StorageProviderInterface | null | ||
public initDone: boolean = false |
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.
public initDone: boolean = false | |
public initDone: Promise<boolean> = false |
src/MetricsProvider.ts
Outdated
@@ -58,9 +62,13 @@ export default class MetricsProvider<T extends CountlyWebSdk | CountlyNodeSdk> { | |||
this.setupAutoTrack() | |||
} | |||
|
|||
const existingConsent = this.getConsentStore() | |||
void this.initExistingConsent() |
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.
void this.initExistingConsent() | |
this.initDone = this.initExistingConsent() |
Signed-off-by: Nishant Arora <[email protected]>
🎉 This PR is included in version 2.0.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
In this PR:
chrome.storage.local.get
...initDone
is nowpublic
.../types/index.d.ts
but it's not actually shipped. Added atypings
folder and moved the shared types to.ts
file, which fixes this (tsc
does not touch.d.ts
files)