[Feature] Access to component state data #45

mennenia · 2017-09-20T13:02:55Z

Hiii 👋

We've been very excited to integrate react-tracking into our work over at Artsy!

In our use of the library, we came the need to access the component's state, in order to pull out certain bits of data for our analytics.

For example, when viewing an Artist's profile, there's a follow button which, depending on the state, changes action and appearance. When pressing this button, we would send the analytics events stating whether the user pressed "follow artist" or "unfollow artist", and this logic is captured in the component's state.

As such I've added the ability to also pull out state, as well as props, when implementing @track().

In code:

  @track((props, state) => ({ action: state.following ? "press unfollow button" : "press follow button" }))
  handleFollowChange() {
    // method body
  }

I'm aware that in this case the follow button could be implemented as its own component with props of course, but we do keep interesting internal details and data within state that we'd like to include in analytics throughout.

Would you be open to this addition? If so I can also add some additions to the README for this case specifically, too :)!

tizmagik · 2017-09-20T21:10:34Z

Neat! I hadn't considered that use case. This seems like a neat solution, thank you! 🎉

I like what you have here, I'm just concerned about the fact that I think this is actually going to be a breaking change for the following use case mentioned in the docs (which I now realize is missing from the test suite, so thank you for that as well!):

  // In this case the tracking data depends on
  // some unknown (until runtime) value
  @track((props, [event]) => ({ // NOTE: This PR would change this function signature
    action: 'click',
    label: event.currentTarget.title || event.currentTarget.textContent
  }))
  handleClick = (event) => {
    if (this.props.onClick) {
      this.props.onClick(event);
    }
  }

It might be okay and we can just publish as semver major, but was wondering if you had any thoughts about this or any way to avoid the breaking change in this case?

Also, before this PR is merged, I think we'd need to add the following:

Additional test case in trackEventMethodDecorator.test.js
Doc updates (be happy to take that myself, but feel free to take a stab at it!)
Add a test case for the (props, [event]) => function signature mentioned above (I might just spin up a separate issue for this, to be fair)

mennenia · 2017-09-21T08:07:05Z

Ha, I had actually expected more tests to break (because of that parameter) and didn't notice that case was missing 😄.

I realise indeed that it's a big change.. I'm not sure what other order would make sense, since you'd always be passed props and state, and the args are optional. I can't think of another neat way personally, but very happy to change this if you think it could be done differently!

Thanks for pointing out the final requirements, I'll update shortly with both trackEventMethodDecorator.test.js and docs (I love writing docs) 👌.

With regards to the test for the (props, [event]) => function, if you know exactly what the test should look like + use cases (because we rarely grab events in React Native), I could merge any of those changes into my branch?

tizmagik · 2017-09-21T14:26:01Z

Sounds great! I'll look forward to your updates for the additional test and docs, and I'll spin up a separate issue for that (props, [event]) => signature test. We don't have to block on that for this PR. 😀

mennenia · 2017-09-26T12:43:46Z

I think by having to add a test to trackEventMethodDecorator.tests.js I ended up sort of doing #46?

I didn't add an additional test for when state isn't set.. It seemed overkill, but I could add one as state will just be undefined :).

Comments inline regarding some caveats around accessing and comparing the call arguments!

mennenia · 2017-09-26T12:44:31Z

src/__tests__/trackEventMethodDecorator.test.js

+    myTC.handleTestEvent(dummyArgument);
+
+    // Access the trackingData arguments
+    const trackingDataArguments = trackingData.mock.calls[0];


mennenia · 2017-09-26T12:48:42Z

src/__tests__/trackEventMethodDecorator.test.js

+    expect(trackingDataArguments[1]).toEqual(myTC.state);
+    // Here we have access to the raw `arguments` object, which is not an actual Array,
+    // so in order to compare, we convert the arguments to an array.
+    expect(Array.from(trackingDataArguments[2])).toEqual([dummyArgument]);


Originally I tried:

expect(trackingData).toHaveBeenCalledWith(myTC.props, myTC.state, spyTestEvent.mock.calls[0]) // also tried ['x'] or [dummyArgument] in this case

However, you end up comparing an argument with an Array, which will not resolve. Hence we extract the arguments one by one, convert this specific case to an Array, which ends up comparing just fine 👌

Makes sense. Thanks for the comment as well 👍

mennenia · 2017-09-26T13:15:46Z

README.md

@@ -59,7 +59,7 @@ Alternatively, if you just want to just silence proptype errors when using [esli
 ### Usage as a Decorator
 `react-tracking` is best used as a `@decorator()` using the [babel decorators plugin](https://github.com/loganfsmyth/babel-plugin-transform-decorators-legacy).

-The decorator can be used on React Classes and on methods within those classes.
+The decorator can be used on React Classes and on methods within those classes. If you use it on methods within these classes, make sure to decorate the class as well. 



I added this here because, initially, it seems optional. You can either/or decorate methods and classes.

For a while we were decorating methods, yet running into runtime errors (because naturally, the tracking props hadn't been added to the component). Thought it might help to explicitly mention, if you decorate methods, also to decorate the class itself so those tracking props are exposed :).

Yes, good call. Thank you!

tizmagik

Beautiful PR, thank you for this. I'll merge and cut a release later today 🎉

tizmagik · 2017-09-26T14:26:52Z

README.md

@@ -59,7 +59,7 @@ Alternatively, if you just want to just silence proptype errors when using [esli
 ### Usage as a Decorator
 `react-tracking` is best used as a `@decorator()` using the [babel decorators plugin](https://github.com/loganfsmyth/babel-plugin-transform-decorators-legacy).

-The decorator can be used on React Classes and on methods within those classes.
+The decorator can be used on React Classes and on methods within those classes. If you use it on methods within these classes, make sure to decorate the class as well. 



Yes, good call. Thank you!

tizmagik · 2017-09-26T14:27:56Z

src/__tests__/trackEventMethodDecorator.test.js

+    expect(trackingDataArguments[1]).toEqual(myTC.state);
+    // Here we have access to the raw `arguments` object, which is not an actual Array,
+    // so in order to compare, we convert the arguments to an array.
+    expect(Array.from(trackingDataArguments[2])).toEqual([dummyArgument]);


Makes sense. Thanks for the comment as well 👍

tizmagik · 2017-09-26T14:29:35Z

Fixes #46

mennenia · 2017-09-26T16:49:27Z

Woohoo 🙌

* add temporary interaction object to remove validation error * update documentation * update build files

mennenia added 3 commits August 22, 2017 14:41

[Feature] Add component's state in tracking data

e103e58

[Tests] Add test for passing state in track function

436399d

[Lint] Fix linter errors

878e60f

tizmagik added the breaking change label Sep 20, 2017

tizmagik mentioned this pull request Sep 21, 2017

Add test for @track((props, [event]) => {}) signature #46

Closed

alloy mentioned this pull request Sep 25, 2017

TypeScript typings #44

Closed

mennenia added 2 commits September 26, 2017 13:34

[Tests] Add test for trackingData function arguments

98d3f35

[Tests] Add comments, fix lint

91de97a

mennenia commented Sep 26, 2017

View reviewed changes

mennenia added 2 commits September 26, 2017 13:51

[Lint] Fix errors

1efecf2

[Docs] Add state use case

60e6cce

mennenia commented Sep 26, 2017

View reviewed changes

tizmagik approved these changes Sep 26, 2017

View reviewed changes

tizmagik merged commit f6783d1 into nytimes:master Sep 26, 2017

mennenia deleted the state branch September 26, 2017 16:49

alloy mentioned this pull request Sep 28, 2017

[react-tracking] Update for v5 and use Pick over Partial. DefinitelyTyped/DefinitelyTyped#20110

Merged

3 tasks

scentless-apprentice pushed a commit to scentless-apprentice/react-tracking that referenced this pull request Apr 26, 2018

update schema...temporarily (nytimes#45)

4ba3f06

* add temporary interaction object to remove validation error * update documentation * update build files

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Feature] Access to component state data #45

[Feature] Access to component state data #45

mennenia commented Sep 20, 2017 •

edited

tizmagik commented Sep 20, 2017

mennenia commented Sep 21, 2017

tizmagik commented Sep 21, 2017

mennenia commented Sep 26, 2017

mennenia Sep 26, 2017

mennenia Sep 26, 2017

tizmagik Sep 26, 2017

mennenia Sep 26, 2017

tizmagik Sep 26, 2017

tizmagik left a comment

tizmagik Sep 26, 2017

tizmagik Sep 26, 2017

tizmagik commented Sep 26, 2017

mennenia commented Sep 26, 2017

[Feature] Access to component state data #45

[Feature] Access to component state data #45

Conversation

mennenia commented Sep 20, 2017 • edited

tizmagik commented Sep 20, 2017

mennenia commented Sep 21, 2017

tizmagik commented Sep 21, 2017

mennenia commented Sep 26, 2017

mennenia Sep 26, 2017

Choose a reason for hiding this comment

mennenia Sep 26, 2017

Choose a reason for hiding this comment

tizmagik Sep 26, 2017

Choose a reason for hiding this comment

mennenia Sep 26, 2017

Choose a reason for hiding this comment

tizmagik Sep 26, 2017

Choose a reason for hiding this comment

tizmagik left a comment

Choose a reason for hiding this comment

tizmagik Sep 26, 2017

Choose a reason for hiding this comment

tizmagik Sep 26, 2017

Choose a reason for hiding this comment

tizmagik commented Sep 26, 2017

mennenia commented Sep 26, 2017

mennenia commented Sep 20, 2017 •

edited