Add use() method for alternative promise implementation

[overlookmotel]

This PR adds the facility for a user to optionally choose to use a non-native promise implementation for the promises created by co.

I've seen the debate in the issues here about V8 native promises vs Bluebird, and totally understand the reasoning that it's best to use V8. This PR does not alter the default behaviour of co but allows the user to choose another promise implementation if they want to.

The PR adds a method co.use():

co.use()

var co = require('co');

// normal behaviour
var p1 = co(function*() {});
console.log(p1 instanceof Promise); // true

// with co.use()
var bluebird = require('bluebird');
co.use(bluebird);

var p2 = co(function*() {});
console.log(p2 instanceof bluebird); // true

The promise implementation is also accessible as co.Promise.

co.use() is chainable, so you can initialize co with:

var co = require('co').use(require('bluebird'));

Why is this needed anyway?

At present the only way to use bluebird as the promise implementation within co is:

var bluebird = require('bluebird');
Promise = bluebird;
var co = require('co');

var p = co(function*() {});
console.log(p instanceof bluebird); // true

But this poisons the global variable Promise which isn't good.
i.e. it needs support within co to achieve this - it's not possible to implement in a module on top of co.

Whether people should want to use bluebird over V8 native promises is a matter of debate, but personally I prefer bluebird. But I also very much like co and it'd be great to be able use the two together.

Completing this PR

This PR isn't yet complete - needs tests and documentation to finish it.

But could you please indicate whether you'd be in principle willing to support this? If so, I'll write the tests and docs.

[yoshuawuyts]

Sounds like a reasonable feature. I'd make it a setter / optional argument though; usually can't be bothered to properly recall the chainable property names.

e.g.

const bluebird = require('bluebird')
const co = require('co')

co.promise = bluebird

[overlookmotel]

@yoshuawuyts Yes, this works too:

var co = require('co');
co.Promise = require('bluebird');

The thing about .use() that I like is that you can do it in one line:

var co = require('co').use(require('bluebird'));

[madbence]

But this poisons the global variable Promise which isn't good.

So let's use another global variable (co.Promise)? Overriding the native Promise is just fine.

[targos]

I agree with @madbence.
Why would you like to change the Promise implementation for co and not for the rest of your application ?

[overlookmotel]

@madbence and @targos: When I say it poisons the global variable Promise, I mean poisons it completely globally.

After doing Promise = require('bluebird') in one file, it changes the value of Promise in every file in your app, including within modules the app requires.

I agree with @targos that most of the time you'd want to use the same Promise implementation for the whole of your app.

But... if you are writing a module and want to use co and bluebird internally within that module, you can't do it without affecting the value of Promise outside your module too. i.e. it changes the value of Promise for the app that requires your module, and for any other modules that app requires too. This is likely to break things all over the place.

@madbence co.Promise isn't global in the same way that Promise is. You assign co to a local variable e.g. var co = require('co') so that doesn't affect the workings of any other file/module.

Am I making any sense?

[yoshuawuyts]

The thing about .use() that I like is that you can do it in one line:

Ah, guess that makes sense.

[madbence]

If two modules are using co, and they resolve to the same version, require('co') will return the same object (npm is pretty good at deduping dependencies), so it's still global. Using co internally and overriding it's Promise implementation with co.use will override the Promise implementation for every other module that uses co internally.

co.use should be a factory, that returns a copy of co that uses the given Promise implementation.

[overlookmotel]

@madbence That's a good point. I hadn't thought of that. I'll change this PR accordingly.

[overlookmotel]

PR altered to fix issue raise by @madbence.

Of course, the code indentation would have to be altered as it's all inside makeCo(), but I've left it as is for now to make the diff clearer.

This has the side-effect of making co a singleton. This should make no difference in almost all cases, but may not be desired - let me know if so and I can work out another way to implement.

[overlookmotel]

Here's an alternative implementation which doesn't turn co into a singleton:
overlookmotel@692f6ee

It's a bit uglier, though...

Are you guys likely to be willing to merge either of these? (once tests and docs are done)

[overlookmotel]

Just wondered if you guys have had a chance to consider this?

I'm happy to add tests if you're going to be willing to merge it.

[ruimarinho]

@overlookmotel, awesome PR. I'm actually a converted ES6 native Promise user to bluebird for several reasons. Just a sneak peek for those who would like to take advantage of an alternative Promise implementation, namely in terms of improved error handling:

index.js

co(function* () {
  throw new Error('foo');
});

❯ node index.js

// <no output, error is swallowed unless an `uncaughtRejection` handler is added>

index.js

co(function* () {
  throw new Error('foo');
});

❯ node index.js

Unhandled rejection Error: foo
    at index.js:4:9
    at GeneratorFunctionPrototype.next (native)
    at onFulfilled (index.js:83:19)
    at index.js:72:5
    at tryCatcher (node_modules/bluebird/js/main/util.js:26:23)
    at Promise._resolveFromResolver (node_modules/bluebird/js/main/promise.js:476:31)
    at new Promise (node_modules/bluebird/js/main/promise.js:69:37)
    at co (index.js:68:10)
    at Object.<anonymous> (index.js:3:28)
    at Module._compile (module.js:430:26)
    at Object.Module._extensions..js (module.js:448:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)
    at Function.Module.runMain (module.js:471:10)
    at startup (node.js:117:18)
    at node.js:948:3

[overlookmotel]

Hello all.

Do the maintainers have any interest in including this functionality? It's something I need personally, but I'm loathe to fork such a popular and useful module, when it could be included in the master and so of more use to others.

