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

Replace all Hooks with appEvents #4915

Merged
merged 46 commits into from
Jan 22, 2019
Merged

Replace all Hooks with appEvents #4915

merged 46 commits into from
Jan 22, 2019

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented Jan 11, 2019

Impact: breaking
Type: feature

Changes

  • Replaces all Hooks usage with the newer appEvents. This is a step on the path to heavier reliance on events as the primary communication mechanism among Reaction plugins and services.
  • Also removed the MethodHooks feature.
  • Change some appEvents events to emit with a different argument signature. All events now emit a single object. This makes it easier to add and remove properties without additional breaking changes in the future.
Before (Hooks.Events) After (appEvents)
Event Name: afterAccountsInsert
Arguments: userId, accountId
Event Name: afterAccountCreate
Arguments: { account, createdBy, isFirstOwnerAccount }
Event Name: afterAccountsUpdate
Arguments: userId, { accountId, updatedFields }
Event Name: afterAccountUpdate
Arguments: { account, updatedBy, updatedFields }
Event Name: afterAccountsRemove
Arguments: userId, accountId
Event Name: afterAccountDelete
Arguments: { account, deletedBy }
N/A Event Name: afterAddUnverifiedEmailToUser
Arguments: { email, userId }
afterRemoveCatalogProduct was sometimes emitted for variants Event Name: afterVariantSoftDelete
Arguments: { variant, deletedBy }
Event Name: afterRemoveProduct
Arguments: product
Event Name: afterProductSoftDelete
Arguments: { product, deletedBy }
N/A Event Name: afterVariantUpdate
Arguments: { _id, field, value }
Event Name: afterPublishProductToCatalog
Arguments: product, catalogProduct
Event Name: afterPublishProductToCatalog
Arguments: { catalogProduct, product }
N/A Event Name: afterOrderUpdate
Arguments: { order, updatedBy }
Event Name: onJobServerStart
Arguments: NONE
Event Name: jobServerStart
Arguments: NONE
Event Name: afterCoreInit
Arguments: NONE
Event Name: afterCoreInit
Arguments: NONE
Deprecated. Do not use in new code. Put your code directly in a non-Meteor plugin startup function instead.
N/A Event Name: sendEmail
Arguments: { job, sendEmailCompleted, sendEmailFailed }
N/A Event Name: afterShopCreate
Arguments: { createdBy, shop }
N/A Event Name: afterMediaInsert
Arguments: { createdBy, mediaRecord }
N/A Event Name: afterMediaUpdate
Arguments: { createdBy, mediaRecord }
N/A Event Name: afterMediaRemove
Arguments: { createdBy, mediaRecord }
N/A Event Name: afterOrderApprovePayment
Arguments: { approvedBy, order }
N/A Event Name: afterOrderCancel
Arguments: { cancelledBy, order, returnToStock }
N/A Event Name: afterOrderPaymentCapture
Arguments: { capturedBy, order, payment }
N/A Event Name: afterOrderCreate
Arguments: { createdBy, order }
N/A Event Name: afterNotificationCreate
Arguments: { createdBy, notification }
Event Name: afterCreateDefaultAdminUser
Arguments: user
No longer emitted. Instead "afterAccountCreate" is emitted and will have isFirstOwnerAccount set to true
Event Name: beforeUpdateOrderWorkflow
Arguments: order, options
No longer emitted.
Event Name: afterUpdateOrderUpdateSearchRecord
Arguments: order
No longer emitted. Use afterOrderUpdate.
Event Name: afterInsertProduct
Arguments: product
No longer emitted. Was not used, but can be added back in the future if necessary.
Event Name: beforeUpdateCatalogProduct
Arguments: product, { userId, modifier }
No longer emitted. Was not used, but can be added back in the future if necessary.
Event Name: afterUpdateCatalogProduct
Arguments: productId, { modifier }
No longer emitted. Was not used, but can be added back in the future if necessary.
Event Name: beforeRemoveCatalogProduct
Arguments: product, { userId }
No longer emitted. Was not used, but can be added back in the future if necessary.
Event Name: afterRemoveCatalogProduct
Arguments: userId, productId
No longer emitted. Was not used, but can be added back in the future if necessary.
Event Name: beforeCoreInit
Arguments: NONE
No longer emitted. Was not used, but can be added back in the future if necessary.
Event Name: onCoreInit
Arguments: NONE
No longer emitted. Was not used, but can be added back in the future if necessary.
Event Name: onImport${collectionName}
Arguments: object
No longer emitted. Was not used, but can be added back in the future if necessary.
Event Name: onCreateUser
Arguments: user, options
No longer emitted. Was not used, but can be added back in the future if necessary.
Event Name: onLogin
Arguments: options
No longer emitted. Was not used, but can be added back in the future if necessary.
Event Name: afterSecurityInit
Arguments: options
No longer emitted. Was not used, but can be added back in the future if necessary.
Event Name: onOrderRefundCreated
Arguments: orderId
No longer emitted. Was not used, but can be added back in the future if necessary. Try using afterOrderUpdate.
Event Name: onOrderShipmentDelivered
Arguments: orderId
No longer emitted. Was not used, but can be added back in the future if necessary. Try using afterOrderUpdate.
Event Name: onOrderShipmentShipped
Arguments: orderId
No longer emitted. Was not used, but can be added back in the future if necessary. Try using afterOrderUpdate.
Event Name: onGenerateSitemap
Arguments: urls
Instead of using events, you can now register a function of type "getPageSitemapItems" to extend the sitemap.

