feat: use localStorage to cache consent in browsers #64

SgtPooki · 2023-01-19T00:55:06Z

feat: save consent in localStorage for browsers

depends on #63

SgtPooki

self review

src/MetricsProvider.ts

SgtPooki · 2023-01-19T01:15:42Z

src/BrowserStorageProvider.ts

@@ -0,0 +1,28 @@
+import type { StorageProvider } from './types/index.js'
+
+export const BrowserStorageProvider: StorageProvider = {


We are only providing storage provider for non-node because that's the only one we currently need.

whizzzkid

Some nits.

src/BrowserStorageProvider.ts

src/MetricsProvider.ts

src/BrowserStorageProvider.ts

src/MetricsProvider.ts

Co-authored-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>

SgtPooki

self review. ready for review from @whizzzkid and @djmcquillan

SgtPooki · 2023-01-20T06:45:56Z

.aegir.js

+        window: 'globalThis',
+        global: 'globalThis',


fix for our code in general, but I was attempting to solve testing the browserMetricsProvider and could not get it to play nice.

SgtPooki · 2023-01-20T06:46:09Z

package.json

+      "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"


renamed the files to match the classes.

SgtPooki · 2023-01-20T06:47:02Z

src/BrowserMetricsProvider.ts

+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


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.

SgtPooki · 2023-01-20T06:49:02Z

src/BrowserStorageProvider.ts

+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']
+  }
+}


pretty much the same as in the earlier PR, but using a class

SgtPooki · 2023-01-20T06:49:47Z

src/MetricsProvider.ts

 }

-export default class MetricsProvider<T extends CountlyWebSdk & CountlyNodeSdk> {
+export default class MetricsProvider<T extends CountlyWebSdk | CountlyNodeSdk> {


either or, not both.

SgtPooki · 2023-01-20T06:54:15Z

src/MetricsProvider.ts

+    if (this.storageProvider != null && this.initDone) {
+      this.storageProvider.setStore(Array.from(this._consentGranted))
+    }


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)

SgtPooki · 2023-01-20T06:55:15Z

src/config.ts

@@ -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


This was removed by a prior commit which caused countly not to respect consent.

SgtPooki · 2023-01-20T06:56:01Z

src/types/countlyNode.d.ts

@@ -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'


these methods don't exist on the NodeJS SDK

SgtPooki · 2023-01-20T06:56:22Z

src/types/index.d.ts

@@ -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>


utility type

SgtPooki · 2023-01-20T06:57:18Z

test/node.spec.ts

@@ -0,0 +1 @@
+import './node/NodeMetricsProvider.spec.js'


only node tests are implemented now because getting the countly UMD module to load properly when running aegir test -t browser is a pain

whizzzkid

I have some concerns regarding shipping tests as part of dist, I might have a fix on my PR shortly.

whizzzkid · 2023-01-20T07:36:04Z

src/MetricsProvider.ts


  constructor (config: MetricsProviderConstructorOptions<T>) {
    const serviceConfig = {
      ...COUNTLY_SETUP_DEFAULTS,
-      ...config
+      ...config,
+      app_key: config.appKey


whizzzkid · 2023-01-20T07:37:05Z

src/MetricsProvider.ts

-      url,
-      require_consent: true
-    })
+    this.storageProvider = storageProvider ?? null

    this.metricsService.init(serviceConfig)


well there needs to be defaults in place in order to ship better defaults. What's the alternative you're thinking about?

whizzzkid · 2023-01-20T07:39:12Z

src/StorageProvider.ts

+    this.setStore = options.setStore ?? this.setStore
+    this.getStore = options.getStore ?? this.setStore


I am not sure what ?? self is achieving here.

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.

tsconfig.json

whizzzkid

unblock, with skipping test in dist

SgtPooki · 2023-01-21T00:25:58Z

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))

github-actions · 2023-01-21T00:33:30Z

🎉 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)

SgtPooki requested review from whizzzkid and 0xDanomite January 19, 2023 00:55

SgtPooki added 2 commits January 18, 2023 17:09

feat: save consent in localStorage for browsers

a82c0f6

Merge branch 'main' into feat/useStorageForBrowsers

0524e74

SgtPooki force-pushed the feat/useStorageForBrowsers branch from f25e892 to 0524e74 Compare January 19, 2023 01:13

SgtPooki commented Jan 19, 2023

