Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

override default useTracking behaviour #166

Closed
huguesbr opened this issue Oct 19, 2020 · 4 comments
Closed

override default useTracking behaviour #166

huguesbr opened this issue Oct 19, 2020 · 4 comments

Comments

@huguesbr
Copy link

huguesbr commented Oct 19, 2020

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?

@huguesbr huguesbr changed the title override default useTracking behaviour override default useTracking behaviour Oct 19, 2020
@tizmagik
Copy link
Collaborator

tizmagik commented Oct 19, 2020

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
Copy link
Author

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
Copy link
Collaborator

tizmagik commented Dec 2, 2020

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

@huguesbr
Copy link
Author

huguesbr commented Dec 2, 2020

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!

@huguesbr huguesbr closed this as completed Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants