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

Refactor after-publish cart updates for speed #5477

Merged
merged 6 commits into from
Aug 27, 2019

Conversation

aldeed
Copy link
Contributor

@aldeed aldeed commented Aug 23, 2019

Resolves #5098
Impact: minor
Type: performance

Changes

After-cart-update performance improvements

There were several "afterCartUpdate" and "afterCartCreate" listeners in various plugins that were essentially transforming the cart but doing it without any coordination was chaotic and slow. Now there is instead a single cart transform function in the Cart plugin, which always runs before every cart insert or update. Plugins now register custom transforms for this using a pattern like this in registerPlugin:

    cart: {
      transforms: [
        {
          name: "setTaxesOnCart",
          fn: setTaxesOnCart,
          priority: 30
        }
      ]
    },

The main cart plugin transform then runs these custom ones in priority order. This is a bit faster, uses less memory, and resulted in consolidated and cleaner code in the Cart plugin.

After-publish performance improvements

The cart plugin has a afterPublishProductToCatalog listener that updates cart item properties from catalog variant properties and, if any have changed, saves the cart. We found that this could be very slow and crash with a Mongo connection error when there are thousands of carts that contain the changed variant.

Now the cart lookups and updates are batched so that memory usage and speed are optimal. This combined with the improved cart transformation flow described above appear to be ~10x speed improvement when publishing to catalog, based on local tests with 8,000 carts.

Other

  • While testing I noticed that Reaction Admin now allows setting tax info for variant options, but when publishing, only the parent variant tax info was copied. Now it copies the option info with fallback to parent variant for backwards compatibility.
  • getSurcharges was calling extendCommonOrder even when there were no surcharges defined. Now it exits early if there are no surcharges, which makes publishing and cart updates faster on shops that don't use surcharges.
  • Much better logging for the post-publish and cart update flows

Breaking changes

None

Testing

Test publishing price and/or tax code changes to a variant or option that is in one or more carts. It should work and be faster than before.

You could also test this with a variant that is in 5-10k carts to verify it no longer crashes (but I've tested this thoroughly).

Almost all code related to carts has been moved around, though is mostly unchanged. The best way to test this is with something like the following smoke test flow in the example storefront:

  1. add first item to cart when not logged in
  2. add additional item to cart when not logged in
  3. log in / register
  4. log out
  5. add same item and different item to anonymous cart
  6. log in
  7. verify all items are merged in account cart and duplicate item qty has been increased
  8. remove an item from cart
  9. change qty of an item in cart
  10. check out the cart

This is prep work for being able to have plugins register
cart transforms that run prior to save.

Signed-off-by: Eric Dobbertin <[email protected]>
This improves the speed of cart updates and simplifies the code paths.

Signed-off-by: Eric Dobbertin <[email protected]>
Copy link
Contributor

@focusaurus focusaurus left a comment

Choose a reason for hiding this comment

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

Some minor nits but overall looks good. Looks like a lot of work sorting this out so thanks for that!

Copy link
Member

@manueldelreal manueldelreal left a comment

Choose a reason for hiding this comment

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

:shipit:

Copy link
Collaborator

@brent-hoover brent-hoover left a comment

Choose a reason for hiding this comment

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

Just one item I think needs to be changed, and a couple of questions/comments that could be deferred to another ticket


let writeErrors;
try {
Logger.trace({ ...logCtx, bulkWrites }, "Running bulk op");
Copy link
Collaborator

Choose a reason for hiding this comment

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

Yay, for use of Logger.trace

imports/plugins/core/cart/server/no-meteor/registration.js Outdated Show resolved Hide resolved
}

/**
* @summary Get updated prices for a single cart, and check whether there are any changes.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think we should be adding/setting a flag here for when the cart has been modified so that the UX could display some sort of message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd need some more information about what you mean. For price changes, that was the idea behind why we have a priceWhenAdded field on there that doesn’t ever change. The thought was that the client can compare price with priceWhenAdded and show a “price has increased/decreased” or whatever messaging they want.

If there are other automatic changes clients would care about, we could do something similar.

@aldeed aldeed merged commit dc448b6 into develop Aug 27, 2019
@aldeed aldeed deleted the fix-aldeed-5098-publish-carts-performance branch August 27, 2019 18:52
@kieckhafer kieckhafer mentioned this pull request Aug 28, 2019
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.

4 participants