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

Alternate Promise libraries #425

Closed
RyanZim opened this issue May 10, 2017 · 23 comments
Closed

Alternate Promise libraries #425

RyanZim opened this issue May 10, 2017 · 23 comments

Comments

@RyanZim
Copy link
Collaborator

RyanZim commented May 10, 2017

As per #403 & overlookmotel/fs-extra-promise#27, people are asking to use custom (non-native) promise implementations.

I don't know if this is a use-case we should support or not.

@rijnhard
Copy link

rijnhard commented May 11, 2017

I went into the universalify code and using the method option that was discussed, the code changes involve adding a module scoped variable and a setter function which even with nodes small module philosophy is still highly negligible.

However when I looked at how you used it in fs-extra I was expecting a final transform step (from callbacks to universal) at the top level export, not at each function definition. Which means that it wont so easy to carry the custom promise implementation into fs-extra. Which means a fair bit of code changes.

Honestly I'd say that it's still worth it to provide it, to make the unknown portion of the 1.1 million odd daily downloaders of Q or bluebird promises that could use fs-extra lives a lot easier.

@nrser
Copy link

nrser commented May 20, 2017

I'd like to voice support for this feature. I'm delighted to see universal promise / callback functionality added out of the box (thanks!), but I've got bluebird integrated across my libraries and it would be great to have that functionality available in filesystem calls without an additional layer of wrapping.

@bsberry
Copy link

bsberry commented May 22, 2017

+1 to this, particularly because having bluebird .finally at the end of a promise chain when dealing with filesystem operations is very helpful. Especially on windows where filesystem operations can fail somewhat mysteriously, having the confidence of a finally that will always run is very useful.

@RyanZim
Copy link
Collaborator Author

RyanZim commented Jun 1, 2017

@jprichardson @manidlou Thoughts?

@jprichardson
Copy link
Owner

If we can do a simple setPromise() without too much trouble, then it may be worth it. But what I don't understand is why people don't just do global.Promise = require('bluebird') or whatever their favorite Promise implementation is. Wouldn't that solve the problem?

@RyanZim
Copy link
Collaborator Author

RyanZim commented Jun 2, 2017

But what I don't understand is why people don't just do global.Promise = require('bluebird') or whatever their favorite Promise implementation is. Wouldn't that solve the problem?

Yeah, I don't get it either. Someone care to explain?

@manidlou
Copy link
Collaborator

manidlou commented Jun 2, 2017

But what I don't understand is why people don't just do global.Promise = require('bluebird') or whatever their favorite Promise implementation is. Wouldn't that solve the problem?

I am wondering too. Is there any specific reasons that users don't want to do this?!

@rijnhard
Copy link

rijnhard commented Jun 2, 2017

@jprichardson @RyanZim @manidlou
because you can use the setPromise method once at the entry point to your application, but with global.Promise = require('bluebird') you have to do it every time before you include fs-extra.

At least i had issues with it, open for correction.

@RyanZim
Copy link
Collaborator Author

RyanZim commented Jun 2, 2017

@rijnhard That isn't necessary; just set global.Promise = require('bluebird') at the top of the entry point to your app. (Before requiring fs-extra or anything else).

@rijnhard
Copy link

rijnhard commented Jun 6, 2017

@RyanZim Welllllllllll then theres no reason. Assuming using es6 imports it's still possible then its just ignorance on my part.

@RyanZim
Copy link
Collaborator Author

RyanZim commented Jun 6, 2017

@rijnhard With ES6 imports, you'd do:

import bluebird from 'blubird'
global.Promise = bluebird

@rijnhard
Copy link

rijnhard commented Jun 7, 2017

I find it ironic that i didn't know this, even though it's pretty obvious.

I see no reason to provide a setPromise anymore, maybe some documentation demonstrating how to set the promise implementation, even though its not something that should be documented here it will surely help others.

I have some refactoring to do.

@thecjharries
Copy link

thecjharries commented Jun 12, 2017

I recently stopped overriding the global Promise for a few reasons. It made my immediate work easier, but I slowly came to the realization that it was going to make maintenance a nightmare. Different coders like different Promise libraries, none of which agree on an API beyond the spec. If I pass my code off to someone that uses Q, for example, they can certainly handle it, but they'll either have to constantly remember the Promise API could change per project or refactor any failed tests to use their API. When I explicitly state I'm using Bluebird or Q, anyone looking at any of my code knows exactly which library I'm using.

