Meteor-Community-Packages / meteor-collection-hooks

Meteor Collection Hooks
https://atmospherejs.com/matb33/collection-hooks
MIT License
658 stars 90 forks source link

Can I do async work inside a hook? #229

Open thearabbit opened 7 years ago

thearabbit commented 7 years ago

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

zimme commented 7 years ago
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 commented 7 years ago

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

thearabbit commented 7 years ago

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 }
zimme commented 7 years ago

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.

thearabbit commented 7 years ago

Thanks, waiting for you...

thearabbit commented 6 years ago

Now It is ready or not?

sualex commented 6 years ago

@zimme I have the same question :)

zimme commented 6 years ago

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 commented 6 years ago

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.

sualex commented 6 years ago

@zimme thanks for reply

thearabbit commented 6 years ago

@zimme Thanks 👍

luixal commented 4 years ago

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 commented 4 years ago

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?

SimonSimCity commented 4 years ago

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

copleykj commented 4 years ago

@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.

luixal commented 4 years ago

@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.

Sharealikelicence commented 2 years ago

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?

copleykj commented 2 years ago

@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.

Sharealikelicence commented 2 years ago

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?

copleykj commented 2 years ago

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.