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

How to check if something is a Future #40

Open
juandopazo opened this issue Mar 7, 2013 · 10 comments
Open

How to check if something is a Future #40

juandopazo opened this issue Mar 7, 2013 · 10 comments

Comments

@juandopazo
Copy link

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
Copy link

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

@slightlyoff
Copy link
Owner

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

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

@domenic
Copy link
Collaborator

domenic commented Mar 7, 2013

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
Copy link
Collaborator

domenic commented Mar 7, 2013

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
Copy link
Collaborator

domenic commented Mar 7, 2013

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

Branding

Branding was considered in promises-aplus/promises-spec#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
Copy link

dherman commented Mar 7, 2013

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
Copy link
Owner

@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
Copy link
Collaborator

domenic commented Mar 8, 2013

@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
Copy link
Collaborator

annevk commented Mar 8, 2013

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

@slightlyoff
Copy link
Owner

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

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

No branches or pull requests

6 participants