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

#4437: Ensure method hooks always run with correct timing (before and after method) #4537

Merged
merged 6 commits into from Aug 14, 2018

Conversation

dancastellon
Copy link
Contributor

@dancastellon dancastellon commented Aug 13, 2018

Resolves #4437
Impact: minor
Type: bugfix

Issue

Due to current limitations with extending the Catalog schema, I created a MethodHooks.after callback to move a couple fields from Products to Catalog when a product is published. I registered the hook to the catalog/publish/products method. When it ran, I was seeing that my callback was firing before the completion of the method.

Solution

I identified that this was happening in Meteor methods that call GraphQL mutation functions, without awaiting for them to complete before returning a result. There were 2 methods that did this - catalog/publish/products and accounts/addressBookAdd.

Meteor methods allow you to return a promise and will await it, so we should make method-hooks work in that scenario.

Breaking changes

None

Testing

  1. Run Reaction in debug mode: METEOR_DISABLE_OPTIMISTIC_CACHING=1 REACTION_LOG_LEVEL="DEBUG" reaction
  2. Create a new tag from the header, and confirm log reads "Created tag..." - this confirms a previously broken core "after" method hook now works
  3. /imports/plugins/included/discount-codes/server/hooks/orders.js
    As first line in MethodHooks.before callback, add logging: console.log("before copyCartToOrder");
  4. /imports/plugins/included/notifications/server/hooks/notification.js
    As first line in MethodHooks.after callback, add logging: console.log("after copyCartToOrder");
  5. /imports/plugins/core/core/server/method-hooks.js
    After line 105 (methodResult = ...) add logging: console.log("method is done");
  6. Add a product to cart, complete the order, confirm server logs read:
    before copyCartToOrder
    cart/copyCartToOrder ...
    Transitioned cart ... to order ...
    sendOrderEmail status: new
    method is done
    after copyCartToOrder
  • This confirms 2 core MethodHooks still work as expected
  1. /imports/plugins/core/catalog/server/index.js
    Add the following code to the end of the file:
import MethodHooks from "/imports/plugins/core/core/server/method-hooks";
MethodHooks.before("catalog/publish/products", (options) => {
  console.log("Before catalog/publish/products hook");
});
MethodHooks.after("catalog/publish/products", (options) => {
  console.log("After catalog/publish/products hook");
  return options.result;
});
  1. Change a product's title (or any field), publish, confirm log reads:
    Before catalog/publish/products hook
    method is done
    After catalog/publish/products hook
  • This confirms the original reason for this PR - being able to register an after hook for catalog/publish/products so that the swag shop can push additional product fields to catalog after the method has completed.

@dancastellon
Copy link
Contributor Author

@zenweasel Can you take one last look here? Thanks!

Copy link
Collaborator

@zenweasel zenweasel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Tested and passed testing instructions.

@spencern spencern merged commit 9913020 into release-1.14.1 Aug 14, 2018
@spencern spencern deleted the fix-4437-dancastellon-async-method-hooks branch August 14, 2018 04:14
@spencern spencern mentioned this pull request Aug 14, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants