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

Expand useTracking API to accept contextual data #168

Closed bgergen closed 3 years ago

bgergen commented 3 years ago

This PR could probably use more refinement, but I want to get some early feedback on it. It addresses issue https://github.com/nytimes/react-tracking/issues/138. I spent some time considering this API vs having the Track component take in props. I think this is the better option because it allows contextual data to be passed into the hook and get back a trackEvent function that can make use of that data, which would not be possible if the data had to be passed into the Track component instead.

One minor (I think) concern I still have is that it might be too easy for a developer to forget to wrap their markup in the Track component, and mistakenly believe that child components are receiving contextual data when they're not. Open question: Would it make any sense to emulate the useState API, and do something like:

const [Track, tracking] = useTracking(data);

That would at least force the developer to very clearly opt out of using Track by needing to do:

const [, tracking] = useTracking(data);

Maybe that's too heavy handed though. I don't know—something worth thinking about perhaps.

bgergen commented 3 years ago

@tizmagik Yes, I think you're right about that tuple API. It could be a bit awkward, and I let my head drift off and forget that such a change wouldn't be backward compatible. Also, I wouldn't want to make things more complicated in anticipation of a potential issue that we haven't actually seen users have in the wild. If we're hearing that folks are running into that issue then we can reevaluate. In the meantime, I'll make that update to prevent any changes in the HoC API and do some manual testing to make sure everything looks good!

bgergen commented 3 years ago

@tizmagik No prob, it's been fun! Going to make the suggested changes to the tests, and then I think I can take a stab at updating the docs in a separate PR if that seems alright.

bgergen commented 3 years ago

FYI, I found a bug while doing some manual testing. Many of the functions in the hook have a dependency on trackingData, which means that if the consumer passes in an object literal and the component renders more than once, the hook will read it as a new object being passed in and all of the memoization in the hook is worthless. The same is true for the dispatchOnMount function if that's defined inline. I need to figure out the best way to memoize those 2 arguments—feels too onerous to ask the consumer of the hook to do so before passing them in—and write or update a couple of tests to catch this. Probably won't be able to get to that until later in the week though.

tizmagik commented 3 years ago

Ah, good catch. I think this test attempts to get at that (unnecessary renders), but I think wouldn't catch the case you're talking about.