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

Don't invoke trackEvent when tracking data is null or undefined #105

Closed williardx closed 5 years ago

williardx commented 5 years ago

This change allows us to conditionally track events depending on the return value of the function passed into @track. If the function returns null or undefined then trackEvent will not be invoked.

We've patched this in our own version of react-tracking in order to only track when a user unchecks a box. Example code is below and can be found here.

  @track((props, state, args) => {
    const willInputBillingAddress = !args[0]
    if (willInputBillingAddress) {
      return {
        action_type: Schema.ActionType.Click,
        subject: Schema.Subject.BNMOUseShippingAddress,
        flow: "buy now",
        type: "checkbox",
      }
    }
  })
  handleChangeHideBillingAddress(hideBillingAddress: boolean) {
    ...
  }
tizmagik commented 5 years ago

@williardx I'm wondering if this should be considered a breaking change. E.g. I can imagine someone expecting the following to just work, although probably unlikely (since they'd typically have some sort of { event: "click" } in the handleClick decorator).

@track({ module: 'button' })
class Thing extends Component {
  @track()
  handleClick = () => { ... }

  render() {
    return <button onClick={this.handleClick}>Click me</button>
  }
}

Thoughts?

williardx commented 5 years ago

If I'm reading things correctly, wouldn't that result in calling trackEvent with an empty object since that's the default value for trackingData?

tizmagik commented 5 years ago

It would result in { module: 'button' } being dispatched, since that's defined on context by the decorator on the class.

So while this is technically incorrect usage of react-tracking API, I could see how someone might do the following:

@track({ module: 'button', event: 'click' })
class Thing extends Component {
  @track()
  handleClick = () => { ... }

  render() {
    return <button onClick={this.handleClick}>Click me</button>
  }
}

And actually { module: 'button', event: 'click' } would get dispatched when the button is clicked. So this change may break that, although, again it's kind of incorrect usage of the intended API so I'm leaning towards just making this a minor rather than breaking change.

alloy commented 5 years ago

I think @williardx means that because the default value of trackingData for the method decorator is {} it will still get dispatched. Only examples such as these would not dispatch: @track(null)

tizmagik commented 5 years ago

Oh, yes, I see, I think you're right, because we default it here: https://github.com/NYTimes/react-tracking/blob/aa5fc1aa2a8a871a1b419a5eefbfc38c5b50d77a/src/trackEventMethodDecorator.js#L3

Okay, I feel better about a minor release for this 😁

Thanks again for the PR/feedback guys. Will publish a release now.

williardx commented 5 years ago

Thanks @tizmagik and @alloy!

tizmagik commented 5 years ago

Released v5.6.0

alloy commented 5 years ago

Thanks! ✨