Feature/byop #244
Feature/byop #244
Conversation
|
If you want to use the same Promise library everywhere, why not overwrite |
|
@cesarandreu I could be wrong, but it feels like setting a particular promise library for co should be a separate implementation detail from overwriting the entire global ES6 promise (which is what this PR seems to address specifically). Thoughts? |
|
I agree with @joshbeam; while there may be times when overriding the global makes sense, generally I prefer to avoid overriding globals in order to prevent problems with minor implementation detail differences and modules or libraries which may not have specific unit tests to test alternate promise libraries, etc. I added unit tests to this PR to make sure it would work with another Promise library, but other libraries which use ES6 promises may not have that. I submit that overriding a global should not be the required way of doing something that was, in actuality, so simple to implement. It does not make the code harder to read or to understand, it does not add a significant burden for code maintenance, and it does prevent the need to follow what I consider to be a dangerous practice -- overriding global built-in types. |
| @@ -47,7 +64,7 @@ function co(gen) { | |||
| // we wrap everything in a promise to avoid promise chaining, | |||
| // which leads to memory leak errors. | |||
| // see https://github.com/tj/co/issues/180 | |||
| return new Promise(function(resolve, reject) { | |||
| return new coPromise(function(resolve, reject) { | |||
There was a problem hiding this comment.
Are we sure this syntax will work for whatever promise library the user wants? If not, should there be some sort of whitelist, or do we want it to just throw the error it would normally throw if the promise library for whatever reason doesn't work (i.e. fail loudly, but without a specific error message saying "this promise library doesn't support this syntax").
Only reason I bring this up is because this seems to have only been tested with bluebird.
There was a problem hiding this comment.
This is a very good question; my reasoning for keeping this is twofold:
- The two promise libraries I've looked at (Q and bluebird) both support this method, though Q requires you to use Q.Promise (and yes, I've tested it and it works)
- It is trivially easy to make a wrapper around any promise library which will support this interface; some libraries may require that anyway in order to support all of the specific function names which co uses. I figure this is fair, though, since co uses the ES6 standard so any promise library wishing to use it will just need to either support that interface or have a wrapper which allows it to.
taxilian
force-pushed
the
feature/BYOP
branch
from
59f2c11 to
020e6b9
Compare
|
@joshbeam valid point on the comment format; fixed. |
|
This same basic pattern is used in mongoose, in case that helps you feel any more comfortable with it =] |
|
Interesting; I thought I'd looked through the pull requests and missed that one. I believe they are functionally equivalent; I guess the only real difference is in a few of the details of implementation and that I provided tests to go with this. Given how long the other pull request has been waiting for review, do I take that to mean that I should give up and use his fork? That would probably work for me. I don't care which I use, but I need the functionality. |
|
I think the use case is valid and I'm experimenting with |
|
I'm meh on it personally, I think having something like this is cleaner in a sense, but at the same time it would be such a massive pain to go set the promise lib to something new for every single package out there in your app. The polyfill route is reasonable in that sense I suppose. Ideally co doesn't even exist (with async/await etc) so I probably wouldn't bother |
|
Need! 👍 |
|
I like the way mongoose implements it: http://mongoosejs.com/docs/promises.html#plugging-in-your-own-promises-library |
|
Forgot to close. I'm firm on not supporting arbitrary Promise implementations. Libs like bluebird et al should work with promises, not implement their own (or optionally act as a polyfill as well). If you need a polyfill, then use a polyfill :). |
This allows a user to provide their own promise library; this is useful as there are libraries which offer benefits over the ES6 Promise object but are also API-compatible (or could be easily made to be so). One particular benefit of using the same promise is that some libraries (notably q and bluebird) support long stack traces across multiple callbacks which is useful for debugging; however, when using a different promise in the middle (or particularly the end) of the chain breaks this functionality.
This is referred to in some libraries as Bring Your Own Promise (BYOP) and is very important to those of us who use third party promise libraries. I have copied the promise unit tests to a byopromises unit test file which performs all of those tests with bluebird set as the Promise type and verifies that it works.