Expand useTracking API to accept contextual data

[bgergen]

This PR could probably use more refinement, but I want to get some early feedback on it. It addresses issue #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.

[tizmagik]

This is amazing, thank you!

Also for your question about the API:

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

I actually quite like that idea, I think it's kind of brilliant! It's helpful because it makes "opting-out" of adding the <Track /> component fairly explicit. But two concerns:

1. Do you think this API would be awkward? It works for useState() because in 99% of cases you're definitely going to be pulling off at least the current state (first item in the tuple array), but for tracking the case where you don't need to add contextual tracking data can be fairly common (e.g. at least at all leaf nodes). What do you think?
2. This would be a breaking change to the existing API. Right now we return an object, this would cause us to return an array. Maybe not a big deal? But it does have me wondering if we should introduce this Hooks API in a backwards compatible way and then in a future semver major release start returning an array instead, especially if we hear of folks forgetting to include <Track /> and being confused why they're missing contextual tracking info? What do you think?

[bgergen]

@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!

[tizmagik]

This is brilliant, thanks for doing this!

Two minor things I commented on inline, but I leave it up to you -- I'm fine merging as-is.

One other thing we'll need to do is add some new docs around this. Happy to leave that for a follow-on PR or just create an issue but we'll def need a new section relatively high up on the current README explaining the new Hooks-only capability.

[bgergen]

@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]

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]

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.

[tizmagik]

Great! One minor thing then I think we can ship this! 💪

[tizmagik]

Maybe click it a couple times and wrap in act() for good measure? Would be great to also assert that the state is actually updated to confirm.

[bgergen]

Great call on this. Adding a couple extra clicks exposed a bug. I decided not to wrap the clicks in act() because it wasn't making a difference in the result, and it turns out that's because as of React 16.8, if we use mount, enzyme wraps calls to .simulate in act for us. I think we just needed it in the other spot to handle the asynchronicity.

I still want to do some manual testing again before we merge this just to be sure everything is working properly. Should we aim for a Monday merge?