Async Methods with Callback

[denihs]

What we're doing here is testing the idea of having callbacks for the new async methods: callAsync, insertAsync, updateAsync, etc...

The reason for this is that right now we rely on flags to decide when to wait or not for the server result.

To give an example, if you were to insert something in the DB today you can do something like this:

// calling from the client

const stubId = Collection.insert(doc, function (err, result) {
   // do something with the server result
})

What's happening in the example above is that you can get an id of something you inserted in the Minimongo, but you can also provide a callback to the insert function to make sure everything worked out in the server.

Now, with the async API, we're working with promises. So if you just do something like:

// calling from the client

const stubId = await Collection.insertAsync(doc);

You will get the result generated in the server. But maybe you don't want to wait that much. Maybe you just want the result from the client.

In that case you can provide a flag:

// calling from the client

const stubId = await Collection.insertAsync(doc, { returnServerPromise: false });

With callback, it would look something like this:

// calling from the client

const stubId = await Collection.insertAsync(doc, function (err, result) {
   // do something with the server result
})

Which looks more like what we have today.

Important to notice

I gave examples here with Collection.insertAsync, but this has nothing to do with Mongo or Minimongo.

All this is about client and server on Meteor.

The examples above are the same if you change Collection.insert by Meteor.call and Collection.insertAsync by Meteor.callAsync.

Sometimes you need to wait for the server, but if you're working with real-time and need to rely on the optimistic UI, you don't want to wait for the whole trip to the server.

With callbacks, it's easier to understand and separate what's client and server.

Also, keeping the callbacks would make it easier to migrate from the older version.

---

To run the test on this branch:

// run inside the root of Meteor's repository
./meteor test-packages

[rodrigok]

Have the callbacks back seems the best solution IMO. So we keep the same behavior as today.

[rj-david]

This will save us from a lot of work. This is one of the major blockers that we are facing with migrating to 3.0

[aogaili]

Callbacks surely looks cleaner and more aligned with the current approach.

[paulincai]

I think for DX we should stick to the general standards in NODE with either await or callbacks. Coming from callbacks in Meteor feels natural to consider callbacks the "right" approach. I'd personally prefer to know that Meteor remains within the current and future standards of NodeJS and Javascript.

[jamauro]

Agree with @paulincai. Feels kinda weird to mix await and callbacks.

What were the downsides to the approach of passing in a flag? Were there other approaches considered?

It would also be nice to see an example where both client side errors and server side errors are caught.

[rodrigok]

@jamauro I may not be the best person to answer this, but IMO the main downside of passing the flag is that you loose the option to await the client side return and listen do the backend return at the same time, so you be locked to one or another.

[Twisterking]

I also have to agree with @paulincai and @jamauro - await is the present (and future) of JS, callbacks just don't really mix well with it I think.

I personally would go for different methods.

Something like (on the CLIENT):

// default: call async function and await until there is a response from the server:

const actualId = await Collection.insertAsync(doc);

// optional: call async function on the client, but NOT await the actual response from the server, but work with "optimistic response":

const stubId = await Collection.insertAsyncOptimistic(doc);

I know, not perfect, but in my (honest, but subjective) opinion the best solution. This methodOptimistic variant we won't need for too many methods I think - we are only talking about these methods in my opinion:

• Meteor.callAsync
• Meteor.applyAsync
• all the "(Mini)Mongo methods"

[denihs]

I think @rodrigok pretty much summarized here.

@Twisterking idea isn't bad, we actually thought about having more methods. But the problem with that is that you have more methods hehe.

And having more methods means more documentation and more things for newcomers to understand.

Besides, another advantage of keeping callbacks is the migration from the old version.

For a code like this:

const stubId = Collection.insert(doc, function (err, result) {
   // do something with the server result
})

All you have to do is this:

const stubId = await Collection.insertAsync(doc, function (err, result) {
   // do something with the server result
})

Without callbacks, you need to use then/catch or try/catch. And in some flows, you'll have to do more things to mimic this behavior.

I know it's not ideal. It looks like we're running from the way promises should be. But we need to remember that in Meteor, these callbacks are side effects. You use them to make sure everything worked out in the server, and get some data from there.

It's different from await Collection.insertAsync(doc). In this case, we're "pausing" our app to wait for the server to finish. And if you use Meteor for the real time, that's not ideal.

In a lot of cases, we don't want to wait for the server, but if something wrong happens there we want to revert the changes in the client. That's where, as I see, callbacks shine.

[Twisterking]

@denihs I am aware of the drawbacks. At the same time my thinking is the following: Meteor 3.0 IS, by definition, a very major release and will have quite a number of breaking changes. ... one of the reasons why this is version 3! ;)

--> in my book, at some point, you need to be "brave enough" to make the proper change to Promises. so yes, you are right: You then need to work with try / catch and all that. But if you don't make this change NOW with this release, when will there ever be "a better time" to do it?

I am okay with your solution - to be perfectly honest, we at Orderlion will never use the callbacks and await the server result anyways, so it won't make any difference to us.

But I stand by my point that this solution seems a bit "half assed" to me and that you need to stand behind your (very good!) decision of now using Promises / async / await everywhere.
But, to be fair: your reasoning of "callbacks are just side effects in the meteor world" is a very valid one! :)

[durac]

I may not be in position to answer this, but I also think "the migration from the old version is easier" is a kinda weak argument, in the end it is about bringing Meteor forward and having the best DX.

But the option to get the client AND server response is definitely an advantage and nice to have. Maybe an alternative solution could be to still give priority to the server response (which is, if you do not heavily rely on the realtime aspect of meteor probably more often used than the client return) by returning it as a promise, but still also have callbacks for the server/client returns.

const stubId = await Collection.insertAsync(doc, {
  onServerData: (result) => console.log('server result: ', result),
  onClientData: (result) => console.log('client result: ', result),
  onError: (error) => console.error('error: ', error)
});

Another consideration is that there are a lot of popular and awesome async state management solutions out there such as react-query and SWR which could be used a lot easier with Meteor if it would properly support await when returning the server response. And in my opinion the reality of WebDev is that for most usecases realtime is just not needed.

[rj-david]

main downside of passing the flag is that you loose the option to await the client side return and listen do the backend return at the same time, so you be locked to one or another.

This is definitely the main issue. Breaking this will break all apps using optimistic UI.

To those who want to use a Promise, or a Promise with the flag on what to return, Meteor can easily provide a very simple wrapper that converts the callback version to a promise. Everybody wins.

Yes, that results to more methods / documentation. But those can be grouped and informing the newcomer that the default is the Promise version. But if you want to use optimistic ui, you can use the callback version

[jamauro]

I use Optimistic UI heavily and think it's a great feature of Meteor. Just wondering if mixing async/ await with callbacks is the best approach. I haven't really seen that approach elsewhere.

For my own edification, what would be the downsides to an approach like this:

const {stubPromise, serverPromise} = insertAsync(doc)

const stubId = await stubPromise
// take action with the stubId

const result = await serverPromise.catch(error => { 
// rollback changes as needed
})

[rj-david]

That looks like a very clean approach, @jamauro.

Maybe we can further simplify it into something like:

const { stubId, serverPromise } = await insertAsync(doc);

const result = await serverPromise.catch(error => { 
// rollback changes as needed
});

[zodern]

For callAsync, I prefer the previous behavior of returning a promise for the server result. It is relatively rare for code to want the returned value from a stub - usually it only needs the server result and the stub is run for the side effects it does. Meteor.call was designed for this use case - it was a simplified api that just provided the server result. When the stub result is also needed, then Meteor.apply could be used instead.

For zodern:relay, I had a similar plan as what @jamauro suggested. It seems it might be a little more complicated than using callbacks, but it does allow doing everything with promises.

import { createProject } from '/methods/projects';

