Track async method

[spencewood]

This introduces the ability to use @track on asynchronous methods and have the track functionality run after the method has resolved:

{
...
    @track()
    async fireEvent() {
        await eventDispatch();
    }
}

It doesn't change the ability to track non-async methods.

Because the tracked method needs to be run before it can be identified as an asynchronous method (it returns a Promise), this change alters the flow of when the function and tracking are called. In all cases, the function will run and then the tracking will be called.

Since this deviates from the original order of operations, I can see how this may introduce unwanted effects. Alternatively, I can see a new @trackAsync decorator that could be used for this. I do prefer transparent handling of this kind of thing, and not having two different decorators depending on usage.

I realize that there are also alternate ways to address the above (as talked about here: https://github.com/NYTimes/react-tracking/issues/42), but I think adding additional methods simply for promise chain handling or explicitly calling trackEvent reduces some of the elegance and simplicity of this approach.

[tizmagik]

Thanks @spencewood for the PR. Love the approach and the elegance of transparently supporting async functions without changing the API.

Since this deviates from the original order of operations, I can see how this may introduce unwanted effects.

My main concern would be exactly this. Can you think of edge cases or test cases that could be added to codify this change in behavior? That would make me much more comfortable in supporting this ability in react-tracking (it has come up a few times) and it would be great to do so without changing the API but I want to be very deliberate and careful about the possible change in behavior.

Either way, I think I would publish this as a breaking change, just to be safe, but I'd like a detailed before/after test case we can point folks to.

Thanks so much!

[spencewood]

The only thing I can think of that may be a breaking change with this would be if the method functionality itself relied on something having happened from the the @track itself. Perhaps something relying on the existence of an item attached to the dataLayer or that something was put in place by the custom dispatch. I don't think this is a great practice to bind tracking and functionality together like this, and in my opinion defeats one of the main points of using this library in a declarative way. With that said though, I can't say that it is out of the realm of possibility that something like this is being done.

I can update the tests to ensure that the ordering of the functions is being respected (function before tracking). Is that what you meant by test cases? Or are you looking for something more documentation related?

[tizmagik]

I can update the tests to ensure that the ordering of the functions is being respected (function before tracking).

Hmm yea, I think that might be enough. And if you can think of any other test cases to put in, that would be great. Also, if you could update the documentation/README to mention this new capability.

Otherwise I should be able to review/merge this in the next few days. Thanks again for the PR, great stuff -- I'm especially glad this didn't require any changes to the existing tests/API.

[spencewood]

I've added another test for ordering and some simple updates for the README. Let me know if you need anything else. I might not be able to get back to it until mid next week though.

Thanks for your eagerness and feedback around this! This has been an excellent tool to work with.

[tizmagik]

Thanks again for the test @spencewood -- looks great!

Curious what you think about this? I'm wondering if we need a way to pass along the resolved/rejected object/error to the @track function?

@track((props, state, args) => {
  if (args[0] instanceof Error) {
    // the promise rejected
  } else {
    // the promise resolved
    const resolvedObj = args[0];
  }
})
async handleEvent() {
  return await someAsyncFn();
}

Or maybe there's a more natural way to know if the promise resolved or rejected, but then how to act on the returned data within the tracking call? The snippet above is kinda weird in the sense that we're "hijacking" the args

[spencewood]

Yeah, I realize isn't ideal at all to pull off arguments in this manner. I looked over the PR again and there are a couple of improvements I can make.

First, the arguments of the class method should be captured separately from the tracking arguments. With this PR as it currently stands, when returning a promise, you'd be getting only the arguments of the return (as you've highlighted above). To make this more consistent with the current API and be able to get the original method arguments, we could call trackData with something like this:

...
  const thisTrackingData =
    typeof trackingData === 'function'
      ? trackingData(
           this.props,
           this.state,
           methodArguments,
           trackingArguments // promise arguments
         )
       : trackingData;
   this.props.tracking.trackEvent(thisTrackingData);

Then we'd have a fourth parameter that would always be used for promises. This isn't great, and I really hate adding more arguments, but I don't really see another way. And like you pointed out above, this would still need to be checked explicitly:

@track((props, state, args, promiseArgs) => {
  if (promiseArgs[1] instanceof Error) {
    // the promise rejected
  } else {
    // the promise resolved
    const resolvedObj = promiseArgs[0];
  }
})

The other thing that needs to be done here is to actually pass the error along when calling trackEvent in the catch:

  return fn.then(trackEvent.bind(this)).catch(error => {
    trackEvent(null, error);
    throw error;
  });

Then we can be assured that if there is a value in the second position of the tracking promise arguments, that we have an error.

Thinking about it even more, I think though that the verboseness of this can be reduced up when needing an argument from a promise:

@track((props, state, args, [{ val }, err]) => { // destructure the resolve property from the first position and error in the second
  if (err instanceof Error) {
    return { }; // fail
  }
  return { val }; // success
}

[spencewood]

@tizmagik I've made the changes that I talked about above and updated the README. I feel like it accomplishes everything that I think is needed. Unfortunately, it does add an additional argument for the tracking function for use with the promise data, but this part isn't a breaking change.

The one thing I'll point out here is that I did use spread for gathering arguments for the function return. I know that arguments was used to get the method arguments (still is) with and explicit /* eslint-disable prefer-rest-params */ linting rule. If there is a reason you'd like to avoid spread for this, I can refactor to use arguments for the trackEvent as well.

[tizmagik]

Thanks @spencewood -- if you're okay to wait on this change, I'm going to be out on Vacation next week and won't get back to this until the week after. If this is blocking you in any way, I'd be happy to cut an @next release tag in the mean time. Let me know!

Thanks so much for your work on this PR, it's really appreciated!

[spencewood]

I've got a workaround for handling async methods right now, so I'm in no rush. Let me know if you need anything else from me. Thanks!

[spencewood]

hey @tizmagik, just wondering if you're ready to proceed on this. Let me know if you have any more concerns or would like any other changes.

[tizmagik]

Hey @spencewood , I haven't forgotten about this. I've just been traveling the last couple weeks and haven't really had a chance to take a closer look at this.

I'm not thrilled with adding extra args to the function signature, but if I can't figure out a cleaner way I think I'll go ahead and merge/release this. This should hopefully land within the next week or so.

Sorry for the delay and thanks so much for your work on this!

[tizmagik]

Okay I can't think of a better way than adding a 4th argument. Let's go with this for now, since it's non-breaking I think we can maybe iterate later 😁

Great work @spencewood -- I'm going to merge this, make some minor tweaks, update some dependencies then cut a new minor release. Thank you!