More importantly, messing with global can have unintended consequences. I slapped together a quick illustration. If this build is failing, it means multiple developers overriding global.Promise (i.e. you did it, a package it, your coworker did, etc.) is a bad idea:
Build Status

(Edit: I should have gone to sleep last night and built that this morning. It's now failing for the right reasons.)

@rmvermeulen
Copy link

Maybe any-promise is a solution here? It defaults to the native Promise, so nothing changes for users who don do anything. Imo it's great to have a single place to set the promise implementation for a bunch of libraries, without touching the global state.

@RyanZim
Copy link
Collaborator Author

RyanZim commented Jun 15, 2017

In regards to RyanZim/universalify#5:

Frankly, I don't get the point. How is overriding the promise implementation used by universalify and anything else that depends on it more "evil" than overriding global.Promise?

If you're a library, yes, overriding global.Promise is evil; but the same applies to overriding universalify's Promise. I'd also contend that a library returning non-standard promises is evil as well.

If you're an app, what's the harm of overriding global.Promise?

@thecjharries
Copy link

You're not overriding universalify.Promise everywhere, you're overriding it in the specific context where it should be overridden. It has to be defined per file, which means you know what you're doing. If you're overriding it in a library, you're either limiting its scope and not exposing it, you're warning dependents you've overridden it (i.e. you're letting them know your library returns Bluebird not Promise), or no one uses your library when it breaks. No matter what, you're changing the behavior properly within its own scope.

@RyanZim
Copy link
Collaborator Author

RyanZim commented Jun 15, 2017

You're not overriding universalify.Promise everywhere

@thecjharries Erm, yes, you are. CommonJS modules are singletons. Anything that imports universalify afterwards will get your custom promise implementation.

@thecjharries
Copy link

@RyanZim Huh, I apologize. I'll modify that pull request when I get off this evening to use a factory. With a factory, my argument still applies. Again, sorry. I should have remembered that.

@thecjharries
Copy link

I've been thinking about this all afternoon, and I don't see any good way to do it without a major version bump, which is a slightly ridiculous request.

Bluebird offers promisifyAll, which is what I've been using. I can't find anything similar in Q's API, and I'm too lazy to hunt down other libraries. It would be really nice to be able to call fse.readFile instead of fse.readFileBluebird, but, again, it doesn't seem possible without a major API change. Thanks for all the feedback.

@develar
Copy link

develar commented Jul 15, 2017

Bluebird offers you long stack traces. In my lib I use special instance of bluebird (it is not singleton, you can have any version of bluebird with any config). That's why I use my own wrapper fs-extra-p and Promise support in the fs-extra is not useful for me (and even more — should be not used to ensure that stack traces will be clear).

@RyanZim
Copy link
Collaborator Author

RyanZim commented Oct 18, 2017

Going to close this out as I've yet to hear a good argument for not just overriding global.Promise in your app.

@RyanZim RyanZim closed this as completed Oct 18, 2017
@johannes-scharlach
Copy link

Let me give it a try:

If you're a library, yes, overriding global.Promise is evil;

That is a believe I share. The case I have is a private npm module, which I'd consider as a library in this sense, so I don't want to overwrite global.Promise. But within that module I'd like to use bluebird features (.tap and .finally are particularly useful in places where I'm using fs-extra). Probably around two thirds of our code are in such private modules, so it's the regular case where we'd like to use bluebird.

In order to use bluebird with fs-extra, we currently have to either wrap the fs-extra call in a promise chain like Promise.resolve().then(() => fse.readFile(filename)).finally or I have to trust that all applications in which this package is used overwrite global.Promise. In addition, I have to overwrite it in all my test files.

For a while we did the global.Promise overwrites, but now we're starting to remove them again, since that can easily be forgotten. This lead to code that worked in some scripts and didn't work in others. It's particularly annoying if your tests are passing but your regular code is failing, because you didn't set global.Promise.

@johannes-scharlach
Copy link

johannes-scharlach commented Dec 13, 2017

I've understood my requirements a little better now: I (as a library) definitely don't want to change the behaviour of fs-extra for everyone, so I need to make sure that if I want to use bluebird, such a promise setting affects only me.

The way I'm now handling the bluebirdification of fs-extra is during the require phase:

const fse = require('ensure-bluebird')(require('fs-extra'))

The ensure-bluebird package is something I just wrote to fix this very problem – I hope others in this thread may also find it useful.

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

No branches or pull requests

10 participants