slightlyoff / Promises

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

Pass resolver to then? #46

Closed slightlyoff closed 11 years ago

slightlyoff commented 11 years ago

Merging of returned Futures is a good feature, but by default it creates a lot of objects for the GC to clean up. I worry that our current design creates the need for alloc when it isn't strictly necessary. One way around this is to provide the resolver to the then callbacks, allowing them to drive the lifecycle of the vended future directly (vs. merging). Merging is, of course, still supported, but savvy then() users could prevent mandatory alloc by returning the Resolver instead of a Future, signaling that they want to drive the process.

Crazy?

sicking commented 11 years ago

The main concern I would have is giving up future extensibility. I.e. if we pass the resolver as the second argument it means that we couldn't use that for something else.

For example, I've always found it an intriguing idea to change the signature of accept and resolve to accept(...args) and then pass all the arguments to then using the spread operator. Though that definitely blows away any chance of ever making the types of expansions suggested in this bug. It also means that .then-handlers that return data using a raw return value is at a disadvantage since they can't specify multiple result values.

So I should probably give up on that idea :-)

But maybe it's worth calling then with the second argument being a dictionary with a resolver property set to the resolver. That way we can add more arguments later (such as the Future itself if needed).

I.e. the code would look something like:

doAsync.then(function(v, { resolver: r }) {
  r.accept(v+1);
}).then(...);

But maybe that means that we're avoiding to create the Future but instead end up creating a dictionary.

domenic commented 11 years ago

I don't think this is crazy, but it falls on the side of not-great idea rather than good one, mainly for portability reasons. In general, I think then is not the right place to make changes to the basic thenable protocol. A separate method, like thenWithResolutionCallbacks, would be better.

then should have basic, minimal functionality so that promise utility libraries can be agnostic to which promise implementation they target. For example, a utility like this:

function parseJSONPromise(promise) {
  return promise.then(function (string) {
    return JSON.parse(string);
  });
}

is reusable because it uses only the basic protocol, and can be reused no matter if you're using a raw DOMFuture, or if you're consuming Q/RSVP promises proxying remote objects via Q-Connection/Oasis, or if you're using WinJS promises created by the Windows Runtime, or...

However, a function like this

function parseJSONPromise(promise) {
  return promise.then(function (string, resolutionCallbacks) {
    resolutionCallbacks.resolve(JSON.parse(string));
    return resolutionCallbacks;
  });
}

is not portable and will behave very differently when used with DOMFuture promises versus when used with promises from other systems. That is, it fulfills with resolutionCallbacks when used with other promise implementations, whereas it fulfills with JSON.parse(string) when used with DOMFuture promises.

If we instead specified a separate function, say thenWithResolutionCallbacks, it'd be very clear that the utility function only works with DOMFuture promises, and does not follow the standard then contract for return values:

function parseJSONDOMFuture(domFuture) {
  return domFuture.thenWithResolutionCallbacks(function (string, resolutionCallbacks) {
    resolutionCallbacks.resolve(JSON.parse(string));
    return resolutionCallbacks;
  });
}

Finally, it's worth noting that the use cases for this method are pretty small. The only time it would avoid allocations would be when interfacing with non-promise async code inside the settlement handlers. (The above is actually an example of when you shouldn't use it---but, I'd be afraid people would.) In my experience working with promises, these interfaces almost never occur inside of the settlement handlers, but instead occur at the start of the promise chain.

So I'd argue YAGNI, and that at the very least we should wait until people run into garbage-collection harm before considering extending the API with something that allows user optimizing.

annevk commented 11 years ago

We can reconsider a feature like this in future if this actually turns out to be a problem.