callAction has a bad condition | bug

[seeden]

Hi

I found a very strange behaviour. My done callback was very often called twice. The problem is on the next line

fluxible/packages/fluxible/utils/callAction.js

Line 29 in 3863005

} else if (action.length < 3) {

I am using babel and babel will transform next action

export function setAction(context, payload = {}, done) {
}

into the

function setAction(context) {
  var payload = arguments[1] || {};
  var done = arguments[2];
}

As you can see transformed action named setAction has just one parameter (original has 3 parameters) and your condition action.length < 3 will be equals true. I think that too many of us are using babel in this moment and can have a same problem.

[mridgway]

That's odd. Looks like this is the behavior of functions that use default parameters. Let me look in to whether this is new behavior from babel and whether we could treat this as a bug there, since it seems like babel shouldn't be changing the length.

[mridgway]

Opened https://phabricator.babeljs.io/T7377 to follow up with Babel team.

[mridgway]

After further investigation, this may be how ES6 spec works with default parameter values. We may have to find another solution for this internally.

[seeden]

yes, the current condition is not good

[mridgway]

So the only way to mitigate this that I can see is to check the return value against undefined. I'm not sure if there are valid cases where undefined could be the resolved value of the action's Promise. Seems like this would be a breaking change though either way.

[mridgway]

After finding a bug internally that was related to this arguments.length, I think the best way forward is to remove the automatic resolution of Promises by returning values from actions. Instead, we would require Promises to be explicitly returned from the action or we will wait for the done callback to be called.

Anyone using the return value as the resolved value would need to use return Promise.resolve(value) instead or just returning the value from the action.

[mridgway]

This would basically be just removing that line linked to in the origin comment (https://github.com/yahoo/fluxible/blob/master/packages/fluxible/utils/callAction.js#L29).

[carystanley]

We do have actions without the callback param, that do not use promises, this is functionality that has been in place from the beginning. These actions are mostly doing synchronous logic and/or context.dispatch calls that would not really track callback, we have quite a few of these actions, so any breaking change introduced that removes support for this case would need substantial changes.

(just want you to be aware)