-
Notifications
You must be signed in to change notification settings - Fork 560
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
[WIP] Refactor: Try middleware for analytics #2001
Conversation
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 is really promising. I think that we should get these actions added. Most of them will be useful as we move state to redux and the simperium middleware.
lib/state/analytics/middleware.ts
Outdated
switch (action.type) { | ||
case 'AUTH_SET': | ||
if (action.status === 'authorized') { | ||
// @todo called twice for some reason |
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.
which is called twice? AUTH_SET
, or AUTH_SET
with status === 'authorized'
? because AUTH_SET
should be called multiple times in succession throughout the sign-in process
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.
huh it WAS being called twice with status authorized, but maybe no longer..
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 find out why if it's being called with with authorized
- that wouldn't likely have been changed in years
|
||
export const middleware: S.Middleware = store => { | ||
return next => (action: A.ActionType) => { | ||
const result = next(action); |
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.
it should be handled in our analytics code itself, but this is a great place to check for the analytics toggle and skip the middleware if analytics are turned off
const result = next( action );
if ( ! window.sendAnalytics ) {
return result;
}
lib/state/analytics/middleware.ts
Outdated
export const middleware: S.Middleware = store => { | ||
return next => (action: A.ActionType) => { | ||
const result = next(action); | ||
const nextState = store.getState(); |
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.
note that unless we need to read data from state this shouldn't be necessary
// @todo this gets called for each character typed, | ||
// possibly also happening in develop? | ||
analytics.tracks.recordEvent('list_notes_searched'); | ||
} |
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 trap FILTER_NOTES
instead if we prefer. we could either get the search query from state or we could add the searched-for query into the FILTER_NOTES
action itself
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.
FILTER_NOTES seems even worse, as it gets called in lots of places not triggered by searching 🤔
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.
ah okay. we're specifically trying to see every time someone enters a character into the search field…
s
so
som
some
somet
someth
somethi
somethin
something
this could be a decent place for a debounce possibly unless we want the numbers to be grossly inflated, which of course I'm guessing they already are…
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.
yeah, that was my "possibly also happening in develop?" comment. I am pretty sure this is, intentionally or not, what this event is already tracking
… that don't have their own action
Done in #2223 |
Fix
My attempt at centralizing the tracks calls to one place in the app
Test
Add a
console.log
inrecordEvent
and make sure the correct events are firing in response to actions taken throughout the app.Release
TBD