-
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
fix: Improving Metrics Provider #9
Conversation
Thank you for submitting this PR!
Getting other community members to do a review would be great help too on complex PRs (you can ask in the chats/forums). If you are unsure about something, just leave us a comment.
We currently aim to provide initial feedback/triaging within two business days. Please keep an eye on any labelling actions, as these will indicate priorities and status of your contribution. |
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.
explainers
src/MetricsProvider.ts
Outdated
private readonly minimalEvents: eventTypes[] = ['sessions', 'views']; | ||
private readonly marketingEvents: eventTypes[] = ['attribution', 'users', 'location']; | ||
private readonly trackingEvents: eventTypes[] = ['events', 'crashes', 'apm']; | ||
private readonly performanceEvents: eventTypes[] = ['scrolls', 'clicks', 'forms', 'star-rating', 'feedback']; | ||
private readonly groupedFeatures: Record<consentTypes, eventTypes[]> = { | ||
all: [ | ||
...this.marketingEvents, | ||
...this.minimalEvents, | ||
...this.performanceEvents, | ||
...this.trackingEvents | ||
], | ||
marketing: this.marketingEvents, | ||
minimal: this.minimalEvents, | ||
performance: this.performanceEvents, | ||
tracking: this.trackingEvents, | ||
} |
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 can build this more efficiently this way instead of building on fly.
src/MetricsProvider.ts
Outdated
} | ||
|
||
private sessionStarted: boolean = false | ||
private _consentGranted: Set<consentTypes> = new Set() |
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 keeps track of consents granted.
@@ -1,2 +1,2 @@ | |||
export * from './components/index.js' | |||
export * from './CountlyMetrics.js' | |||
export * from './MetricsProvider.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.
rename to classname
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.
@whizzzkid how do you feel about this being a class rather than a set of functions? I ended up migrating the solution @djmcquillan put together to a class while debugging an issue that lead me to Countly/countly-sdk-web#337, but functions may be easier to consume by react
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 would rather have this as a namespace, something like:
import { * as ReactComponents } from "./components/index";
import MetricsProvider from "./MetricsProvider";
export namespace IgniteMetrics {
export const MetricsProvider = MetricsProvider;
export const ReactComponents = ReactComponents;
The reason I did not touch this as part of this PR was the API not being final yet, once we do that, it would be better suited as a module namespace.
src/types/countly.d.ts
Outdated
export type eventTypes = 'apm' | 'attribution' | 'clicks' | 'crashes' | 'events' | 'feedback' | 'forms' | ||
| 'location' | 'scrolls' | 'sessions' | 'star-rating' | 'users' | 'views' | ||
export type consentTypes = 'all' | 'minimal' | 'marketing' | 'tracking' | 'performance'; |
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.
stronger literal types instead of just strings.
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 one thing I would block on: checkConsent. see comment
"react": "^18.2.0", | ||
"react-dom": "^18.2.0", | ||
"react-router-dom": "^6.4.5", | ||
"react-router-dom": "^6.5.0", |
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.
nit: these should probably be optional dependencies, but we can address later.
Co-authored-by: Russell Dempsey <[email protected]>
Improves: #1
In this PR: