Set user #52
Set user #52
Conversation
| @@ -420,7 +420,7 @@ module.exports = { | |||
| 'import/exports-last': 'off', | |||
| 'import/group-exports': 'off', | |||
| 'import/no-cycle': 'error', | |||
| 'import/no-default-export': 'error', | |||
| 'import/no-default-export': 'off', | |||
There was a problem hiding this comment.
We were already ignoring this rule elsewhere.
| @@ -1,5 +1,5 @@ | |||
| /* @flow */ | |||
|
|
|||
| import type { MuseGlobalType } from './types'; | |||
| import type { MuseGlobalType } from './lib/types'; | |||
There was a problem hiding this comment.
would it make sense to create a types folder and put MuseGlobalType in index.js for now until we have more types? When I think about lib usually I think helper libraries and things like that. I think types should be in a folder of its own.
There was a problem hiding this comment.
Agreed, but I'd prefer to do that + additional refactoring in another PR if possible.
src/lib/track.js
Outdated
| // remove null, undefined values | ||
| user.id || delete user.id | ||
| user.email || delete user.email | ||
| user.name || delete user.name |
There was a problem hiding this comment.
The 'delete' statements above are new. The rest of this method is unchanged.
src/lib/get-property-id.js
Outdated
| @@ -0,0 +1,33 @@ | |||
| import { getClientID, getMerchantID } from '@paypal/sdk-client/src'; | |||
|
|
|||
| export const getPropertyId = ({ paramsToPropertyIdUrl }) => { | |||
There was a problem hiding this comment.
This method is unchanged.
| @@ -26,3 +33,39 @@ export const removeFromCart = (items, currentItems = []) => { | |||
| export const addToCart = (items, currentItems = []) => { | |||
| return [ ...currentItems, ...items ]; | |||
| }; | |||
| // $FlowFixMe | |||
| export const composeCart = (type, data) => { | |||
There was a problem hiding this comment.
This method now uses 'constants' imported above. Otherwise unchanged.
| @@ -21,3 +22,12 @@ export const setCookie = (cookieName : string, cookieValue : string, expirationM | |||
| const expires = `expires=${ d.toUTCString() }`; | |||
| document.cookie = `${ cookieName }=${ cookieValue }; Path=/; ${ expires }`; | |||
| }; | |||
|
|
|||
| export const getUserIdCookie = () : ?string => { | |||
There was a problem hiding this comment.
These methods were removed from 'tracker-component'. They have not been altered. (note: ONE_MONTH_IN_MILLISECONDS should probably be in constants)
| @@ -0,0 +1,16 @@ | |||
| /* @flow */ | |||
| export default { | |||
| 'sevenDays': 6.048e+8, | |||
There was a problem hiding this comment.
Personally, (nitpick) I think its easier to understand if you wrote it like this: sevenDays: 7*24*60*60*1000
😆
src/lib/track.js
Outdated
| }; | ||
|
|
||
| // remove null, undefined values | ||
| user.id || delete user.id |
There was a problem hiding this comment.
this is impossible right? Because right above you are setting userIdCookie
src/lib/track.js
Outdated
| // remove null, undefined values | ||
| user.id || delete user.id | ||
| user.email || delete user.email | ||
| user.name || delete user.name |
There was a problem hiding this comment.
I thought setting email and name field to null or '' will reset it on our back end? We want these values to get passed down the pipeline to get processed as is, so maybe we should not delete these fields?
| accessTokenUrl, | ||
| storage, | ||
| defaultTrackerConfig | ||
| } = constants; |
There was a problem hiding this comment.
why not just import { accessokenUrl .... } from './lib/constants'
There was a problem hiding this comment.
Each key would have to be exported separately in that case.
| const track = <T>(config : Config, trackingType : TrackingType, trackingData : T) => { | ||
| export const clearTrackQueue = (config : Config) => { | ||
| // $FlowFixMe | ||
| return trackEventQueue.filter(([ trackingType, trackingData ]) => { // eslint-disable-line array-callback-return |
There was a problem hiding this comment.
nitpick: much cleaner to use a forEach and then return [] imo
There was a problem hiding this comment.
Agreed. This was Zach's original implementation, and looks like it's just moved in the file. Maybe we can add a TODO instead to change this method?
| export const clearTrackQueue = (config : Config) => { | ||
| // $FlowFixMe | ||
| return trackEventQueue.filter(([ trackingType, trackingData ]) => { // eslint-disable-line array-callback-return | ||
| track(config, trackingType, trackingData); |
There was a problem hiding this comment.
question, does track also fire off JL events?
There was a problem hiding this comment.
It uses a different 'tracker' method. See "trackEventByType" & "getJetlore".
| const track = <T>(config : Config, trackingType : TrackingType, trackingData : T) => { | ||
| export const clearTrackQueue = (config : Config) => { | ||
| // $FlowFixMe | ||
| return trackEventQueue.filter(([ trackingType, trackingData ]) => { // eslint-disable-line array-callback-return |
There was a problem hiding this comment.
Agreed. This was Zach's original implementation, and looks like it's just moved in the file. Maybe we can add a TODO instead to change this method?
M-OC
force-pushed
the
setUser
branch
2 times, most recently
from
20c5154 to
5670038
Compare
{user: {email: 'foo'}}and{email: 'foo'}