Incorrectly identifying promises

[medovob]

I've encountered an issue in the latest (7.1.0) release relating to commit 492d53e705acb6c

The issue is that isPromise makes a huge assumption that any object with a then method is a Promise. If the object is not a Promise, then will be called which may not be desirable.

This can be demonstrated very simply

Immutable({then:function() { console.log("should not be called") }});

I encountered this when using superagent which provides a thenable interface but is not technically a Promise. I suspect there may well be other libraries with a similar approach.

[pladaria]

Just for reference: https://promisesaplus.com/

Terminology

1. “promise” is an object or function with a then method whose behavior conforms to this specification.
2. “thenable” is an object or function that defines a then method.

I think it's fair to assume that an object or function with a then method is a promise. Anyway, if this gives conflicts, this behaviour could be overridden via config.

[medovob]

Thanks @pladaria but I think you've just highlighted the clear distinction between the two. Its not safe to assume that a "thenable" conforms to the Promise specification.

There must be a better way to identify a Promise but I'm not sure what that is

[medovob]

Perhaps something like:

function isPromise(obj) {
  return typeof obj === 'object' &&
    typeof obj.constructor === "function" &&
    typeof obj.constructor.resolve === 'function';
 }

[LukeusMaximus]

Is identifying a web standards promise sufficient? i.e.:

function isPromise(obj) {
    return obj.constructor === Promise;
}

If not, I'd go with a solution that had the methods isPromise and isThenable to prevent confusion.

[medovob]

Yeah - that was my initial thought too but I think the problem is that environments might not support native promises and even in supporting environments, a developer may be using a Promise polyfill or a Promise compliant lib (like RSVP or Bluebird) and these won't necessarily referenced as the the global Promise which is why I was suggesting testing the object for compatibility rather than strict equality.

You could also also use the "resolver" to return the promise rather than calling then() and have an additional check to make sure that the "resolved" object is "thenable"

if (obj.constructor.resolve) {
  var resolved = obj.constructor.resolve(Immutable(obj));
  if (typeof resolved.then === "function") {
    return resolved;
  }
}

[sandro-pasquali]

Another way to look at this is to normalize all values to promises via Promise.resolve():

let imm = Immutable(Promise.resolve(<PromiseOrAnythingElse>))

Since it would seem that your implementation expects some Immutable-ized values to be Promises and others not, I think this would mean a consistent interface (always Promises) could be achieved.

This may or may not be possible in your case of course...