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

Promise.coroutine should respect the arity of the generator function being wrapped? #927

Closed
renegare opened this Issue Dec 18, 2015 · 15 comments

Comments

Projects
None yet
4 participants
@renegare

renegare commented Dec 18, 2015

Hi guys,

My use case is in the context of a cucumber-js step definition:

  this.Then(/^I should do (.+)$/, Promise.coroutine(function* (something) {
    expect(yield this.do(something)).to.be.true
  }))

Currently the above step when executed will return the following error from cucumber-js:

Error: step definition has 0 arguments, should have 1 (if synchronous or returning a promise) or 2 (if accepting a callback)

I appreciate this could be something cucumber-js should avoid throwing an error on or I solve it locally in my project, but I feel it would be a useful change within bluebird for all.

Thoughts?

@phpnode

This comment has been minimized.

Show comment
Hide comment
@phpnode

phpnode Dec 18, 2015

This is not something that can be fixed by bluebird at all. It's cucumber-js which is making a pretty silly decision to rely on Function.prototype.length

phpnode commented Dec 18, 2015

This is not something that can be fixed by bluebird at all. It's cucumber-js which is making a pretty silly decision to rely on Function.prototype.length

@renegare

This comment has been minimized.

Show comment
Hide comment
@renegare

renegare Dec 18, 2015

Fair enough and I agree more or less. cucumberjs aside, Do you mind explaining why is this something that cannot or should not be implemented?

I don't see this as bug, more of a missing feature.

(P.S: Currently looking into how to programmatically create anonymous wrapper functions with matching arity)

renegare commented Dec 18, 2015

Fair enough and I agree more or less. cucumberjs aside, Do you mind explaining why is this something that cannot or should not be implemented?

I don't see this as bug, more of a missing feature.

(P.S: Currently looking into how to programmatically create anonymous wrapper functions with matching arity)

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Dec 18, 2015

Owner

The only way to do it is with new Function or creating a function for every possible arity. Since in browsers new Function cannot be used due to CSP, that leaves creating a function for every possible arity which is obviously unacceptable.

Owner

petkaantonov commented Dec 18, 2015

The only way to do it is with new Function or creating a function for every possible arity. Since in browsers new Function cannot be used due to CSP, that leaves creating a function for every possible arity which is obviously unacceptable.

@renegare

This comment has been minimized.

Show comment
Hide comment
@renegare

renegare Dec 18, 2015

Fair enough, that does make it unreasonable.

renegare commented Dec 18, 2015

Fair enough, that does make it unreasonable.

@phpnode

This comment has been minimized.

Show comment
Hide comment
@phpnode

phpnode Dec 24, 2015

@petkaantonov in node, Firefox and Chrome (I couldn't immediately check other environments) it is actually possible to do this using Object.defineProperty():

> function a (a, b, c) {}
undefined
> a.length
3
> Object.defineProperty(a, 'length', {value: 1111})
[Function: a]
> a.length
1111
> 

phpnode commented Dec 24, 2015

@petkaantonov in node, Firefox and Chrome (I couldn't immediately check other environments) it is actually possible to do this using Object.defineProperty():

> function a (a, b, c) {}
undefined
> a.length
3
> Object.defineProperty(a, 'length', {value: 1111})
[Function: a]
> a.length
1111
> 

@petkaantonov petkaantonov reopened this Dec 24, 2015

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Dec 24, 2015

Owner

@phpnode oh that's cool

Owner

petkaantonov commented Dec 24, 2015

@phpnode oh that's cool

@renegare

This comment has been minimized.

Show comment
Hide comment
@renegare

renegare Dec 24, 2015

great news! this is "a way" to achieve what I was looking for:

var Promise = require('bluebird')

module.exports = function(gen) {
  var arity = (new Array(gen.length)).fill('a').map((i,n) => i + n)
  eval('function func (' + arity.join(', ')+ ') { return Promise.coroutine(gen).apply(this, arguments) }')
  return func
}

@phpnode not entirely clear on how your example works, but if theres anything I can do, let me know.

renegare commented Dec 24, 2015

great news! this is "a way" to achieve what I was looking for:

var Promise = require('bluebird')

module.exports = function(gen) {
  var arity = (new Array(gen.length)).fill('a').map((i,n) => i + n)
  eval('function func (' + arity.join(', ')+ ') { return Promise.coroutine(gen).apply(this, arguments) }')
  return func
}

@phpnode not entirely clear on how your example works, but if theres anything I can do, let me know.

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Dec 24, 2015

Owner

@renegare like I said before, eval etc cannot be used because of CSP (content security policy). But defineProperty can be used.

Owner

petkaantonov commented Dec 24, 2015

@renegare like I said before, eval etc cannot be used because of CSP (content security policy). But defineProperty can be used.

@spion

This comment has been minimized.

Show comment
Hide comment
@spion

spion Dec 24, 2015

Collaborator

@renegare it should be straightforward to Object.defineProperty on the Promise.coroutine function here: https://github.com/petkaantonov/bluebird/blob/master/src/generators.js#L181-L189 instead of directly returning it. Or in your example:

function co(gen) {
  var len = gen.length;
  var coFn = Promise.coroutine(gen);
  Object.defineProperty(coFn, 'length', {value: len})
  return coFn;
}
Collaborator

spion commented Dec 24, 2015

@renegare it should be straightforward to Object.defineProperty on the Promise.coroutine function here: https://github.com/petkaantonov/bluebird/blob/master/src/generators.js#L181-L189 instead of directly returning it. Or in your example:

function co(gen) {
  var len = gen.length;
  var coFn = Promise.coroutine(gen);
  Object.defineProperty(coFn, 'length', {value: len})
  return coFn;
}
@renegare

This comment has been minimized.

Show comment
Hide comment
@renegare

renegare Dec 24, 2015

hence why the above is not a PR. just an example of the desired functionality ;)

renegare commented Dec 24, 2015

hence why the above is not a PR. just an example of the desired functionality ;)

@renegare

This comment has been minimized.

Show comment
Hide comment
@renegare

renegare Dec 24, 2015

@spion I see! yeah I was confused about how to apply it. Apologies guys if the eval example implied the right way to do it.

Nice one 👍

renegare commented Dec 24, 2015

@spion I see! yeah I was confused about how to apply it. Apologies guys if the eval example implied the right way to do it.

Nice one 👍

@renegare

This comment has been minimized.

Show comment
Hide comment
@renegare

renegare Dec 24, 2015

Shall I have a stab at a PR?

renegare commented Dec 24, 2015

Shall I have a stab at a PR?

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Dec 24, 2015

Owner

Sure if you are up for it, but make sure you use the es5.js's defineProperty as it works in IE as well.

Owner

petkaantonov commented Dec 24, 2015

Sure if you are up for it, but make sure you use the es5.js's defineProperty as it works in IE as well.

@renegare

This comment has been minimized.

Show comment
Hide comment
@renegare

renegare Dec 24, 2015

Hey I believe its done. Feedback comments are welcome.

Idea, would it be cool to set the Function.length as a getter? than a mutable param? Cross browser issues?

renegare commented Dec 24, 2015

Hey I believe its done. Feedback comments are welcome.

Idea, would it be cool to set the Function.length as a getter? than a mutable param? Cross browser issues?

@petkaantonov

This comment has been minimized.

Show comment
Hide comment
@petkaantonov

petkaantonov Jan 23, 2016

Owner

Fixed in 3.1.2

Owner

petkaantonov commented Jan 23, 2016

Fixed in 3.1.2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment