meteor-collection-hooks icon indicating copy to clipboard operation
meteor-collection-hooks copied to clipboard

Can I do async work inside a hook?

Open thearabbit opened this issue 7 years ago • 20 comments

I would like to create auto-increment in _id

async function getNextSequence(name) {
  const ret = await Counters.rawCollection().findOneAndUpdate(
    { _id: name },
    { $inc: { seq: 1 } },
    { upsert: true, returnOriginal: false }
  );
  return ret.value.seq;
}
//...
Units.before.insert(function (userId, doc) {
  const nextSeq =  getNextSequence('unitId');
  console.log(nextSeq);
  doc._id = nextSeq;
    ........
});

But don't work (before hook don't wait for the nextSeq return. Please help me

thearabbit avatar Aug 30 '17 14:08 thearabbit

Units.before.insert(async function (userId, doc) {
  const nextSeq = await getNextSequence('unitId');
  console.log(nextSeq);
  doc._id = nextSeq;
    ........
});

Should probably work. But using an async function here will result in you not being able to abort the insert from the hook by returning false from the hook function. What you could do in to support that is using the "old" promise .then/.catch semantic.

Units.before.insert(function (userId, doc) {
  getNextSequence('unitId').then(() => {
    console.log(nextSeq);
    doc._id = nextSeq;
    ........
  }).catch((error) => {...});
});

zimme avatar Aug 30 '17 15:08 zimme

Closing this, @ping me if you need this re-opened.

zimme avatar Aug 30 '17 15:08 zimme

Thanks for your reply, but now still don't work (_id don't wait for nextSeq)

Units.before.insert(function (userId, doc) {
    getNextSequence('unitId').then((result) => {
        doc._id = result;
        console.log('before', doc);
    }).catch((error) => {
        console.log(error);
    });
});

Units.after.insert(function (userId, doc) {
    console.log('after', doc);
});
--------
// getNextSeq

Get

I20170831-08:19:30.500(7)? after { name: 'Hi', category: 'Count', _id: 'cfnMKjYELnCwjSP7o' }
I20170831-08:19:30.504(7)? before { name: 'Hi', category: 'Count', _id: 9 }

thearabbit avatar Aug 31 '17 01:08 thearabbit

Yes, I see now... The problem is that this package isn't promise/async aware.

i.e. the .insert method is actually called right after the hook has run and then in the next tick the callback for getNextSequence is run and doc._id is updated after the fact.

We would have to add support for returning a promise and waiting for that in the package code to allow for this. I'll look into adding this feature whenever I have time. I would accept a PR for this.

zimme avatar Aug 31 '17 07:08 zimme

Thanks, waiting for you...

thearabbit avatar Aug 31 '17 08:08 thearabbit

Now It is ready or not?

thearabbit avatar Jan 19 '18 15:01 thearabbit

@zimme I have the same question :)

sualex avatar Jan 19 '18 19:01 sualex

I haven't had time to look at this, but as I wrote in my last reply. I would accept a PR for this.

https://github.com/matb33/meteor-collection-hooks/blob/master/find.js#L17 https://github.com/matb33/meteor-collection-hooks/blob/master/findone.js#L17 https://github.com/matb33/meteor-collection-hooks/blob/master/insert.js#L17 https://github.com/matb33/meteor-collection-hooks/blob/master/remove.js#L31 https://github.com/matb33/meteor-collection-hooks/blob/master/update.js#L50 https://github.com/matb33/meteor-collection-hooks/blob/master/upsert.js#L44

In all of these places you would have to check if any of the return values for any of the hooks (aspects) are promises and if so, wait for all of them to resolve and if any of them return false set abort = true and then after waiting for all of the "potential" promises to have resolved check abort and if it's true then return without calling the actual insert/update/find/etc...

zimme avatar Jan 19 '18 19:01 zimme

