nytimes / react-tracking

🎯 Declarative tracking for React apps.
https://open.nytimes.com/introducing-react-tracking-declarative-tracking-for-react-apps-2c76706bb79a
Other
1.88k stars 123 forks source link

Option to make `dispatch` fired after decorated method is fired #157

Open guanw opened 4 years ago

guanw commented 4 years ago

First of all really appreciate the effort on making this library and it saves me a lot of time to implement my own version.

The way I use react-tracking is like the following:

export function tracking(trackingInfo) {
  const options = {
    dispatch: (data) => {
      // send out data to tracking service
    },
    dispatchOnMount: true
  };
  return function decorator(...toDecorate) {
    const name = annotate(trackingInfo.name);
    return track(Object.assign({}, trackingInfo, { name }), options)(...toDecorate);
  };
}
...

@tracking({ name: 'counter' })
class Counter extends Component {
  @tracking({ name: 'increment' })
  increment = () => {
     // increment count in global redux state
     this.props.dispatch({
       type: 'INCREMENT'
     })
  }
}

i also attach a bunch of metadata from redux store to the optons.dispatch. The problem i'm seeing is that let's say if we have a redux state called count that's going to add 1 everytime increment is fired. react-tracking will always attach the old count as part of metadata instead of count + 1 after increment is fired. I'm assuming this is because react-tracking dispatches first before it actually fires the decorated function? So curious is there anyway around? Thanks and looking forward to any help here!

guanw commented 4 years ago

Or would it be better to add a afterDispatch callback that i can use?

tizmagik commented 4 years ago

Yes, good insight here. I think conceptually it makes sense that the tracking event is fired before the decorated function since syntactically that's how it appears: @track(first)(secondFn = () => {}).

The Codesandbox Example linked to in the main README actually has an example of this (although it's not using redux).

I'm sort of not inclined to adjust this API since I'd be concerned about the added complexity that would introduce, although happy to explore it further with you and see if there's a clean way to approach it. However, with the current API there are some options:

  1. You could manually dispatch at the end of the function, so only decorate the class (not the method) and then call this.props.tracking.trackEvent() at the end of your method. Although I'm not sure if this would actually reflect the updated Redux store yet or not.
  2. If all you need is the current state (so that you can send state.count or state.count + 1) you can use the tracking signature described in the README here.
  3. You could create some custom "redux tracking middleware" that looks for type: 'INCREMENT' actions and sends off tracking events. This sorta goes agains the design pattern of keeping tracking co-located with the component, but it might be a simple enough solution for your use case.

Curious to get your thoughts!

guanw commented 4 years ago

@tizmagik Thanks for replying. Yeah i think if tracking is always gonna happen before dispatch then doing something like @track((props, state) => ({ event: "click", count: state.numClicks })) would be the way to go for me. But i also don't like the idea of specify option to top level component like

// top-level options
  {
    // custom dispatch to console.log in addition to pushing to dataLayer[]
    dispatch: data => {
      console.log(data);
      (window.dataLayer = window.dataLayer || []).push(data);
    }
  }

cause that means people need to add separate piece of code in order to instrument tracking for every project. Like I copied and pasted earlier, the way i did it is expose that single decorator called tracking and developers only need to worry about adding it to their components&methods instead of having to add option to top level component.

Or Is it possible to make track decorator take both

  1. callback function like
    (props, state) => ({
    action: state.following ? "unfollow clicked" : "follow clicked",
    name: props.name
    })

    and

  2. options like
    {
    dispatch,
    dispatchOnMount,
    process,
    forwardRef
    }

at the same time?

tizmagik commented 4 years ago

So the function signature (props, state) => ({ }) is only applicable when decorating class methods (since state is only available within the class instance). The options signature is only applicable when decorating the class itself.

@track((props) => ({ name: props.name || 'Foo' }), { dispatch, dispatchOnMount, process, forwardRef })
class Foo extends Component {
  @track((props, state) => ({ 
    action: state.following ? "unfollow clicked" : "follow clicked"  
    // note that "name" comes from the class-level decorator and is not needed here
  }))
  follow = () => {
    // ... follow logic
  }
}
guanw commented 4 years ago

oh, ic. I think it actually should work with my setup. I will get back to u once i verified. Thanks for the example!

guanw commented 4 years ago

hmmm just did some initial experiments on this. so basically make tracking method as followed:

export function tracking(trackingInfo) {
  if (typeof trackingInfo === 'function') {
    const fn = trackingInfo;
    return function decorator(...toDecorate) {
      return track(fn)(...toDecorate);
    }
  }
  const options = {
    dispatch: (data) => {
      // send out data to tracking service
    },
    dispatchOnMount: true
  };
  return function decorator(...toDecorate) {
    return track(Object.assign({}, trackingInfo), options)(...toDecorate);
  };
}
...
@tracking({ name: 'counter' })
class Counter extends Component {
  @tracking((props, state) => ({ action: 'increment' }))
  increment = () => {
     // increment count in global redux state
     this.props.dispatch({
       type: 'INCREMENT'
     })
  }
}

seems that action: 'increment' is never received on options.dispatch callback tho. I will try to make a reproducible demo later when i got time.

guanw commented 4 years ago

hmmm, i have one more question, I created a small react native project to try out advance feature of react-tracking. Here is a link to the repo: https://github.com/various-playgrounds/expo-tracking-demo

I notice once i add the following commit (i.e the latest commit in that repo) to App.js it will throw maximum call stack error for me, am I doing anything wrong here? image

image

tizmagik commented 4 years ago

If I had to guess, it's probably because the event object has a cyclical reference to itself somewhere in that object. I would pull off only the property that you actually need on event and not try to include the whole object.