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

Call dispatch with latestProps also #160

Closed adamschoenemann closed 3 years ago

adamschoenemann commented 4 years ago

A small change that'd be very nice. For example, I have a component that takes an API object that communicates with my backend and I want to use this prop to dispatch the tracking data

tizmagik commented 3 years ago

Hey @adamschoenemann thanks for submitting a PR! I think I'm not fully grokking the use case exactly. Don't we already have access to the props like in the advanced usage example?

adamschoenemann commented 3 years ago

You have access to props when creating the track data but not in the dispatch function which actually sends the events to some backend.

tizmagik commented 3 years ago

Ah, I see now! I think I get the use case now, although I'm hesitant to increase the API surface area for a couple reasons.

  1. It's not clear which props should get passed to the dispatch function; the tracking data gets merged all the way up to the top-level tracked component, would users expect that same behavior for props? That could get ugly fast. Do we just send the props at the level of the component that triggered the dispatch? But what if that component is a "dumb" component like a button or otherwise and the real props are higher up? I don't think we'd want to require users to propagate props just so that it's exposed to the dispatch trigger as that goes against the goal of this project (to keep tracking concerns as out of the way as possible).
  2. Even still, this behavior can be achieved in user-land with the existing API. You can attach the props to the trackEvent function and then pluck them off in the dispatch function like so:
/* tracking component */
@track(props => ({ module: 'foo', props }))
class Foo extends Component { ... }

/* dispatch function */
const dispatch = (data) => {
  const { ...trackingData, props } = data;

  // can do whatever with `props`

 // push the rest to the dataLayer
 (window.dataLayer = window.dataLayer || []).push(trackingData);
}

What are your thoughts?

adamschoenemann commented 3 years ago

The use case is quite simple: You have your top-level component on which you define your dispatch function. This top-level component receives some props that you want to access when dispatching. In my case, i have the quite common case where there is an API object that is created at runtime and passed to my top-level component as a prop. This API object ultimately determines where the events should be sent.

  1. Just the props of the component you're tracking
  2. That's true, but imo it's not very elegant. Also, then you'd have to meticulously strip out these props before sending the actual event

In any case, I ended up not using this library anyway, so I won't push this PR much further, but I think it'd be a good addition to the library :)

tizmagik commented 3 years ago

Thanks @adamschoenemann it's an interesting case and maybe something we could consider in the future if this sort of request comes up again. Given that you ended up not using this library (curious, what did you end up using?) I think I'll close this PR for now. Thanks again for the contribution and the discussion though, it's appreciated!