Unification of executeAction between ActionContext and MockActionContext

[mridgway]

This is a potentially breaking change that I want to be careful about.

Problem

Given the example:

var fooAction = function () { 
    return 'foo'; 
};

• MockActionContext.executeAction currently returns the return value of the action: 'foo' == mockActionContext.executeAction(fooAction).
• FluxibleContext.executeAction returns a Promise that would be resolved with the value 'foo'.

The problem with this is that if there are nested executeAction calls, then the parent action will not receive a Promise as it would expect:

var parentAction = function (context) {
    return context.executeAction(fooAction).then(function (result) {
        console.log(result);
    });;
}

This causes significant problems in testing a code base that is mixed with actions that use Promises and callbacks, which is supported by FluxibleContext, but not MockActionContext.

Solution

The solution is to ensure that they are using the same implementation. In this PR I have made both contexts use the same implementation.

Breaking Change or Bug?

I could easily mark this as a bug, since it presents an inconsistency which makes it impossible to test certain cases. The issue however, is that there will be many tests out there that may be affected by this change:

1. Tests that relied on the return value of executeAction to match the action's return value.
   • This is resolved by calling the action directly instead of through executeAction.
2. The ordering of parallel actions could change due to the setImmediate. Any tests that rely on the order of parallel actions (which you could argue is unsafe) could be broken.
   • This is resolved by making tests not reliant on order of parallel actions.

We found a few tests (5 out of 301 tests) in one internal package that were affected due to the second item. We never relied on the first, but others in the community may be.

Options

1. Classify as a bug and release as a patch fix. Users may be affected by broken tests.
2. Classify as a breaking change and release as a major version bump (2.x). No one is affected immediately, but a migration would be need to upgrade to 2.x.
3. Classify as a breaking change and release a separate MockActionContext API (what would we even name it?) and release as minor version or a separate package. Current MockActionContext would be deprecated in 2.x. This minimizes the breakage but would still eventually require a migration when 2.x is released.

Request for Input

I would love to get the community's input on this. I'm also curious how many people are affected by these issues.

Please try installing fluxible@test in your project and run your test suite. Please report any breakages that you see.

Assuming the impact is minimal, we will go ahead with releasing this as a bug fix that gets released as 1.0.4.

[mention-bot]

By analyzing the blame information on this pull request, we identified @localnerve, @cesarandreu and @Vijar to be potential reviewers

[yahoocla]

CLA is valid!

[localnerve]

I'd be fine with option 1, but IMHO, if you really don't want to introduce an unexpected breaking change then you have to go with 2 (or 3).
I'm also curious about the degree of breakage.
Sometimes I publish an 'rc-' branch for a limited period so that I can get a feel for the amount of breakage.

[mridgway]

I have published a new version under the test tag that can be tried with the above changes: npm i fluxible@test which will give you version 1.0.4-rc1. Anyone that has a chance to try these changes out on your test suite, please post your findings here.

If you see a missing peerDependency warning due to other packages depending on ^1.0.0, it should be ok to ignore that warning.

[localnerve]

Dropped in 1.0.4-rc1 in two of my example projects, flux-react-example (156 tests) and flux-react-example-sw (344 tests). Test suites ran perfectly with no changes required. Double checked that the fluxible package.json actually reflected 1.0.4-rc1 in both.

agrant at Alexs-MacBook-Pro in ~/projects/flux-react-example-sw on master
$ node --version
v4.2.4

Please note that the code delivered with 1.0.4-rc1 was not the same as the commit with this PR. For example, line 33, fluxible/utils/MockActionContext.js:

return callAction.callActionNoImmediate(this, action, payload, callback);

[mridgway]

Good catch. Forgot that I had an experimental branch checked out. I published 1.0.4-rc2 with the code from this PR.

[localnerve]

Yep, now I have the right code, 1.0.4-rc2. Worked on both example projects as drop-in replacement, no change required.

[mridgway]

Alright, so since this change seems to have limited impact and only when the tests are written to expect parallel executeAction calls to be in a specific order which should not be the case, we are going to release this as a bug fixing patch release today.

[mridgway]

Created https://gist.github.com/mridgway/4d66eb1d3277d8b442f2 as an overview for the changes and possible impacts.