Skip to content
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.

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

Using triggerPromise I only receive one argument in .then? #5

Open
spoike opened this issue Oct 13, 2015 · 8 comments
Open

Using triggerPromise I only receive one argument in .then? #5

spoike opened this issue Oct 13, 2015 · 8 comments

Comments

@spoike
Copy link
Member

spoike commented Oct 13, 2015

Reported by @ponelat in reflux/refluxjs#416

When I have a triggerPromise... I only receive the first argument in then clause.

var someAction = Reflux.createAction({asyncResult: true});
someAction.listen(function(a,b,c) {
  someAction.completed(a + '-eh', b + '-eh', c + '-eh')
})

someAction.triggerPromise(a,b,c)
  .then(function(d,e,f) { // Assume d,e,f are passed into 
    console.log(d,e,f) // <- Only d is defined
  })
.catch(function() {})

I've looked into the source and I'm not sure why this is assumed to be an array of args?... https://github.com/reflux/reflux-core/blob/master/src/PublisherMethods.js#L159

Could be a bug, or it might be me. I can PR if I get some time, but otherwise help is always appreciated!

@levrik
Copy link
Contributor

levrik commented Oct 13, 2015

This isn't possible due to limitations of the Promise API. It only allows one argument in the callbacks.
(Source: http://stackoverflow.com/a/22776850/3443442)

It tried this

var removeSuccess = me.completed.listen(function () {
    removeSuccess();
    removeFailed();
    resolve.apply(null, arguments);
});

but only the first argument is defined in the then-callback.

@spoike
Copy link
Member Author

spoike commented Oct 13, 2015

Actually I believe you can by using arrays and Promise.all to handle the arguments internally. This leads to the following changes:

  • (Breaking) then will have array as first argument

In order to have the user use the array as arguments they can do either the following:

  • The user needs to use spread method instead to have the array spread out to arguments, which is supported by most Promise implementation
someAction.triggerPromise(a, b, c)
  .spread((d, e, f) => {
    console.log(d, e, f);

  })
  • Destructure the array
someAction.triggerPromise(a, b, c)
  .then(() => {
    var { d, e, f } = arguments;
    console.log(d, e, f);
  })

@levrik
Copy link
Contributor

levrik commented Oct 13, 2015

I will try this later

@ponelat
Copy link

ponelat commented Oct 13, 2015

@spoike @DerNivel thanks for the follow up to this issue!

@levrik
Copy link
Contributor

levrik commented Oct 13, 2015

@spoike spread is not available in the native implementation of Promise. But with this change down below we could support Promise libraries that support it:

var removeSuccess = me.completed.listen(function () {
    var args = Array.prototype.slice.call(arguments);
    removeSuccess();
    removeFailed();
    resolve(args.length > 1 ? args : args[0]);
});

In ES6 this would be possible with the native implementation of Promise:

someAction.triggerAsync().then(([obj1, obj2]) => {
    // obj1 and obj2 are available
});

If this is okay I'll create a PR.

@spoike
Copy link
Member Author

spoike commented Oct 13, 2015

@DerNivel 👌

@levrik
Copy link
Contributor

levrik commented Oct 13, 2015

#7 fixes this issue

@spoike
Copy link
Member Author

spoike commented Oct 16, 2015

Other than #7, we also need to:

  • Add to the README a note on the use of arrays
  • Add a proper test, so this won't regress

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

No branches or pull requests

3 participants