-
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: use localStorage to cache consent in browsers #64
Conversation
f25e892
to
0524e74
Compare
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
src/BrowserStorageProvider.ts
Outdated
@@ -0,0 +1,28 @@ | |||
import type { StorageProvider } from './types/index.js' | |||
|
|||
export const BrowserStorageProvider: StorageProvider = { |
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 are only providing storage provider for non-node because that's the only one we currently need.
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.
Some nits.
Co-authored-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>
Co-authored-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>
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. ready for review from @whizzzkid and @djmcquillan
window: 'globalThis', | ||
global: 'globalThis', |
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.
fix for our code in general, but I was attempting to solve testing the browserMetricsProvider and could not get it to play nice.
"browser": "./dist/src/BrowserMetricsProvider.js", | ||
"import": "./dist/src/NodeMetricsProvider.js" | ||
}, | ||
"./browser-vanilla": { | ||
"types": "./dist/src/BrowserMetrics.d.ts", | ||
"browser": "./dist/src/BrowserMetrics.js", | ||
"import": "./dist/src/BrowserMetrics.js" | ||
"browser": "./dist/src/BrowserMetricsProvider.js", | ||
"import": "./dist/src/BrowserMetricsProvider.js" | ||
}, | ||
"./node-vanilla": { | ||
"types": "./dist/src/NodeMetrics.d.ts", | ||
"node": "./dist/src/NodeMetrics.js", | ||
"import": "./dist/src/NodeMetrics.js" | ||
"node": "./dist/src/NodeMetricsProvider.js", | ||
"import": "./dist/src/NodeMetricsProvider.js" |
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.
renamed the files to match the classes.
import Countly from 'countly-sdk-web' | ||
import MetricsProvider, { MetricsProviderConstructorOptions } from './MetricsProvider.js' | ||
import { BrowserStorageProvider } from './BrowserStorageProvider.js' | ||
import type { MetricProviderOptionalConstructorArgs, WithOptional } from './types/index.js' | ||
|
||
export class BrowserMetricsProvider extends MetricsProvider<typeof Countly> { | ||
constructor (args: WithOptional<MetricsProviderConstructorOptions<typeof Countly>, MetricProviderOptionalConstructorArgs>) { | ||
super({ | ||
metricsService: Countly, | ||
storageProvider: new BrowserStorageProvider(), | ||
...args | ||
}) | ||
} | ||
} | ||
|
||
export default BrowserMetricsProvider |
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.
pretty much the same as before with BrowserMetrics.ts
except that args can overwrite metricsService & storageProvider if necessary. we will need to overwrite storageProvider for companion
Also, created utility type WithOptional
that changes the original required constructor argument's properties to optional via the new MetricProviderOptionalConstructorArgs
string union.
import type { consentTypes } from './types/index.js' | ||
import type { StorageProvider } from './StorageProvider.js' | ||
|
||
export class BrowserStorageProvider implements StorageProvider { | ||
setStore (consentArray: consentTypes[]): void { | ||
try { | ||
const jsonString = JSON.stringify(consentArray) | ||
globalThis.localStorage.setItem('@ipfs-shipyard/ignite-metrics:consent', jsonString) | ||
} catch (err) { | ||
// eslint-disable-next-line no-console | ||
console.error(err) | ||
} | ||
} | ||
|
||
getStore (): consentTypes[] { | ||
try { | ||
const jsonString = globalThis.localStorage.getItem('@ipfs-shipyard/ignite-metrics:consent') | ||
if (jsonString != null) { | ||
return JSON.parse(jsonString) | ||
} | ||
} catch (err) { | ||
// eslint-disable-next-line no-console | ||
console.error(err) | ||
} | ||
/** | ||
* Return minimal consent if there is nothing in the store. | ||
*/ | ||
return ['minimal'] | ||
} | ||
} |
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.
pretty much the same as in the earlier PR, but using a class
} | ||
|
||
export default class MetricsProvider<T extends CountlyWebSdk & CountlyNodeSdk> { | ||
export default class MetricsProvider<T extends CountlyWebSdk | CountlyNodeSdk> { |
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.
either or, not both.
if (this.storageProvider != null && this.initDone) { | ||
this.storageProvider.setStore(Array.from(this._consentGranted)) | ||
} |
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.
basically the same as the previous revision, except we're blocking if this resulted from a call from the constructor (when we read the store and addConsent)
@@ -5,5 +5,6 @@ export const COUNTLY_SETUP_DEFAULTS = { | |||
interval: 5000, | |||
max_events: 500, | |||
queue_size: 1000, | |||
session_update: 60 | |||
session_update: 60, | |||
require_consent: true |
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 was removed by a prior commit which caused countly not to respect consent.
@@ -1,5 +1,6 @@ | |||
declare module 'countly-sdk-nodejs' { | |||
export type CountlyNodeSdk = import('countly-sdk-web').CountlyWebSdk | |||
type missingNodeSdkMethods = 'track_clicks' | 'track_domains' | 'track_forms' | 'track_links' | 'track_scrolls' | 'track_sessions' |
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.
these methods don't exist on the NodeJS SDK
@@ -1,3 +1,7 @@ | |||
|
|||
export type consentTypesExceptAll = 'minimal' | 'performance' | 'ux' | 'feedback' | 'location' | |||
export type consentTypes = 'all' | consentTypesExceptAll | |||
|
|||
export type WithOptional<T, K extends keyof T> = Pick<Partial<T>, K> & Omit<T, K> |
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.
utility type
@@ -0,0 +1 @@ | |||
import './node/NodeMetricsProvider.spec.js' |
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.
only node tests are implemented now because getting the countly UMD module to load properly when running aegir test -t browser
is a pain
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 have some concerns regarding shipping tests as part of dist, I might have a fix on my PR shortly.
|
||
constructor (config: MetricsProviderConstructorOptions<T>) { | ||
const serviceConfig = { | ||
...COUNTLY_SETUP_DEFAULTS, | ||
...config | ||
...config, | ||
app_key: config.appKey |
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.
oh man!
url, | ||
require_consent: true | ||
}) | ||
this.storageProvider = storageProvider ?? null | ||
|
||
this.metricsService.init(serviceConfig) |
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.
well there needs to be defaults in place in order to ship better defaults. What's the alternative you're thinking about?
this.setStore = options.setStore ?? this.setStore | ||
this.getStore = options.getStore ?? this.setStore |
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 am not sure what ?? self
is achieving here.
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.
if (options.setStore) {
this.setStore = options.setStore
}
otherwise, throwing an error that the method isn't implemented. It allows someone to build a StorageProvider "implementation" without needing to define both. Extending StorageProvider, or implementing it, does not allow for constructing of a StorageProvider that can be passed to MetricsProvider, which means you can't build a storageProvider that has no setStore
function.
With this constructor, you can build such a storageProvider.
It's mainly for testing the usecase you previously brought up, but also for flexibility of the unknown. It's not unsafe and is supporting of various futures.
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.
unblock, with skipping test in dist
confirmed it's working with ipfs-companion, merging |
## [1.2.0](v1.1.3...v1.2.0) (2023-01-21) ### Features * use localStorage to cache consent in browsers ([#64](#64)) ([5fd14ce](5fd14ce))
🎉 This PR is included in version 1.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
* main: chore(release): 1.2.0 [skip ci] feat: use localStorage to cache consent in browsers (#64)
depends on #63