I haven't had a need for this and that's the reason I haven't gotten to implementing this... I only do work on features for open-source work when I need it myself or when a majority of the users of the package want/need that functionality.

zimme avatar Jan 19 '18 20:01 zimme

@zimme thanks for reply

sualex avatar Jan 20 '18 09:01 sualex

@zimme Thanks 👍

thearabbit avatar Jan 20 '18 14:01 thearabbit

Hi,

I'm having a different issue that my be related. I'm trying to fire an asynchronous process from an .after.update hook.

Example: When a new Employee is inserted, I want to fetch some data from and external source (using and HTTP query).

I have a function that returns a promise:

getExternalData = function(id) {
  return new Promise(
    function(resolve, reject) {
      Meteor.http.get(
        'https://myurl/' + id,
        (err, res) => {
          if (err) reject(err);
          if (res) {
            // i have more logic here, which should be irrelevant
            resolve(res);
            console.log('promise finished');
          }
        }
      );
    }
  )
}

then, in my hook, I perform some operations and want to fire that asynchronous function at the end,so my hook looks like this:

Employees.after.update(function (userId, doc, fieldNames, modifier, options) {
  if (Meteor.isServer) {
    // some other code here performing different operations
    getExternalData(doc._id).then( (res) => { console.log('promise finished in hook'); });
    console.log('returning');
  }
});

Problem is, hook doesn't return until promise has finished execution. So I get this kind of log:

// res data here
promise finished
promise finished in hook
returning

while I expected something like:

returning
// res data here
promise finished
promise finished in hook

Any idea why is this working this way?

luixal avatar Nov 27 '19 11:11 luixal

I've found a solution, calling the function inside a Meteor.defer, like this:

Employees.after.update(function (userId, doc, fieldNames, modifier, options) {
  if (Meteor.isServer) {
    // some other code here performing different operations
    Meteor.defer(
      () => { getExternalData(doc._id).then( (res) => { console.log('promise finished in hook'); }) }
    );
    console.log('returning');
  }
});

But it seems weird it doesn't do the same when using a plain promise... could this be related to Fibers?

luixal avatar Nov 27 '19 12:11 luixal

So ... why not use Promises then? Meteor added a new function to the Promise constructor, called await, which will literally await the finish of this promise until it continues with this code. It's like writing async code in a synchronous way:

Promise.resolve(() => { ... }).await()

https://github.com/meteor/promise/blob/master/promise_server.js#L59

SimonSimCity avatar Nov 27 '19 15:11 SimonSimCity

@luixal this issue thread isn't really the place to get into a deep discussion on this. If you are on the Meteor Community Group slack workspace and want to start a discussion, I'm more than happy to provide some insight.

copleykj avatar Nov 27 '19 19:11 copleykj

@SimonSimCity I was just trying the opposite, for the method to finish while the promise is running.

@copleykj never joined the group as we're stuck with and old version of Meteor. I was thinking this issue was hook's related only.

luixal avatar Nov 27 '19 21:11 luixal

I am willing to work on adding async support to this but have noticed that the project is currently on ES6. Would there be any issues in updating to a newer version that supports the async/await keywords?

Sharealikelicence avatar Aug 01 '22 02:08 Sharealikelicence

@Sharealikelicence can you elaborate on why you think this package is locked into es6 features? Meteor supports async/await and AFAIK that should mean this package does as well as long as we specify a meteor version >= the version where Meteor started supporting it.

copleykj avatar Aug 02 '22 00:08 copleykj

Yeah, it's not locked in, es6-promise is just a prereq for spacejam and I didn't want to start breaking things. Also, wasn't wanting to break compatibility with ie if that is a requirement for the project?

Sharealikelicence avatar Aug 02 '22 04:08 Sharealikelicence

Oh yes.. so we would need to upgrade tests as well.

I don't see any reason why you couldn't proceed if you see a viable path forward.

copleykj avatar Aug 02 '22 04:08 copleykj