kevinbeaty / any-promise

NOTE: You probably want native promises now
MIT License
179 stars 17 forks source link

Use native promise if it exists, first. #1

Closed RangerMauve closed 8 years ago

RangerMauve commented 8 years ago

As mentioned here, it might make sense to use the native promise implementation first if it exists and only consider user-land implementations if it isn't defined.

julien-f commented 8 years ago

I disagree, native promise implementation is not a choice while a installing a library is.

Library implementations:

While native implementations can be bugged (e.g. Node 0.11.13).

kevinbeaty commented 8 years ago

@RangerMauve I think I agree with @julien-f here, but I don't think I fully understand the concern with npm3 in the referenced issue. Will this only be an issue if a user does not explicitly install a Promise library at the top level? Or is it a concern because there currently isn't a good way for the user to decide to use the global Promise library when using any-promise?

I haven't thought this through, but if it's only a case of the latter, I wonder if we could solve it by adding a new package e.g global-promise that simply exports the global promise and then any-promise could be updated to check for global-promise first. If global-promise is installed at the top-level, the user is opting in to using the global promise. If a library wanted to enforce a global Promise if defined, the var Promise = global.Promise || require('any-promise') would probably work, but I don't know if that's for any-promise to enforce. If it does, it essentially becomes a useless as a library in new Node.js installations.

What do you both think?

RangerMauve commented 8 years ago

I think it's more the former, in that one might be getting back a user-land promise implementation that was installed by some dependency rather than the global as one anticipated, or maybe even some other user-land library.

addaleax commented 8 years ago

The Problem with npm3 is that, if my application A has some dependency B which uses bluebird, npm2 installs the following directory tree:

A
 \ B
   \ bluebird
 \ …

whereas npm3 installs it as

A
\ B
\ bluebird
\ …

which means that any-promise, when required from anywhere in the dependency tree will find bluebird, meaning that the author of A has currently no possibility to express a preference for anything with a lower priority than bluebird, e.g. q or the global Promise object.

RangerMauve commented 8 years ago

Ping @rstacruz @targos, any comments?

kevinbeaty commented 8 years ago

@addaleax Thanks. Yeah, that's a bummer. I'm now wondering if there were cases where this could have been broken even in npm2 depending on the order of loading the dependency tree.

I would like to leave the choice up to the author of the application if it is possible. Anyone have any thoughts on a way to resolve this?

any-promise does allow explicitly setting the promise implementation using the PROMISE_IMPL env variable, but the original intention was mostly for testing. I suppose that is a potential solution, but it would be nice if there was a better way.

addaleax commented 8 years ago

One could check the top-level package.json for an explicitly listed promise depedency and/or some explicit "any-promise": "…" entry. Other modules have defined there own extensions to package.json, and I don’t see a real chance of a namespace collision here.

The bigger problem with this would be finding the top-level package.json: I guess walking up the directories from require.main.filename and inspecting the last found package.json would be more or less reliable, but I’m not sure that there aren’t corner cases in which that wouldn’t work.

RangerMauve commented 8 years ago

If there was an API change, you could specify a function that takes a list of preferences for loading, though that'd make this module lose it's "automagic" feel, and that won't work too well for nested modules, obviously.

julien-f commented 8 years ago

@addaleax browsing the file system looking for the top level package.json looks like an ugly hack.

@RangerMauve good idea but it should optional, IMHO the current, straightforward behavior should be kept.

kevinbeaty commented 8 years ago

@addaleax This is an interesting idea, but I share your concern re. corner cases.

@RangerMauve I'm not sure how this would work. If a library is using any-promise to avoid adding a dependency on a Promise library, how would it choose the list of preferences? The choice would be at the discretion of the library author, not the application author at that point, wouldn't it?

addaleax commented 8 years ago

Yes, it is an ugly hack, no doubt on that. There are some npm modules available for locating parent package.json files, but I’d love something more reliable, too.

RangerMauve commented 8 years ago

@kevinbeaty Good point, bad idea on my part. :P

kevinbeaty commented 8 years ago

@julien-f I share your desire to keep require('any-promise') working directly and leaving the choice to application user. But it does seem like it's broken now. Just looking for a solution. @RangerMauve no ideas are bad ideas. @addaleax ditto . It is an intriguing idea, but I agree that it would be better if there were something more reliable. Maybe there isn't.

Hmm... if the user of the application did install a Promise library at the top level, there are still issues based on loading of the dependency tree, correct?

addaleax commented 8 years ago

@kevinbeaty Yes, the problem is that, ultimately, the npm3 dependency tree is non-deterministic and can basically end up in any way which the dependency specifications allow. And I’d say looking for Promise libraries installed at the top level is basically just as reliable as looking for the top-level package.json?

Maybe letting the application do something like require('any-promise').override(Promise) would be better, but I don’t think that’s easily integrated with the way that require('any-promise') currently works?

kevinbeaty commented 8 years ago

@addaleax Do you have an idea on how require('any-promise').override(Promise) would work? Are you thinking that it would change the module.exports to the overridden implementation and should be called at app startup? It is possible to have more than one instance of any-promise loaded, depending on version correct?

There may be no good solution for all cases. We might need to make a best effort and fall back to the current (mostly arbitrary) priority list.

Loading could look something like this (with potential additions of Promise.override or similar):

1) Use PROMISE_IMPL env variable if set 2) Attempt to find top level package.json and look for a custom key 3) Use current priority list 4) Use global promise

The browser version currently always exports the global Promise and a polyfill is required so I'm not too concerned about adding some complexity in the Node.js version lookup.

I'm starting to wonder if the real answer is no answer and just to use whatever promise implementation is desired and treat all library promises as arbitrary "thenables", wrapping them with your desired Promise implementation.

addaleax commented 8 years ago

@kevinbeaty How you described it is the general idea, yes. I mean, the obvious problem with this approach is that it can’t change any promises retroactively, so one would really have to call it at application startup. And no, I don’t think there’s a way to have more than one instance of any-promise, require() will always return the same object when the parameter resolves to the same physical file.

kevinbeaty commented 8 years ago

I'm thinking about this again and the non-deterministic loading of the Promise implementation is very bothersome. If we can't figure out a way to reliably load a user-specified library, then I think it may be best to remove the "priority list" loading since a different implementation could be non-deterministically chosen depending on install order of the dependency tree.

I no longer think a "best effort" approach is a good idea. If users can't be certain that the promise implementation is one that is expected, then any Promise exported from any-promise would have to be wrapped anyway if using implementation specific methods since it can't be guaranteed that they will have the correct implementation.

I'm not sure where that leaves us. We could always export PROMISE_IMPL if the env is set (Maybe changing name to ANY_PROMISE_IMPL or something), and use global Promise if not. Since we can only currently guarantee that some promise implementation is exported, users can only expect the base level features anyway, and now that new Node.js versions support a decent Promise implementation, it may be best to default to that one anyway. That leaves a lot to be desired, but might be the best bet. So I'm now coming around on @RangerMauve suggestion to prefer global promise first.

I'm open to any other solutions, but I do think it's very important that whatever is decided needs to be deterministic behaviour.

RangerMauve commented 8 years ago

The way I've been using any-promise in my libraries is to assume that the return will only really have .then() and .catch() implemented since that's the only thing in the spec. Basically, I use it as a polyfill where someone else will specify the implementation. I doubt that there are many libraries out there that make a different assumption unless they're at the top level and installing the promise library themselves.

kevinbeaty commented 8 years ago

@RangerMauve Yes, that's all I've been assuming as well. But I do think some users are attracted to the claim in the README that the choice is "up to the end user", which is not actually true.

I've actually started to use the global Promise in new code anyway, so maybe there is nothing to do here and a note should be added to the README.

kevinbeaty commented 8 years ago

Note that #2 attempts to find top-level package.json and looks for a custom key or installed Promise implementation. Refer to #2 for a discussion on that approach.

kevinbeaty commented 8 years ago

Copied from #2, I'm currently thinking using this order of resolution:

  1. Use ANY_PROMISE env if set
  2. Use PROMISE_IMPL env if set (backwards compatible)
  3. (Maybe) Use global.ANY_PROMISE if set (user can set at app load)
  4. (Maybe) Use custom "any-promise" value in app root path package.json if found and set (#2)
  5. Use global.Promise if set
  6. Throw error

And remove the priority list lookup. I'm up for alternative names to "ANY_PROMISE".

Thoughts?

kevinbeaty commented 8 years ago

I've added #4 which I believe addresses these issues and still allows user to select a preferred implementation. It's actually pretty simple.

  1. If the end user does nothing, defaults to global.Promise
  2. If the end user has a preferred implementation, she must register on "app startup", i.e. before any call to require("any-promise")

Registration looks like this:

// at app-startup (top of the index.js file or similar)
// can be called multiple times, but must be with the same implementation
require("any-promise/register")("bluebird")

// any time after the above in any module (library or otherwise)
var Promise = require("any-promise")

I've removed the "priority list" and the PROMISE_IMPL environment variable. I also don't think #2 is really required any more as registering an implementation at startup is more reliable and no more difficult than registering an implementation in package.json.

If there are no objections, I will merge soon.

addaleax commented 8 years ago

require("any-promise/register")("bluebird")

That’s neat! Sounds good to me!

addaleax commented 8 years ago

I’ve thought about this a bit – Your proposal of requiring any-promise/register in the top-level application will not be available in the npm2 situation (or npm3 with different versions of any-promise installed), because any-promise might not end up in the top-level node_modules/ directory. And I’m not even sure whether that can be fixed in some way… :/

kevinbeaty commented 8 years ago

What if we use a global variable in register? I've been avoiding it, but it might be the only way. That will be available across modules right?

require("any-promise/register")("bluebird")
// set global['@@ANY_PROMISE_IMPLEMENTATION'] = "bluebird"
//  (or anything else not likely to conflict)
// load and return per current implementation

This is any-promise.js:

module.exports = require("any-promise/register")()

So in top level module, this would return prior registration. In other modules, it would load the implementation specified by the global variable

kevinbeaty commented 8 years ago

Or maybe actually set the implementation, so we have the same one:

// in register:
global['@@ANY_PROMISE_REGISTRATION'] = registration // the loaded promise impl
// registration contains {Promise, implementation} 

We would only set a global variable if user calls register, so the global variable is set because of a user action. This may be OK. If global variable is not set, we would still fall back to global.Promise

addaleax commented 8 years ago

The problem is that require("any-promise/register") might just not work in the top-level directory in the npm2 situation: The dependency tree might look something like

A
\ bluebird
\ B
  \ any-promise

which would leave A (i.e. the top-level app) with no clue how to access any-promise. You might be able to circumvent that by manually specifying any-promise as a top-level dependency or something like that…

kevinbeaty commented 8 years ago

You might be able to circumvent that by manually specifying any-promise as a top-level dependency or something like that…

I think you would have to

kevinbeaty commented 8 years ago

It would only be required if the user wants to override the global.Promise implementation, so it is the users choice

addaleax commented 8 years ago

@kevinbeaty Just for clarification – you want to always provide global.Promise by default, i.e. completely drop node v0.10 support?

kevinbeaty commented 8 years ago

you want to always provide global.Promise by default,

Yes. I'd like the behaviour to be deterministic, and the global.Promise is good enough for >= 0.12

i.e. completely drop node v0.10 support?

Not necessarily, just de-emphasize it. Node < 0.12 would require registration or a polyfill.

kevinbeaty commented 8 years ago

Maybe instead of a global variable, we should just document that to use any-promise/register the user should require any-promise at the top level and npm dedupe to ensure only one version of any-promise is installed. I don't really foresee many version changes to this library due to it's limited scope, so npm dedupe should work well.

I could potentially add back the PROMISE_IMPL env variable for cases where this wouldn't work (maybe with deprecation). This could also allow a workaround if libraries are pinned on the old any-promise version.

I could also potentially fall back to the old behavior (PROMISE_IMPL + priority list) for Node.js versions <0.12 (Node.js >= 0.12 will prefer "global.Promise"). That way we wouldn't use the buggy Promise in v0.11.x and versions without a global Promise wouldn't start throwing errors when they worked before.

kevinbeaty commented 8 years ago

I've updated #4 to do the following on require('any-promise'):

  1. Use previous registration through require('any-promise/register')('bluebird')
  2. Implementation specified by PROMISE_IMPL
  3. global.Promise if node.js version >= 0.12
  4. Auto detected promise based on first successful require of known promise libraries. Note this is a last resort, as the loaded library is non-deterministic. node.js >= 0.12 will always use global.Promise over this priority list.
  5. Throws error

2 and 4 are mostly to help prevent code previously running without error in any-promise versions before this upgrade. I may decide to remove 2 and 4 in a future version so they are undocumented. But if they prove to be useful, I'll leave them be.

I also added a Node.js version check and do not return the global.Promise if <= 0.11 to avoid buggy implementations. There is a corner case where current installations are relying on the global Promise in these versions, but this should probably be an error anyway and I think it's increasingly unlikely that there are may installations where this exists. It's easy enough to fix if there are.

I think I'm going to wait on the global variable registration. I'll consider for another version if there are enough issues with npm dedupe not working and a user wants to register a custom implementation.

RangerMauve commented 8 years ago

:+1: I like it. Should this be marked as closed?

addaleax commented 8 years ago

:+1: Cool!

kevinbeaty commented 8 years ago

Published any-promise@0.2.0

Thanks, all, for your feedback!