Add new useTracking React hook

[damassi]

Closes #120

Note

This is an internal pattern that we're currently rolling out to devs on our team and have yet to use extensively. Wanted to open up a PR for feedback and other suggestions.

Overview

This PR adds a new useTracking hook now that #118 has been implemented.

Usage

// App.js 

import { track } from 'react-tracking'
import { Child } from './Child'

// Inject a tracking context into the tree
const App = track({
 foo: 'bar'
})(props => {
  return <Child />
})

And then the parent and children can pull it out via useTracking

import { useTracking } from 'react-tracking'

export const Child = () => {
  const tracking = useTracking()  

  return <div onClick={() => tracking.trackEvent(...)}>
}

Since Hooks are only supported in React 16.8+, this requires a major version bump as it's a breaking change. Which leads to the question: should decorators be deprecated inside of react-tracking? The decorator proposal is still Stage II, and doesn't seem to be going anywhere.

[juliusdelta]

Fwiw I'm for this change. My team and I have been using react-tracking for about about 9 months and have a complete tracking system with it. We've also been writing about half our new components with hooks, so something like this would be really useful for us.

We haven't upgraded to version 6.0 as of yet but I anticipate it will happen in the coming weeks.

[tizmagik]

Thanks so much for this PR @damassi ! Will spend some more time reviewing and playing around with it.

At a high level it looks great, but I'm wondering what your thoughts are around avoiding having to wrap the component in the @track() HoC? Can we implement the same API entirely using hooks? The goal being to reimagine the react-tracking API into more idiomatic hooks usage rather than just "connecting" the two APIs.

Specifically the current decorator/HoC API allows for:

1. Adding additional tracking context to the tree as we go
2. Conditionally dispatching on mount

I haven't had a chance to really think through this design, but wanted to spit-ball some ideas with you. What are your thoughts on doing something like this:

( @juliusdelta your thoughts on this API would be greatly appreciated as well! )

export const Child = ({ name }) => {
  const tracking = useTracking(
    {
      // (1) To support adding additional tracking context
      // this object gets added to tracking context,
      // so that any sub-components that Child renders get this data.
      // Is this even possible? Or do we need to wrap/render something in the tree to add to its context?
      childName: name,
      additional: "properties",
      could: "go here"
    },

    // (2) To support dispatching when a component mounts/renders (for the first time)
    // is "dispatchOnMount" terminology confusing with hooks usage?
    // is it better to be consistent with the "legacy" API or is there a better name here?
    { dispatchOnMount: contextData => true }
  );

  return <div onClick={() => tracking.trackEvent(/* ... */)} />;
};

But, is that dispatchOnMount option a worthwhile abstraction? Thinking through, if we don't provide something like that, then user-land would either need to wrap in a HoC (which mixes APIs and could be confusing), or would need to use useEffect hook to reproduce the functionality -- and have to deal with tracking being in the dependency array of the hook. Here's what that looks like:

export const ChildManualDispatchOnMount = ({ name }) => {
  const tracking = useTracking({ childName: name });

  // dispatchOnMount "manually"
  useEffect(
    () => {
      // if it depends on contextual data, grab it first
      const contextData = tracking.getTrackingData();

      if (contextData.someThing) {
        tracking.trackEvent({ mounted: "data" });
      }
    },
    // note that "[]" is here to only fire once when the component mounts
    // but leaving it empty causes React to warn:
    //    React Hook useEffect has a missing dependency: 'tracking'.
    //    Either include it or remove the dependency array. (react-hooks/exhaustive-deps)
    // but if we do include `tracking` then this will fire repeatedly because
    // (at least for this current PR) we get a new object every time: https://github.com/nytimes/react-tracking/pull/124/files#diff-06fcede0ecd25735ce64c15bd39106f4R16
    // 
    // So maybe it's better to abstract this away as shown above?
    []
  );

  return <div onClick={() => tracking.trackEvent(/* ... */)} />;
};

---

should decorators be deprecated inside of react-tracking? The decorator proposal is still Stage II, and doesn't seem to be going anywhere.

Good question. What would it mean to deprecate them inside of react-tracking? In user-land if folks don't want to use decorators, they don't have to and can just compose track() calls with other HoC. This PR, introducing useTracking would make that approach much more practical, which is great!

[damassi]

@tizmagik - IMO HOC's are an abstraction that isn't going anywhere, and using them isn't mutually exclusive to hooks. I think it's perfectly reasonable to keep these things combined. It's also clear what's going on ("inject tracking context, which can then be pulled from the tree via a hook") with low complexity and backwards compatibility.

In a version that we sketched out before opening this pr we had a function named provideContext, which injected the context in in the same way that track does now. Maybe it's just a naming issue? But upon review we decided that was just more overhead and wanted keep things simple and 1:1 via patterns already established by this lib.

[tizmagik]

Thanks @damassi that makes sense, I can totally see the argument for keeping it simple. The only other consideration for this API would be around the ergonomics of using the hooks API with the HoC API (you then have two "tracking" objects with similar interfaces, props.tracking from the HoC and tracking from the useTracking hook). Maybe that's okay for now and we can let this live out in the wild for a bit and see if a cleaner API/abstraction presents itself later.

[damassi]

One thought about the hook API is that we could destructure, which would fairly clearly create a separation between the two if updated in the docs:

const { trackEvent } = useTracking()

But either way, my thinking was to ensure that the API was the same between the HOC and the hook so that users who wanted to unwrap / flatten-out child components and only rely on the hook could easily do so without needing to refactor inner-component logic.

const WrappedChild = ({ tracking }) => {
  ... 
  tracking.trackEvent({ ... })
}

export default Child = track(WrappedChild)

becomes

const Child = props => {
  const tracking = useTracking()
  ... 
  tracking.trackEvent({ ... })
}

[tizmagik]

A small note on potentially using useMemo or useRef, otherwise resolving the conflict with master and then I think we can get this released as 7.0.0

[damassi]

Sounds good! Updated with useCallback as that better suits the use-case here since it accepts any kind of function as an argument.

[tizmagik]

Awesome! Thank you 🙏

[tizmagik]

Released as v7.0.0 🎉 Please holler (or open a new issue) if you see any problems.

[alloy]

Nice one, @damassi ✨

[tizmagik]

Heads up, I'm seeing build issues with v7.0.0. I re-opened this issue: #119 (comment)

Are you seeing any problems in your app @alloy @damassi ?

[damassi]

Ah yup, throwing this error, per #119:

Cannot find module 'core-js/modules/es.object.define-property' from 'index.js'

[tizmagik]

Ah sorry about that. Ok prepping a revert PR and will publish as 7.0.1

[tizmagik]

Hm @damassi would you mind testing out updates to your core-js and babel config to see if it gets around your module errors? Specifically: #119 (comment)

I'm wondering if that means folks who are already on core-js@3 if they'll start seeing errors if we revert that upgrade.

(Also wondering if the proper solution here is using microbundle or something 😅 )

[damassi]

(👍 Will comment over in #119)

[tizmagik]

@damassi I think we don’t need ReactTrackingContext to be exported as part of this hooks support, right? Just wanted to double check because I plan on removing that export in a separate PR.

[damassi]

Nope, not necessary! I left it as a main export because it could be useful for other folks to build tooling on top of, but if they need it they could dig into a nested path.