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

Pass resolver to then? #46

Closed
slightlyoff opened this issue Mar 11, 2013 · 3 comments
Closed

Pass resolver to then? #46

slightlyoff opened this issue Mar 11, 2013 · 3 comments

Comments

@slightlyoff
Copy link
Owner

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

sicking commented Mar 12, 2013

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

domenic commented Mar 12, 2013

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

annevk commented Apr 25, 2013

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

@annevk annevk closed this as completed Apr 25, 2013
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

4 participants