domenic / promises-unwrapping

The ES6 promises spec, as per September 2013 TC39 meeting
1.23k stars 94 forks source link

Should we kill Promise.resolve? #20

Closed erights closed 10 years ago

erights commented 10 years ago

Given Promise.cast, no useful functionality is served by the existence of Promise.resolve. It is not needed.

annevk commented 10 years ago

We could make it an alias for cast. @dherman argued to me some months ago that having an alias is not so bad and you expect Promise.resolve() to be there given Promise.reject().

annevk commented 10 years ago

I guess the argument for not making it an alias is that we'd want to expose the Resolve() primitive. (Don't feel strongly about that.)

erights commented 10 years ago

In the interests of finding an approximately-minimal subset, consisting of the stuff we need now, I suggest we omit Promise.resolve for now. Again, we can always add it back in later.

That said, I think omitting it is the right long term decision as well. It adds no useful functionality while still paying the price of more API surface.

annevk commented 10 years ago

Yeah, seems fine. (Though one could say the same about thenable support I suppose...)

erights commented 10 years ago

(Though one could say the same about thenable support I suppose...)

@annevk It would be great if we could. But the subset has to be a compatible subset of what we hope to grow into. Without thenable assimilation in early, it would be incompatible to add it in later.

domenic commented 10 years ago

I don't like this. Promise.resolve is very nice for symmetry with Promise.reject. Consider writing test code that creates stubs:

var successStub = sinon.stub().returns(Promise.resolve({ foo: "bar" }));
var failureStub = sinon.stub().returns(Promise.reject(new Error("bad!")));

// vs.

var successStub = sinon.stub().returns(Promise.cast({ foo: "bar" }));
var failureStub = sinon.stub().returns(Promise.reject(new Error("bad!")));

And in theory, always creating a new promise could be useful, e.g. if you are trying to create distinct keys in an identity map. (I don't really have a practical example of this.)

tabatkins commented 10 years ago

In my experience writing objects with both "create" and "cast" methods, both have proven useful in different circumstances.

It would be annoying if, in order to create a fresh fulfilled promise, I needed to go through the Promise constructor, while I could use the static operation if I was rejecting one.

erights commented 10 years ago

@tabatkins Hi Tab, could you explain an example where you found it useful to make a fresh promise that was behaviorally identical to a promise you already have?

erights commented 10 years ago

@domenic Given the purpose of the subset we're trying to define here, "in theory ... could be useful" sounds to me like a reason not to include something. An actual example where it is useful might better make the case.

ghost commented 10 years ago

Symmetry is a good thing even in minimalism.

tabatkins commented 10 years ago

Actually, yes, I can give a concrete example, since I just wrote some spec text that needs the "resolve" semantics yesterday.

Check out http://dev.w3.org/csswg/css-font-load-events/#font-face-ready. I want fontface.ready() to always return a fresh Promise, as that's what you expect from methods like this (and FontFaceSet#ready() also returns a fresh promise each time).

The easiest way to handle all of this to have a single internal promise which I resolve/reject at the appropriate time, and then create fresh promises when the method is called and resolve it with the internal promise.

I could go without this, of course - I could track the resolve/reject status separately, and switch based on that, resolving or rejecting the fresh promise directly. But then I'd also need to keep track of the promises when the font is still pending, so I could resolve/reject all of them at the right time, too. That's a lot of machinery for something that is trivial to do myself.

Or I could manually invoke the necessary code to do resolution semantics myself, but again, that's just adding silly weight to a simple semantic.

In code, it's the difference between:

return Promise.resolve(this._internalReadyPromise);

and:

return new Promise((resolve)=>resolve(this._internalReadyPromise));

As @zenparsing said, maintaining symmetry even for small cases is important for API predictability.

erights commented 10 years ago

@tabatkins I'm trying to understand this example. Why would you not do

return Promise.cast(this._internalReadyPromise);

?

Or, if (as the name suggests) you know that this._internalReadyPromise is a genuine promise, why would you not do

return this._internalReadyPromise;

?

It seems much more straightforward. What functionality is lost?

erights commented 10 years ago

Sorry, I hit the wrong button. I had not meant to close the issue.

domenic commented 10 years ago

@tabatkins Thanks for the example; I hope we can flesh it out into something reasonable, since I want Promise.resolve for symmetry reasons alone. But can you explain why your API is designed as is? Why not just a ready property that always returns the same promise?

tabatkins commented 10 years ago

Mostly for symmetry for FontFaceSet#ready(), which does need to return a fresh promise each time (or else do much complicated things to keep track of state, which would involve sometimes returning the same promise and sometimes a different one, depending on when you asked).

Since one is a method, it's more consistent to make them both methods, and methods by convention return new objects each time.

tabatkins commented 10 years ago

But also, for serious, "minimal" doesn't let you ignore "consistent" as a criteria. Having both static methods is good. Having neither is less good, but acceptable (but I wouldn't want to change to that now). Having one but not the other is crazy-times.

erights commented 10 years ago

I do not care enough about this issue to continue objecting. All the strong feelings seem to be for keeping Promise.resolve, so I will not stand in the way of declaring this as the consensus. For the record, I still think this is a bad idea, but only mildly so.