[C-843] Native analytics sagas#1734
Conversation
dylanjeffers
left a comment
There was a problem hiding this comment.
all makes good sense, nice job
| PushNotifications.setWebRef(webRef) | ||
| }, [webRef]) | ||
|
|
||
| useEffect(() => { |
| import { Linking, Pressable } from 'react-native' | ||
|
|
||
| import { ToastContext } from 'app/components/toast/ToastContext' | ||
| import { make, track } from 'app/services/analytics' |
There was a problem hiding this comment.
under services makes much more sense
| const sagas = [ | ||
| // config | ||
| ...backendSagas(), | ||
| ...analyticsSagas(), |
There was a problem hiding this comment.
cool! so we want to have these around for now then?
There was a problem hiding this comment.
Yeah, they aren't touching any state and are very small so I think it's ok to have them
| recordAnalytics: ( | ||
| event: string, | ||
| properties?: Record<string, any>, | ||
| event: { eventName: string; properties?: Record<string, any> }, |
There was a problem hiding this comment.
do we want to use Record<string, unknown> i think that is technically the best version of "any object"
There was a problem hiding this comment.
Yup, can actually use AnalyticsEvent here
| selectedAs: 'primary', | ||
| reason | ||
| recordAnalytics({ | ||
| eventName: Name.CREATOR_NODE_SELECTION, |
There was a problem hiding this comment.
wow epic job changing all these
There was a problem hiding this comment.
Thanks! idk if this is the final interface we should use but updating web was the quickest route
| analytics.track, | ||
| { | ||
| eventName, | ||
| properties: omit(properties, 'type') |
There was a problem hiding this comment.
awesome stuff, so do we just want to omit type in the destructuring?
There was a problem hiding this comment.
That causes the linter to complain about type not being used so I opted for this instead
| track: (event: AnalyticsEvent, callback?: () => void) => Promise<void> | ||
| identify: ( | ||
| handle: string, | ||
| traits?: Record<string, any>, |
There was a problem hiding this comment.
same here just regarding <string, any> vs <string, unknown>
|
|
||
| import { useModalState } from 'common/hooks/useModalState' | ||
| import { getPlaylistLibrary } from 'common/store/account/selectors' | ||
| import { make, useRecord } from 'common/store/analytics/actions' |
There was a problem hiding this comment.
does it makes sense to have useRecord in actions? i guess it's a special action? and looks like that's what was there before anyway so nvm
There was a problem hiding this comment.
No it doesn't make sense, we can address in the other analytics issue I created
| const audiusBackendInstance = yield* getContext('audiusBackendInstance') | ||
| yield* call(waitForBackendSetup) | ||
| yield* call(audiusBackendInstance.markAllNotificationAsViewed) | ||
| if (NATIVE_MOBILE) { |
There was a problem hiding this comment.
i forgot, we still want this yeah?
There was a problem hiding this comment.
Good catch! Yup, I'm getting too eager to remove stuff
| source | ||
| }) | ||
| record( | ||
| make(Name.REWARDS_CLAIM_FINISH_COGNITO_FLOW, { |
There was a problem hiding this comment.
just wondering if make should just be a part of record at some point?
There was a problem hiding this comment.
Definitely think so, can address as part of the other issue
|
Preview this change https://demo.audius.co/sk-c-843-native-analytics-sagas |
|
Preview this change https://demo.audius.co/sk-c-843-native-analytics-sagas |
Description
analyticsobject into storeContextwebAnalyticsSagasthat handle page view tracking (still emitting messages to mobile for now)utils/analyticstoservices/analyticsDragons
Had to make the
analyticsinterface consistent between web and mobile to use in the storeContext. Decided to conform tobecause mobile was already using that format everywhere and calling
trackdirectly, whereas web was mostly using theuseRecordhook so the change only needed to happen in one place. I added an issue to make the readdress the analytics implementation and make it consistent across the board: https://linear.app/audius/issue/C-848/make-analytics-implementation-consistent-across-web-and-mobileHow Has This Been Tested?
Tested analytics on web and mobile including page views and payloads
How will this change be monitored?
For features that are critical or could fail silently please describe the monitoring/alerting being added.
Feature Flags
Are all new features properly feature flagged? Describe added feature flags.