Skip to content
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

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

Open
guanw opened this issue Jun 26, 2020 · 8 comments
Open

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

guanw opened this issue Jun 26, 2020 · 8 comments
Labels

Comments

@guanw
Copy link

guanw commented Jun 26, 2020

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
Copy link
Author

guanw commented Jul 7, 2020

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

@tizmagik
Copy link
Collaborator

tizmagik commented Jul 8, 2020

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
Copy link
Author

guanw commented Jul 8, 2020

@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
Copy link
Collaborator

tizmagik commented Jul 8, 2020

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
Copy link
Author

guanw commented Jul 8, 2020

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
Copy link
Author

guanw commented Jul 8, 2020

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
Copy link
Author

guanw commented Jul 9, 2020

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
Copy link
Collaborator

tizmagik commented Jul 9, 2020

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants