slightlyoff / Promises

DOM Promises IDL/polyfill
Apache License 2.0
154 stars 28 forks source link

How to check if something is a Future #40

Open juandopazo opened 11 years ago

juandopazo commented 11 years ago

Hi!

I noticed the polyfill is checking for the existence of both the then and done functions in an object. Why check for done? Pretty much every implementation out there is only checking for then. Examples:

unscriptable commented 11 years ago

And many implementations don't support .done().

slightlyoff commented 11 years ago

I'm not tied to the .done() check. Fixing.

I know that @dherman also had concerns along these lines.

domenic commented 11 years ago

I'm not sure .length === 2 is correct semantics either, as with ES6 default paramters I would implement

DOMFuture.prototype.then = function (
    onaccept = function (x) { return x; },
    onreject = function (x) { throw x; }
);

and of course that has .length === 0.

domenic commented 11 years ago

Put another way, the number of "required" parameters is zero, not two, and generally a function's .length reflects the number of required parameters.

There's also compatibility with the many implementations that implement progress as the third argument, e.g. Q, WinJS, and when. They all have .length === 3.

domenic commented 11 years ago

Generally there are two approaches here: branding and no-branding.

Branding

Branding was considered in https://github.com/promises-aplus/promises-spec/issues/2 and generally rejected, with @wycats in favor and myself slightly in favor, but most others opposed.

The basic idea is that we "brand" Promises/A+-compliant promises e.g. with promise.then.aplus === true, and only treat thenables as potential promises if they pass the brand test. This impacts return values from onFulfilled and onRejected, and would impact the behavior of the resolve callback in the Promise constructor.

Note that ES6 symbols are not a great solution here, as the non-collision benefits are not important and the symbol would need to be system-supplied for interoperability, in which case a symbol and a string are equivalent.

It would mean that the following code does not work:

var promise = aplusCompliantPromise.then(() => jQueryPromise);

var promise = new Promise(resolve => resolve(jQueryPromise));

i.e. the promise becomes fulfilled with jQueryPromise, not with the fulfillment value of jQueryPromise.

Instead you need to provide an assimilation function, e.g.

var promise = aplusCompliantPromise.then(() => Promise.from(jQueryPromise));

var promise = new Promise(resolve => resolve(Promise.from(jQueryPromise)));

Implementations would often alias this as e.g. Q or when, so e.g. () => Q(jQueryPromise), which is a bit more palatable.

Still, it breaks generic interoperability, and would be a major backward-incompatible change for most promise libraries in use today. I am not sure whether implementers would be willing to shoulder this change.

No Branding

No branding means we use the current Promises/A+ putativeThenable && typeof putativeThenable.then === "function" test to detect thenables. It assumes that all thenables you pass to the promise implementation, through either return or resolve, should be treated as promises.

This forces you to wrap any non-promisey thenables, e.g.

var promise = aplusCompliantPromise.then(() => [nonPromiseyThenable]);

var promise = new Promise(resolve => resolve([nonPromiseyThenable]));

promise.then(([nonPromiseyThenable]) => { ... });

One major hazard I see is that it further exacerbates the already-big dangers of treating objects as hashes, especially for user input. E.g. you could envision a user crashing your server by adding a query string ?then=foo if the results of parsing the query string become { then: "foo" } and the results of that parsing, or something derived from those results, are ever returned from a promise handler.

dherman commented 11 years ago

D'oh, I filed Issue #41 around the same time as this… I think my suggestions go into a bit more detail than @domenic does above, but I agree with him and @wycats that some sort of branding would be better. I really hope we can do better than duck-testing.

Dave

slightlyoff commented 11 years ago

@domenic : hadn't considered the default arg situation, but I honestly don't feel that's apropos. Libraries will do what they need to to fit our contract. That's independent from coming up with a good contract.

I'll respond in #41, but I'm not hopeful about branding. It ends up at cached duck-typing for anything that needs to work cross-frame.

domenic commented 11 years ago

@slightlyoff more or less agree. But, eventually you'll want to spec then.length. (Does IDL do this already, indirectly? Since both params are optional?) And I think the correct value is probably 0 going by the way it's defined in ES6. Not really important for now.

annevk commented 11 years ago

IDL defines length and if there's optional arguments it will indeed not count those.

slightlyoff commented 11 years ago

I've removed the .length check in the polyfill.