// continues returning a promise for the server result
let serverResult = await createProject();

// The returned promise has two additional properties with promises
// that can be used instead
let { stubPromise, serverPromise } = createProject();
let stubResult = await stubPromise;
let serverResult = await serverPromise;

There are also some discussions from 2015 around a promise api that might provide some inspiration: #5005 and #4939.

[make-github-pseudonymous-again]

Is the idea to get rid of automated optimistic UI? What did not work with the implicit "Simulate on the client, reconcile on server response" approach? Why does it need to be explicit now?

[radekmie]

Sorry for being late, but let me throw my two cents. I completely understand where the idea of using a callback comes from, but my concern about that is it doesn't have the "meaning" bound to it. I mean, if I saw a codebase that uses a callback, I'd most likely ignore the returned value of this call.

That's why I'm far more in the direction of what @zodern proposed (also elsewhere). I don't see the need for serverPromise there, though: stub should be enough, I think. Here's an example:

const promise = Meteor.call(...); 
const serverResult = await promise;
const stubResult = await promise.stub;

The benefit is that it's backward compatible, i.e., await Meteor.call(...) just works. It's all about returning a Promise object with additional stub property (which is a Promise as well).

[radekmie]

@make-github-pseudonymous-again

Is the idea to get rid of automated optimistic UI? What did not work with the implicit "Simulate on the client, reconcile on server response" approach? Why does it need to be explicit now?

No, Optimistic UI is not going anywhere. Meteor (almost) always gave you the option to react to the "method stubs", i.e., know what was the result of the local simulation before the server responded. This discussion is only about the API for doing that.

[denihs]

@radekmie the problem I see with your approach is that apps that use Minimongo will always have to worry about waiting for the right promise. For example:

// This:

const stubId = Collection.insert(doc, function (err, result) {
   // do something with the server result
})

// Will have become this:

const promise = Collection.insertAsync(doc)

promise.catch(function (err) {
   // do something if server fails
})

const stubId = await promise.stub;

So basically every call for a Minimongo (Local) method you'll have to add this .stub.

This is because Meteor.call and Collection.insert use the same flow to perform their actions.

So, as I see, if we were to follow this approach, we'll have to invert promises for the Minimongo (Local) methods.

// For callAsync
const promise = Meteor.callAsync(...); 
const serverResult = await promise;
const stubResult = await promise.stub;

// For Local Mongo methods:

const promise = Collection.insertAsync(doc)

promise.server.catch(function (err) {
   // do something with the server result
})

const stubId = await promise;

At least this is what makes sense to me because in most cases you don't even provide a callback to insert, update, etc, to check if there is a problem in the server. So in this case you just care for the stub.

Based on that, @jamauro's approach here looks cleaner to me. I would just change stubPromise for the result itself:

const { result, serverPromise } = await Coll.insertAsync(doc)

// and

const { result, serverPromise } = await Meteor.callAsync(...)

[radekmie]

@radekmie the problem I see with your approach is that apps that use Minimongo will always have to worry about waiting for the right promise. For example: [...] At least this is what makes sense to me because in most cases you don't even provide a callback to insert, update, etc, to check if there is a problem in the server. So in this case you just care for the stub.

I honestly never cared about the stubs, mostly because they "lie". Like, if I'd like to insert a document into a collection and then do something using the newly added ID (which I can imagine is a very common pattern), I have to wait for the server anyway. The same goes for the number of updated documents returned from update.

I'm really interested in what others will say here, because maybe the tens of Meteor apps I've worked on and audited were special in this area 😛

---

EDIT: I know that currently, Minimongo returns stub results on the client. And I also know that it caused a lot of problems in the projects I worked on.

[filipenevola]

I think we have different discussions in the same mix and it is going to be hard to reach one conclusion:

• A) How methods should work in terms of awaiting results from the server;
• B) How MongoDB/Minimongo methods should work;
• C) How can we minimize the migration effort;