Could you please give some indication as to whether this is something you would consider merging?

To recap...
The idea is to add method .use() to allow the user to specify another promise implementation to use with co.
I've provided two different possible implementations - one on this PR, and an alternative at overlookmotel@692f6ee

Please let me know what you think...

[overlookmotel]

I'm very happy to add tests and documentation, once it's agreed if this can in principle be merged.

[overlookmotel]

Ping @madbence @yoshuawuyts @jonathanong

[yoshuawuyts]

I'm +0 on this since I no longer using co and actively evade using bluebird.

[ruimarinho]

@yoshuawuyts with Promise.coroutine?

[yoshuawuyts]

@ruimarinho nah, the implicit catch in Promises is incredibly harmful. Learning {errbacks,streams,async} properly in the past few months has made me indifferent towards co.

[overlookmotel]

OK, well I've created a separate module that adds the .use() method. Here it is: co-use.

I still think this would be better off in co though...

[taxilian]

Sure wish the owners would get around to making a decision and either accept this or #244 or finally tell us they aren't willing to do so....

[overlookmotel]

@taxilian Well I kind of gave up and made my own module co-use.

There's also a shortcut for people who want co with bluebird co-bluebird. require('co-bluebird') is the same as require('co-use').use(require('bluebird')).

[d6u]

global.Promise = require('bluebird');
const co = require('co');

This way co will start using Bluebird as Promise.

[taxilian]

@d6u sure, that works... but there are times when you don't want everything to use bluebird, only the things that you have visibility into. It's dangerous to override globals when you don't know what other libraries may be using that global internally, no matter how good the replacement you have. That is a messy workaround which will probably usually not cause problems, but it is not a solution.

[d6u]

@taxilian I understand your argument. This is indeed a hackish solution. But think about those scenarios:

1. Libraries use native Promise. We replaced it with Bluebird. Bluebird is a superset of native Promise. So libraries using native Promise will still work with Bluebird.
2. Libraries use their Promise library of choice. They can do it in ways:
   1. require them, no problem with native Promise now being replaced with Bluebird
   2. It stubs global.Promise. Stubbing without users' consent is wrong. Those libraries will cause trouble with or without Bluebird. Not our fault.

So if we are developing a package to be used by other people, yes, replacing global.Promise is bad. But if we just use it ourselves, I think it's OK.

[d6u]

@taxilian Also for libraries using co, e.g. koa, setting Promise through layers of abstractions could be tedious. Does that mean now every layer will expose a use method now? I think global stub is cleaner.

[taxilian]

I can see that argument, and I can understand it. I completely disagree with it, and I think replacing globals is a bad and dangerous pattern, and I think any library which uses Promise should allow the consumer to override which Promise implementation it uses, but I have no problem with other people doing that in their own application -- as long as they don't do it in a library =]

[overlookmotel]

@d6u I'm with @taxilian on this. Wherever possible best not to override globals as may have unforseen circumstances. In your own application code, well it's up to you, but I still think it's inadvisable. As @taxilian says, in a library it's definitely bad practice. And what if some other library also overrides the same global var?

What's the harm in allowing user code (application or library) to choose their promise implementation without affecting global scope, with a method like .use()?

Anyway, this particular case can be worked around by using my modules co-use and co-bluebird

Next job is to make it work perfectly with CLS...

[d6u]

@overlookmotel How to use co-use with koa though?

[overlookmotel]

I don't know the ins and out of koa, so I can't answer that. I'd suggest that if it's needed koa should also have an equivalent of .use(), or some other kind of hook where you can transform the native promise to bluebird.

[tj]

I think the bottom line is that co is temporary, and I agree that it wouldn't be elegant for Koa to have to expose this as well, since again co is temporary for Koa's case as well. I'd also argue that you should just use native promises, if you have issues with native promises, then replacing it with your own version is fine I think, that's more elegant than requiring that every library ever supports pluggable promises. I appreciate the pull-request, but I don't think it makes sense for now.

[taxilian]

There are some significant benefits to those of us who use bluebird to having co use it as well; the main one for us is the deep stack trace support which it adds. This adds a very small bit of complexity to co, and in return it resolves a concern which is demonstrably shared by a lot of people in the community who are using co. Temporary it may be, but temporary is not likely to end for several years, given the lifespan of LTS versions which will never support async/await.

Given all of that, you're really not willing to accept this small change into the project? That seems a little strange to me; basically that forces those of us who actually need this change (and yes, we actually need it, your other solution doesn't work for us because there are libraries which we use that have problems if they use bluebird, so for stability reasons we can't replace the global Promise implementation) to maintain a fork of co just to keep this functionality.

I understand that you'd prefer not to have people use this, but couldn't you at least allow something that would solve our use case? Do you have any specific concerns with the implementation that perhaps we could resolve by changing to a cleaner way, for example?

[tj]

I'm not saying that isn't the case, but again who is going to add .use() to every promise library in npm? I don't see it happening personally. I don't have anything against the implementation, I just don't think it makes sense. It's difficult to close issues without someone being disappointed, but without making judgement calls you just end up with a big mess, small or not. IMHO this is the exact right case for a polyfill, since that's what bluebird is.

I think spreading this sort of api into every promise library is a much worse design decision than overriding a global. The only thing inherently wrong with overriding the global is that bluebird may be broken, but if it's broken then it's not a promise implementation.

[taxilian]

Every other library I use which supports Promise has added something which gives me this capability. co is the only exception.

[tj]

Sorry, I don't think it makes sense. Like I said if it causes issues with some libraries then bluebird is broken.