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

`tracking` prop on DOM element not serialized from decorated component #139

Closed smitch88 closed 4 years ago

smitch88 commented 4 years ago

I'm not entirely sure if this is on purpose or not but I am seeing tracking="[object Object]" on the rendered DOM element.

Screen Shot 2019-10-01 at 11 03 13 AM

The code seems a bit weird, is there a reason why this.tracking, https://github.com/nytimes/react-tracking/blob/master/src/withTrackingComponentDecorator.js#L116 and https://github.com/nytimes/react-tracking/blob/master/src/withTrackingComponentDecorator.js#L39, is being forwarded if its not even serialized for inspection purposes assuming thats what its for.

tizmagik commented 4 years ago

Hmm, it's not meant to be a rendered DOM attribute. Could you share what your setup/code looks like that reproduced this?

smitch88 commented 4 years ago

I cannot share the code as its proprietary but just given the way the HOC is putting this.tracking into tracking as a prop I would imagine its the case for every implementation. https://github.com/nytimes/react-tracking/blob/master/src/withTrackingComponentDecorator.js#L116

If it helps the implementation is using decorators on es6 classes versus hooks or props.

tizmagik commented 4 years ago

Yea, sorry unless I can see a snippet of what you're doing it's hard for me to diagnose your issue, but it's likely incorrect usage since the tracking prop shouldn't be placed directly on DOM elements. You might be spreading props while rendering DOM elements, which would cause this issue, in that case, you'd want to explicitly exclude the tracking prop.

For example, we at NYT make heavy use of react-tracking for most of our components, but if you query for any DOM elements with a tracking prop in the dev console on nytimes.com you'll see there's no such elements:

document.querySelectorAll('[tracking]')

smitch88 commented 4 years ago

We are not doing anything out of the ordinary I believe,

Root component has our defined handler for capturing events,

@track(top-level handler fn)
class Root extends Component {}

// Our page level components have @track with mount phase events fired as "impressions"
@track(props => // code to pull out some props to merge into an upstream event obj)
class Page extends Component {
  @track(props => ..) // capture page impression
   componentDidMount(){
      // noop basically unless we have functionality here
   }
}

// Our event handlers follow the same pattern
class Button extends Component {
  @track(props => ..) // capture click event
   handleOnClick(e){
      // typical overwriting onClick to bake in the tracking + regular onClick call
      if(this.props.onClick){
          this.props.onClick(e)
      }
   }
}

Those are the 3 usecases we use and how we use them with @track. The code in that higher order component is putting the this.tracking onto the HOC so its the component spreading it on there. We have no control on our side to eliminate that do we?

Let me try isolate an example to see if its continues to do this.

smitch88 commented 4 years ago

I think this must be an issue with some spreading of props. I think that is enough for me to go back and look at our implementation and ensure we are not needlessly forwarding tracking.

Thanks for your help @tizmagik. Close out the issue as you see fit. Im sure its on our implementation side given no other issues noted about this.

tizmagik commented 4 years ago

Sure, happy to help! Feel free to open a new issue if you notice anything else.

Also, by the way, you can avoid writing componentDidMount if all you need it for is to dispatch a tracking event when the component mounts; react-tracking provides a dispatchOnMount option as part of the second arg in the decorator exactly for this purpose :)

@track({ ... }, { dispatchOnMount: true })
class Page extends Component { ... }