Breaking down into different things we can reach different conclusions for each case and maybe find a good trade-off.

What IMO would be the best trade-off:

• Return two promises (doesn't matter exactly how) for Methods (solution for A);
• Split each MongoDB/Minimongo operation in two methods, one with the simple way (without caring about stub) and another more complete that exposes both promises (solution for B);
• Create a wrapper to use the complete option of MongoDB and Methods with both promises to simplify migrations (solution for C);

Hmmmm, and about beginners/newcomers? Maybe we could also have two options for Methods, one very simple (ignoring stub) and another with these two promises.

I don't know if my answer and explanation were too vague so let me know if my suggestion is not clear enough and I can expand on that but I think these three proposals (four with the beginner one) would address all the concerns in this PR.

The bad part of this trade-off: more code to be maintained. 🤷

[ToyboxZach]

IMO the priority with an API should be to be as simple as possible while making sure its clear what is happening with everything. I would be fine with this solution, but the problem I have is as a developer its not immediately clear whats going to happen with a callback and a promise.

For instance if you do:

collection.insertAsync(data, (err, result)=>{})

Is that going to be the server response the stub? What if you don't have a stub defined and you have a callback? Its leaving a lot open for interpretation, which means more documentation, and confusion.

I like @radekmie's solution with the separate promise to listen to while not complicating the base API if you are doing something simple.

To go forward with this proposed API I would suggest having to explicitly name the callback, so you are completely clear what you are getting:

const serverResult = await collection.insertAsync(data, {stubResult: (err, clientResult)=>{}, serverResult:(err, serverResult)=>{})

I think stubs are things you are specially handling, and needing to be explicit is worth the added functionality.

[jamauro]

I like the approach @zodern outlined above. I put together a quick REPL to play around with it. Check it out

Here's what I like about it:

1. It has the option of waiting for the server result with the cleanest syntax. This can be documented as the default.

const result = await Meteor.callAsync(...);

2. It has another option for working with the stub but it isn't required. This can be documented as an advanced approach with the appropriate caveats around the stub value.

const { stubPromise, serverPromise } = Meteor.callAsync(...);
const stubResult = await stubPromise;
const serverResult = await serverPromise;

3. With a stub promise, it could allow for working with stub exceptions which is nice and one of the reasons I like Validated Method - it allows catching stub exceptions and preventing the server method from running.

Regarding:

I honestly never cared about the stubs, mostly because they "lie". Like, if I'd like to insert a document into a collection and then do something using the newly added ID (which I can imagine is a very common pattern), I have to wait for the server anyway.

This is probably a separate discussion but this feels like an opportunity to change Meteor.callAsync to return the stub value so that at least for inserts the stub ID can be used with confidence. Otherwise I think the stubResult would be undefined for Meteor.callAsync so we'd need to point people to use Meteor.applyAsync with returnStubValue: true.

Curious to hear what y'all think.

[denihs]

@jamauro looking at the REPL you created I think it looks really good!

Also addresses most of what @filipenevola mentioned here.

I'm curious to hear what the others think too.

[radekmie]

With a stub promise, it could allow for working with stub exceptions which is nice and one of the reasons I like Validated Method - it allows catching stub exceptions and preventing the server method from running.

Not all stub exceptions should prevent server from running, I think. The easiest example is using .remove() with a non-ID selector, which is not allowed in untrusted code (e.g., client). But that's already built into Meteor's "way of simulations" and I'm unsure why this is related to the way we call them (not execute).

I honestly never cared about the stubs, mostly because they "lie". Like, if I'd like to insert a document into a collection and then do something using the newly added ID (which I can imagine is a very common pattern), I have to wait for the server anyway.

This is probably a separate discussion but this feels like an opportunity to change Meteor.callAsync to return the stub value so that at least for inserts the stub ID can be used with confidence. Otherwise I think the stubResult would be undefined for Meteor.callAsync so we'd need to point people to use Meteor.applyAsync with returnStubValue: true.

(Emphasis mine.) I think we're mixing two things here. Sure, .insert() calls a method underneath (it has to communicate with the server), but I don't think that no code should be treated differently when it comes to methods. In my example I used the ID of inserted document, which can be easily generated by the stub (just Random.id() is fine, as long as it doesn't clash with an existing one) but cannot be safely used afterward (e.g., Meteor.call('x', await Collection.insertAsync({}))).

My point is basically what @ToyboxZach wrote:

I think stubs are things you are specially handling, and needing to be explicit is worth the added functionality.

[jamauro]

Let me attempt to clarify a couple things:

1. I think this approach is a really nice way to handle async methods and Meteor.callAsync. I believe it allows the engineer to craft the UX they want and addresses the concerns raised in this discussion. Hat tip to zodern. Would be great to hear other's thoughts on it.

2. Regarding additional changes to Meteor.callAsync's behavior – namely, support for returning the stub value and handling stub exceptions – I suggest we spin up a different thread if there is enough interest since it's tangential to this discussion. I'll attempt to quickly distill my thoughts:

Let's assume everyone likes the approach and it's implemented. Then my thoughts were, what else can be simplified / improved? For example, it might be nice if Meteor.callAsync was changed to act more like Validated Method in the sense that 1) the stub value is returned with consistent ID generation and 2) stub exceptions are thrown by default. Your point that not all stub exceptions should prevent server from running is a good one, but perhaps it should be the default and people can opt out. At a minimum, it might be a good idea to return the stub value for Meteor.callAsync so that if it's destructured:

const { stubPromise, serverPromise } = Meteor.callAsync(...);
const stubResult = await stubPromise;
const serverResult = await serverPromise;

the stubResult isn't undefined, and the engineer doesn't need to wade through the docs to discover they need to use Meteor.applyAsync with the option {returnStubValue: true}.

[make-github-pseudonymous-again]

No, Optimistic UI is not going anywhere. Meteor (almost) always gave you the option to react to the "method stubs", i.e., know what was the result of the local simulation before the server responded. This discussion is only about the API for doing that.

@radekmie My question is why is a change in the API required at all? All we are doing is going from sync to async, why is there a need to change how to specify which of the real or stub value is of interest? Is it because they run concurrently and each of them could finish first and we do not want to wait for the other? Or is it a new feature to be able to act on both values explicitly, something that was not possible before?

I would personally prefer const {stub, real} = Collecion.insertPromises(...) (note the absence of await and the name of the method), or the flag solution, or both (implement the second on top of the first).

[Strdate]

1. Rookie question - why do we need to await for the stub at all? As far as I know all minimongo operations are sync by default. I guess there is a problem with the optimistic execution because the method that must run on the client is async?
2. As mentioned above, I would prefer the API to be as simple as possible, thus const serverResult = await Meteor.callAsync(...) is the best choice.
3. What is the issue with awaiting on the serverResult, when we have optimistic execution on the client?
4. In case of destructuring I prefer the React-like syntax const [ stubPromise, serverPromise ] = Meteor.callAsync(...);, which allows you to name your variables to your needs

[radekmie]

@make-github-pseudonymous-again Sorry for answering so late, I missed your message 😐

@radekmie My question is why is a change in the API required at all? All we are doing is going from sync to async, why is there a need to change how to specify which of the real or stub value is of interest?

I think it's more about how both of them are accessible, not which one is where. And the fact that the result should be awaitable now allows us to improve the ergonomics of this API.

I would personally prefer const {stub, real} = Collecion.insertPromises(...) (note the absence of await and the name of the method), or the flag solution, or both (implement the second on top of the first).

This is not possible without Fibers, at least as long as real: T, not real: Promise<T>.

---

@Strdate

Rookie question - why do we need to await for the stub at all? As far as I know all minimongo operations are sync by default.

In short: it's all about the complexity. To make Minimongo "async-native", we have to implement async methods. To do that, we can either wrap the sync ones in async functions or make them truly async. The latter is easier, as it plays well with all of the "asyncifycation" of Meteor happening elsewhere, as some internals may become async now. If we'd like to keep them async, then it may require more work or doubled code.

[make-github-pseudonymous-again]

To make Minimongo "async-native", we have to implement async methods

Note that you can await non-promises, so you can await the existing API out-of-the-box. Having them typed as async or return a dummy Promise is only required for type-aware tools (e.g. TypeScript).

[make-github-pseudonymous-again]

This is not possible without Fibers, at least as long as real: T, not real: Promise<T>.

I meant both stub and real to be promises.

[make-github-pseudonymous-again]

I think it's more about how both of them are accessible, not which one is where. And the fact that the result should be awaitable now allows us to improve the ergonomics of this API.

So my question is: What is the current accessibility?

[Twisterking]

What is the current state of this discussion here?
It seems to me that this is one of the most important decisions which have to be moved out of the way in order for Meteor v3 to properly progress?!

My opinion still stands, I would opt for something like this as suggested by @zodern and @radekmie :

const promise = Meteor.call(...); 
const serverResult = await promise;
const stubResult = await promise.stub;

All other solutions seem "dirty" to me and not really following the standards out there.

In 99% of cases you want to await the server response, so this use case is exactly the one which it all should be optimized for. --> const result = await Meteor.call(...). perfect.

if you really want to opt for the other approach, I would personally at least go for "array like destructing" like e.g. React's useState() hook:

const [ result, serverPromise ] = await Coll.insertAsync(doc)

// and

const [ result, serverPromise ] = await Meteor.callAsync(...)

[DanielDornhardt]

Hi, i've ran into this too right now...

With another simple thing:

Calling a method with a stub using callAsync:

const res1 = await Meteor.callAsync('asyncMethod1')

then the await yields... if another piece of the code then runs and tries to set a timer somewhere:

Meteor.setTimeout(() => {
    sayHi();
}, 1000);

You get an error & _an exception which breaks our code saying:

"Can't set timers inside simulations" (thrown here:

meteor/packages/meteor/timers.js

Line 38 in de88454

Meteor.setTimeout = function (f, duration) {

)

Which is not good, but it's manageable if you just use window.setTimeout if you know what you're doing because you're not inside a stub / method, right?

Ah, one funky thing is: We could maybe actually allow timers now using promises and/or async/await in method stubs for async methods, right? 😄

But another, more troublesome thing is:

The timer thing looks like it could be mitigated easily, but that also means that

If an async stub gets awaited, and we're not entirely synchronising all code across our entire codebase in the client, then another method could indeed get called in another part of the frontend without us having much control besides manually synchronizing using reactive vars or some other mechanism? 😯

Because... BLAZEJS in the template lifecycle events isn't async-aware - and even if you make your onCreated's etc async, it won't await them.

So you can't rely on anything being synchronized outside your templates lifecycle methods' boundary.

Which is bad in my view. Very bad.

So this PR: Add firstRunPromise to Tracker.computation objects might be back in the race... to be able to await the first run of a computation at least...

This might be not so bad for react or other frameworks, but for Blaze this is bad.

The Stub part is a cool feature for instant reactions in the client and a part of Meteors' original story. We rely on it for timely updates in our UI. Nothing that couldn't be undone / skipped over / mitigated, but it just makes life less nice. I think it'd be very good to find a solution besides "ah make sure yourself that no two .callAsyncs can will ever be called at the same time", because, especially for blazeJS, the affordances just aren't there yet.

One thing I don't get or what might be an idea (?): Why does the stub-or-not - check have to be done depending on some environment setting on another?

For regular client -> server calls it's pretty obvious that if it's in the client, it's the stub, and if it's running on the server it's the real deal, no? :D

Maybe we could choose a "default interface" for this regular use case, and if somebody really needs stubs to run for his server to server calls, the he could use an advanced interface somehow?

[denihs]

We're solving this issue on this PR.