Skip to content

Return promise for decorator on async method #147

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
tizmagik merged 4 commits into from
Dec 19, 2019
Merged

Return promise for decorator on async method #147

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8f6df64
Methods will now return original promise when tracking descorator is …
rickh18 Nov 14, 2019
cc3e8f4
7.2.2
rickh18 Nov 16, 2019
9c47128
7.2.1
rickh18 Nov 19, 2019
d7adf1f
Added e2e test for async function
rickh18 Nov 19, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "react-tracking",
"version": "7.2.1",
"version": "7.2.2",
"description": "Declarative tracking for React apps.",
"keywords": [
"declarative",
Expand Down
9 changes: 6 additions & 3 deletions src/__tests__/trackEventMethodDecorator.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,8 @@ describe('trackEventMethodDecorator', () => {
const dummyData = {};
const trackingData = jest.fn(() => dummyData);
const trackEvent = jest.fn();
const spyTestEvent = jest.fn();
const eventData = 'eventData';
const spyTestEvent = jest.fn(() => eventData);
const dummyArgument = 'x';

class TestClass {
Expand All @@ -90,11 +91,12 @@ describe('trackEventMethodDecorator', () => {
}

const myTC = new TestClass();
myTC.handleTestEvent(dummyArgument);
const result = myTC.handleTestEvent(dummyArgument);

// Access the trackingData arguments
const trackingDataArguments = trackingData.mock.calls[0];

expect(result).toEqual(eventData);
expect(trackingData).toHaveBeenCalledTimes(1);
expect(trackingDataArguments[0]).toEqual(myTC.props);
expect(trackingDataArguments[1]).toEqual(myTC.state);
Expand Down Expand Up @@ -131,11 +133,12 @@ describe('trackEventMethodDecorator', () => {
}

const myTC = new TestClass();
await myTC.handleTestEvent(dummyArgument);
const result = await myTC.handleTestEvent(dummyArgument);

// Access the trackingData arguments
const trackingDataArguments = trackingData.mock.calls[0];

expect(result).toEqual(dummyResolve);
expect(trackingData).toHaveBeenCalledTimes(1);
expect(trackingDataArguments[0]).toEqual(myTC.props);
expect(trackingDataArguments[1]).toEqual(myTC.state);
Expand Down
13 changes: 9 additions & 4 deletions src/trackEventMethodDecorator.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,15 @@ export default function trackEventMethodDecorator(trackingData = {}) {

const fn = Reflect.apply(decoratedFn, this, args);
if (Promise && Promise.resolve(fn) === fn) {
return fn.then(trackEvent.bind(this)).catch(error => {
trackEvent(null, error);
throw error;
});
return fn
.then(trackEvent.bind(this))
.then(() => {
return fn;
})
Copy link
Collaborator

@tizmagik tizmagik Nov 19, 2019

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

@rickh18 rickh18 Nov 19, 2019

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

@tizmagik tizmagik Nov 19, 2019

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)

.catch(error => {
trackEvent(null, error);
throw error;
});
}
trackEvent();
return fn;
Expand Down