Skip to content

Conversation

@rickh18
Copy link
Contributor

@rickh18 rickh18 commented Nov 14, 2019

After updating from 5.3.0 my application broke on a method that was returned a promise. It turned out that the instead of the promise, undefined was returned by the method, comparable to the behaviour described in issue #106.

This has been fixed by adding return fn in trackEventMethodDecorator.
The unit test now has an extra check to see if the original method still works.

Copy link
Collaborator

@tizmagik tizmagik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, and great work! This makes sense -- I just had a couple notes inline.

Thanks also for adding a test to trackingEventMethodDecorator.test.js -- would you mind also adding an e2e test with an async use case? I tend to put more weight behind that suite since it's actually testing the external API.

Comment on lines 27 to 29
.then(() => {
return fn;
})
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: mind rewriting as:

.then(() => fn)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed with d7adf1f
I also added an e2e test as suggested.

When writing the test, I noticed that the tracking function breaks if a promise is rejected. It will fail on { value } since value is set to null after reject. So when using the example in the readme:

@track((props, state, methodArgs, [{ value }, err]) => { } <-- will give TypeError

.catch(error => { trackEvent(null, error); throw error; }); <-- trackEventMethodDecorators.js line 28, value is set to null

So the happy flow is working now, but maybe this should be fixed as well. (changing null to {}?)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea that makes sense, good catch! Mind making that change as a separate PR (will need an accompanying test update)

Copy link
Collaborator

@tizmagik tizmagik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this! (And apologies for the delay!)

@tizmagik tizmagik merged commit 7e79b3a into nytimes:master Dec 19, 2019
This was referenced Dec 19, 2019
@tizmagik
Copy link
Collaborator

Released as v7.3.0 -- thank you! 🎉

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

Successfully merging this pull request may close these issues.

2 participants