Breaking changes

This does not break anything within the core and included plugins, but

  • if you use community or custom plugins that depend on the @reactioncommerce/hooks package, you will need to update or obtain updated versions that use context.appEvents instead.
  • if you have a plugin that uses MethodHooks, update it to implement those hooks a different way.
  • review all appEvents consumed and emitted by custom plugins. Update expected and emitted arguments. See the table above.

Testing

Look through code and test anything that looks like it might be affected by these changes, for example, anything that runs based on consuming an event.

@aldeed aldeed self-assigned this Jan 11, 2019
@aldeed aldeed changed the title [WIP] Replace all Hooks with appEvents Replace all Hooks with appEvents Jan 11, 2019
Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

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

I took a look at the conflicted file (imports/plugins/core/hydra-oauth/server/init.js), but there didn't seem to be much info on what was in conflict. Perhaps it's the whole file that was deleted and then re-added here? Can you please take a look?

@aldeed
Copy link
Contributor Author

aldeed commented Jan 15, 2019

can this line be deleted now?

That comment might actually be referring to MethodHooks, but I don't see those being used anymore either, so I can remove those and the comment, too.

should we use this PR to update all the existing appEvents.emit to use an object to pass down variables?

Yes! I was thinking of doing that, too. I'm still planning to go through this once more and document all of the changes in a before/after table. So I'll verify they are all using object arg at that time. I'm leaning toward also adding a way to define a SimpleSchema for each event, which we can then validate the objects. That way nobody can mess up.

@aldeed
Copy link
Contributor Author

aldeed commented Jan 22, 2019

@kieckhafer This is updated and ready for another look. Note the table I added to the PR description, which shows how each event has changed.

@aldeed aldeed added this to the 🏔 Torreys milestone Jan 22, 2019
Copy link
Member

@kieckhafer kieckhafer left a comment

Choose a reason for hiding this comment

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

Most everything looks good. I was able to create orders, accounts, update accounts, process orders, update products, publish, etc. Anything that seemed like it touched a hook, I had no issue with.

I came across one issue, however I don't think it's related to this PR, so I've created an issue for it to look into ( #4931 ). My guess is this was introduced in #4908 and missed during that review (by me).

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.

2 participants