Meteor-Community-Packages / meteor-collection-hooks

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

Async hooks run in parallel to the collection operation #301

Open pmogollons opened 9 months ago

pmogollons commented 9 months ago

Describe the bug Async hooks dont wait for the hook to return to perform the Collection op, for example.

To Reproduce

Tickets.before.insert(async function ticketsBeforeInsert(userId, doc) {
  doc.idNumber = await incrementCounter(`PQR::${doc.orgId}`);
});

console.log(doc.idNumber) // This will be undefined

Expected behavior Async hooks should await for the response before calling the collection op.

Additional context Happens in all of the package versions.

Workaround In a Meteor version before 3.0 you can just use Promise.await

Tickets.before.insert(function ticketsBeforeInsert(userId, doc) {
  doc.idNumber = Promise.await(incrementCounter(`PQR::${doc.orgId}`));
});

Possible way to fix it When we call the aspect function callbacks (hooks) we should await them, this will require to use async on CollectionHooks.defineAdvice and to use await Promise.all on each aspect.

For example, this are the changes done to the insert advice:

      await Promise.all(aspects.before.map(async (o) => {
        const r = await o.aspect.call({ transform: getTransform(doc), ...ctx }, userId, doc)
        if (r === false) abort = true
      }))

The issue with this is that this makes the collection methods to be async too and test will fail.

Any ideas on how to approac this issue will be appreciated.

Related issues

71

github-actions[bot] commented 9 months ago

Thank you for submitting this issue!

We, the Members of Meteor Community Packages take every issue seriously. Our goal is to provide long-term lifecycles for packages and keep up with the newest changes in Meteor and the overall NodeJs/JavaScript ecosystem.

However, we contribute to these packages mostly in our free time. Therefore, we can't guarantee you issues to be solved within certain time.

If you think this issue is trivial to solve, don't hesitate to submit a pull request, too! We will accompany you in the process with reviews and hints on how to get development set up.

Please also consider sponsoring the maintainers of the package. If you don't know who is currently maintaining this package, just leave a comment and we'll let you know.

pmogollons commented 9 months ago

@StorytellerCZ any ideas where to look? I fiddled around with it yesterday with no luck. I think one of the possibilities to fix this is to only support this use case in Async methods, so I added a new insertAsync advice, but it never gets called.

vincentcarpentier commented 6 months ago

Any news about this issue ?