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

Can I do async work inside a hook? #229

Open
thearabbit opened this issue Aug 30, 2017 · 20 comments
Open

Can I do async work inside a hook? #229

thearabbit opened this issue Aug 30, 2017 · 20 comments
Labels

Comments

@thearabbit
Copy link

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

zimme commented Aug 30, 2017

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

zimme commented Aug 30, 2017

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

@zimme zimme closed this as completed Aug 30, 2017
@thearabbit
Copy link
Author

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 zimme reopened this Aug 31, 2017
@zimme
Copy link
Member

zimme commented Aug 31, 2017

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 zimme changed the title Wait async before hook? Can I do async work inside a hook? Aug 31, 2017
@thearabbit
Copy link
Author

Thanks, waiting for you...

@thearabbit
Copy link
Author

Now It is ready or not?

@sualex
Copy link

sualex commented Jan 19, 2018

@zimme
I have the same question :)

@zimme
Copy link
Member

zimme commented Jan 19, 2018

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

zimme commented Jan 19, 2018

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

sualex commented Jan 20, 2018

@zimme thanks for reply

@thearabbit
Copy link
Author

@zimme Thanks 👍

@luixal
Copy link

luixal commented Nov 27, 2019

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

luixal commented Nov 27, 2019

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

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

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

luixal commented Nov 27, 2019

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

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

copleykj commented Aug 2, 2022

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

Sharealikelicence commented Aug 2, 2022

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

copleykj commented Aug 2, 2022

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

8 participants