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

Use promises in Meteor core #3776

Closed
deanius opened this Issue Feb 20, 2015 · 9 comments

Comments

Projects
None yet
5 participants
@deanius
Contributor

deanius commented Feb 20, 2015

Meteor.call only accepting a callback leads invariably to callback nesting issues for sequential calls.

I created a package showing one possible API https://atmospherejs.com/deanius/promise as an illustration of one possible way to allow for a Promise, a meaningful return value to be returned, by introducing Meteor.promise, but I think we can do better than this.

Perhaps on the client, if the callback is omitted, a promise should be returned automatically.
This would be as close to isomorphic as possible, given the lack of futures/fibers on the client, and is the pattern used by a few libraries such as: https://github.com/philschatz/octokat.js, and https://github.com/percolatestudio/meteor-google-api/

I'm not a fan of jQuery promises, prefering Q, for reasons explained here: https://t.co/PNenuDtf5p, but their advantage is that jQuery is already included as a dependency in Meteor.

I think a solution to callback-hell is warranted, and this should be in core. Are other places, other than Meteor.call (and Meteor.apply and ilk), where this could be useful? Perhaps creating a subscription could return a promise for its readiness Meteor.subscribe('sub').then( ... ). Probably a few other places, as well, let's discuss.

@glasser

This comment has been minimized.

Member

glasser commented Feb 20, 2015

Thanks for the feature request! We welcome discussions about how to make Meteor better. If you haven't yet, check out our wiki page about feature requests on GitHub.

Honestly, the main approach to avoiding callback hell client-side in Meteor is reactivity. Much of Meteor client-side is based around being able to use reactivity and eventual-consistency instead of event-based callbacks. So you can use the reactive ready() callback for subscribe readiness, etc.

Meteor already provides fibers (server-only), reactivity (client-mostly), and callbacks. I'm not sure that providing yet another paradigm (promises) will make Meteor simpler rather than more complex. Isn't it pretty easy to provide your own wrapper around Meteor.call that uses your promises implementation of choice?

@glasser glasser added the feature label Feb 20, 2015

@Slava

This comment has been minimized.

Member

Slava commented Feb 20, 2015

As a different approach to solve the callback-hell is to use Tracker and its recomputation primitives. You can come up with different reactive schemes to rerun the function when the result is available. An example would be https://github.com/stubailo/meteor-reactive-method

@deanius deanius referenced this issue Feb 22, 2015

Closed

Comment: #1

@the1mills

This comment has been minimized.

the1mills commented Feb 23, 2015

Doesn't Meteor.wrapAsync(Meteor.call()) do anything to solve this?

@the1mills

This comment has been minimized.

the1mills commented Feb 23, 2015

@deanius

This comment has been minimized.

Contributor

deanius commented Feb 23, 2015

@the1mills No, per http://docs.meteor.com/#/full/meteor_wrapasync, on the client a callback is always required.

@deanius

This comment has been minimized.

Contributor

deanius commented Feb 23, 2015

Ok, @Slava I see your point. I'm looking at that package. And I don't want to clutter the Meteor api with too many ways to do async. Reactivity does present its challenges though. In the following, from ReactiveMethod:

Template.foo.helpers({
    methodResult: function () {
        var result1 = ReactiveMethod.call("myMethod", "a", "b");

        if (result1) {
            return ReactiveMethod.call("myMethod2", result1, "b");
        }
    }
});

The helper above will in fact be re-run as many times as is needed to 'resolve' all of the ReactiveMethod calls. So the guard clause if (result1) will be executed twice, and is absolutely necessary to keep from calling the 2nd ReactiveMethod call in each invocation of the helper.

I guess I'm still getting my head around reactivity and the idea that a single function() may need to be run a number of times to resolve a single chain of events.

I love Reactivity, but to set up a chain of things to occur, I think I prefer a cascade of functions to the rerunning of a single function.

@deanius

This comment has been minimized.

Contributor

deanius commented Feb 23, 2015

@Slava, I've illustrated some points in an issue on that repo.
In it, I explain why I do not think that the reactive style of chaining fits the bill, or is ready yet for Meteor core. As I point out in the referenced issue, the developer can too easily write a function that gets invalidated forever in a nasty loop, where as Promises are more explicit about sequentiality.

With doThis().then(doThat).then(doAnother) you don't have to know the innards of invalidations and rerunning to avoid problems, because no function is run more than once. I know it's a competing style, and maybe with a more solid reactive method call, I wouldn't have any hesitation, but for now I'm still a promise advocate.

@stubailo

This comment has been minimized.

Contributor

stubailo commented Feb 24, 2015

@chicagogrooves Thanks for the feedback! I'm going to work on this issue a little more in the coming days - I think Tracker is the best way to get eventual consistency in the face of changing data, and hopefully I can come up with a good package and some compelling examples.

One example that would be impossible to do with promises:

Tracker.autorun(function () {
  // Have the method check for a new return value every 30 seconds
  Session.set("x", ReactiveMethod.apply("myMethod",
    [Session.get("y")], { pollInterval: 30000 }));
}

Basically, since a promise can only resolve once, you're restricted to functions that can only ever return a single value. If you want to design an API that allows returning different values over time, Tracker or event emitters are the only option.

This also applies in the case where you want to mix in packages like mdg:geolocation and iron:location, which represent a constantly-changing variable. Using such a library with a method call that returns a promise could result in a complex chain of tracker computations and promise resolutions.

I understand that Tracker can sometimes be hard to reason about since you have to remember that functions could re-run many times, sometimes with "invalid" data for some of the variables. I think this can be improved by improving the ecosystem of Tracker-enabled functions so that people learn what to expect, improving tooling so that people can better understand when their functions are rerunning, and improving the documentation to communicate edge cases that people can get caught on (like you suggested for my package!)

@deanius

This comment has been minimized.

Contributor

deanius commented Feb 24, 2015

@stubailo Great point about 1-time promises vs. re-run reactivity. I think you're right that Promises are a bit of an impedance mismatch - they were just the tool I know best, but I'm convinced to see the Tracker thing out. I look forward to your patch, let me know if I can help / retest etc..

@deanius deanius closed this Feb 24, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment