Skip to content

Conversation

artemtsushko
Copy link

Addresses #94. The fix is copied from #95, but targets version 2.0.
Also added 2 tests to keep 100% coverage and make sure that cases when request type descriptor's 'meta' or 'payload' properties are functions are still handled properly.

@coveralls
Copy link

coveralls commented Jan 17, 2018

Coverage Status

Coverage remained the same at 100.0% when pulling c66c7f4 on artemtsushko:request-action-without-await into cbf0553 on agraboso:master.

Copy link
Collaborator

@nason nason 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 putting this together @artemtsushko -- I have a couple concerns below but this looks nice!

next(await actionWith(requestType, [action, getState()]));
if (
typeof requestType.payload === 'function' ||
typeof requestType.meta === 'function'
Copy link
Collaborator

Choose a reason for hiding this comment

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

How about the other requestType properties that can be passed as functions? ie, headers, options or bailout

Copy link
Author

Choose a reason for hiding this comment

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

headers, options or bailout aren't properties of requestType, they are properties of [RSAA], thus this change doesn't apply to them.

) {
next(await actionWith(requestType, [action, getState()]));
} else {
next(requestType);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it possible/worth it to add tests that verify this does happen synchronously, as expected?

Copy link
Author

Choose a reason for hiding this comment

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

I think it's neither worth nor possible to test that this happens synchronously. Provided there is no mistake in if condition, and taking into account there is no await in else branch, this should work as expected. I have added 2 tests that cover first branch of this if statement, and they ensure that meta or payload functions are called, that they are called with correct arguments, and that their result is inserted to meta or payload property of request FSA. A lot of existing tests cover the second branch.

Copy link
Collaborator

@nason nason 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 your helpful responses @artemtsushko 👍

@nason nason merged commit 77b949f into agraboso:master Jan 24, 2018
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.

3 participants