View reviewed changes

chore: fix linter

d8d6326

SgtPooki self-assigned this Jan 19, 2023

0xDanomite approved these changes Jan 19, 2023

View reviewed changes

This was referenced Jan 19, 2023

feat: add telemetry pln-planning-tools/Starmap#272

Merged

Feature: Ensure we have opt-out metrics for all of our projects ipfs/ipfs-gui#105

Closed

whizzzkid requested changes Jan 19, 2023

View reviewed changes

SgtPooki and others added 12 commits January 18, 2023 21:09

Update src/MetricsProvider.ts

2cd942e

Co-authored-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>

Update src/MetricsProvider.ts

20920d6

Co-authored-by: Nishant Arora <1895906+whizzzkid@users.noreply.github.com>

chore: fix lint

d14d52a

fix: use globalThis instead of window

1e68898

feat: create StorageProvider class

cc19b56

chore: enable testing

0263567

fix: types and construction

84ee0e2

chore: add StorageProvider class

8334d47

fix: build artifacts dont use this

3e9c2a7

feat: add tests

57636b2

fix: code cleanup and issues with serviceConfig

f01a86b

fix: tests cleanup

77a95c5

SgtPooki commented Jan 20, 2023

View reviewed changes

SgtPooki requested review from whizzzkid and 0xDanomite January 20, 2023 07:30

chore: remove unused test scripts

af2b67d

whizzzkid reviewed Jan 20, 2023

View reviewed changes

fix: remove dist/test from npm package

ef9d4cc

whizzzkid approved these changes Jan 20, 2023

View reviewed changes

0xDanomite approved these changes Jan 21, 2023

View reviewed changes

SgtPooki merged commit 5fd14ce into main Jan 21, 2023

SgtPooki deleted the feat/useStorageForBrowsers branch January 21, 2023 00:26

github-actions bot pushed a commit that referenced this pull request Jan 21, 2023

chore(release): 1.2.0 [skip ci]

e4c5d2e

## [1.2.0](v1.1.3...v1.2.0) (2023-01-21) ### Features * use localStorage to cache consent in browsers ([#64](#64)) ([5fd14ce](5fd14ce))

github-actions bot added the released label Jan 21, 2023

whizzzkid added a commit that referenced this pull request Jan 21, 2023

Merge branch 'main' into feat/accumulate-events

a28bc36

* main: chore(release): 1.2.0 [skip ci] feat: use localStorage to cache consent in browsers (#64)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: use localStorage to cache consent in browsers #64

feat: use localStorage to cache consent in browsers #64

SgtPooki commented Jan 19, 2023 •

edited

SgtPooki left a comment

SgtPooki Jan 19, 2023

whizzzkid left a comment

SgtPooki left a comment

SgtPooki Jan 20, 2023

SgtPooki Jan 20, 2023

SgtPooki Jan 20, 2023

SgtPooki Jan 20, 2023

SgtPooki Jan 20, 2023

SgtPooki Jan 20, 2023

SgtPooki Jan 20, 2023

SgtPooki Jan 20, 2023

SgtPooki Jan 20, 2023

SgtPooki Jan 20, 2023

whizzzkid left a comment

whizzzkid Jan 20, 2023

whizzzkid Jan 20, 2023

whizzzkid Jan 20, 2023

SgtPooki Jan 20, 2023 •

edited

whizzzkid left a comment

SgtPooki commented Jan 21, 2023

github-actions bot commented Jan 21, 2023

		@@ -0,0 +1,28 @@
		import type { StorageProvider } from './types/index.js'

		export const BrowserStorageProvider: StorageProvider = {

		this.setStore = options.setStore ?? this.setStore
		this.getStore = options.getStore ?? this.setStore

feat: use localStorage to cache consent in browsers #64

feat: use localStorage to cache consent in browsers #64

Conversation

SgtPooki commented Jan 19, 2023 • edited

SgtPooki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whizzzkid left a comment

Choose a reason for hiding this comment

SgtPooki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whizzzkid left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SgtPooki Jan 20, 2023 • edited

Choose a reason for hiding this comment

whizzzkid left a comment

Choose a reason for hiding this comment

SgtPooki commented Jan 21, 2023

github-actions bot commented Jan 21, 2023

SgtPooki commented Jan 19, 2023 •

edited

SgtPooki Jan 20, 2023 •

edited