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

override default `useTracking` behaviour #166

Closed huguesbr closed 3 years ago

huguesbr commented 3 years ago

Hi,

We have multiple React App (we're using rails-react), but we have some component shared by all our App (let's say a button) which make usage of tracking.

For some of our App it doesn't make sense to track anything (let's say admin).

We would like to change default behaviour of useTracking which raise if context is not defined, by returning a no-op instead.

const safeUseTracking = () => {
  const trackingContext = useContext(ReactTrackingContext);

  if (!(trackingContext && trackingContext.tracking)) {
    return () => ({
      getTrackingData: () => ({}),
      trackEvent: () => {},
    })
  }

  return useTracking();
}

The issue is ReactTrackingContext is not exposed by the react-tracking

I guess I could do something like

const ReactTrackingContext = require('react-tracking/build/withTrackingComponentDecorator').ReactTrackingContext;

Any reason (beside exposing internal API) is not exposed?

tizmagik commented 3 years ago

Hello @huguesbr -- good question. The reason we throw an error when there's no context in the tree is to warn in the case the user forgot to establish react-tracking context before attempting to dispatch tracking events or to track components within the tree, it helps catch easy to miss cases where components that are expected to be tracked are included just outside the tracked root.

Can you help me understand your use case a bit better? Why not wrap your admin example with a track() higher-order component first, even if you don't need to include any tracking context?

And yeap the reason we don't export the tracking context is to avoid exposing the internal API. In the future, the work in #138 may actually obviate the need for this since using the useTracking() hook without an existing tracking root will transparently provide that tracking root for you.

huguesbr commented 3 years ago

Yes I'm following #138 really closely as the need to define context with functional comp is a core for us.

There is lot of different reasons why we don't want to have top comp defining a context. But the main one is being is being avoiding code where there should be none ^^ An App,we shouldn't track should be able to use a comp that track if within a tracking context?

Our current solution works but really on exposure of context. If useTracking just took an (optional) argument to define is behavior, it would not. (Happy to PR if such direction makes sense)

Also for specs we've found the need to redefine useTracking and tracking function to either return no op (dummyTracking) for useTracking or identity function for tracking HOC in order to avoid dealing with any issue in shallow component (which doesn't trigger child component behaviors -- in our case the actual tested comp) or finding a component by class (which are rename to WithTracking(Comp))

So I think the need to either work in an unexpected context (to increase comp re-usability) or ability to be shortcuted (in context of test) is nice.

But nothing we can't handle ourselves ^^ (so far) so great work!

So please keep an option open for exposing some internal if needed, with warning if unsafe ^^

tizmagik commented 3 years ago

This should be easier to do now with the release of v8.1.0

huguesbr commented 3 years ago

oh my https://github.com/nytimes/react-tracking/releases/tag/v8.1.0 is a gem! full support of react hooks is a blast! congrats!

will close now, thx!