Update Shopping Cart Example

[hootlex]

What problem does this feature solve?

The goal of updating the example is to make it more friendly to newcomers and also demonstrate some best practices.

What does the proposed API look like?

After reviewing the shopping cart example and discussing it with @chrisvfritz, I’ve started working on possible improvements.

Summary

• Use separate mutations to update the checkout status and the cart.
• Move/create cart related getters to the cart module.
• Remove abbreviations
• Make the example comply with the style guide.
• Make mutations responsible for updating one piece of state to make debugging easier.
• Use an action (addProductToCart) to commit separate mutations on cart and products modules instead of using a shared mutation.
• Remove mutation types.
• Remove getters that return a piece of the state as is.
• Make modules namespaced
• Rename state properties

Discussion

Mutation types

What do you think of removing the mutation types in order to make the example simpler? Mutation types might be beneficial in some cases but by not using them in the example might make it more friendly to newcomers.

Remove getters that return a piece of state as is

I feel that this pattern might give the wrong impression to newcomers that they always have to use a getter in order to access the state.
Example:

const getters = {
  allProducts: state => state.all
}

Other

I think that we could use more descriptive names in Vuex assets.
For instance added can be cartItems or just items since it’s being accessed as cart.items. In general, I like the items convention for all modules. This way I always know the property name and distinguish the content based on the module name. For example users.items, products.items​.

Also, addToCart could be addProductToCart since a cart can also contain other things.

That might be too peaky but I believe that when people go over an official example it’s very likely to use it as a reference for best practices.

What do you think? 🙂

[chrisvfritz]

Implemented changes #1107

Just reviewed (and approved) that PR! Great work!

Proposed changes

Make the example comply with the style guide.

👍

Make mutations responsible for updating one piece of state to make debugging easier.

Yes please! 😂

Use an action (addProductToCart) to commit separate mutations on cart and products modules instead of using a shared mutation.

Yes! While shared action/mutation names are kind of a cool feature, I think they can make debugging quite difficult.

Remove getters that return a piece of state as is

Sounds great. Your reasoning makes sense.

I think that we could use more descriptive names in Vuex assets. For instance added can be cartItems or just items since it’s being accessed as cart.items. In general, I like the items convention for all modules. This way I always know the property name and distinguish the content based on the module name. For example users.items, products.items​.

I agree that added could be improved, since many things could be "added" to a cart, such as coupon codes, a note if this is a gift, a shipping address, etc. In this case though, I think items might be too vague. Maybe addedProducts or checkoutProducts?

I agree that items can be a good convention for other cases (e.g. users.items, products.items), though my personal preference is actually to use all (e.g. users.all, products.all). A big part of that is probably just having used Ruby on Rails a lot (where User.all is how you query all users in the database), so not sure if it sounds better to everyone. 😅 At the very least, I don't think anyone's complained about that convention in the teams I've used it in.

Also, addToCart could be addProductToCart since a cart can also contain other things.

Yes, agreed.

Other potential changes

Namespaced modules in all (or almost all?) module examples

For this (and other examples) with modules, I wonder if we should start demonstrating namespaced modules as the recommended default. In my experience, they're much easier for beginners to grasp and they scale better, so starting with them allows you to avoid a refactor later.

Global mutations and actions still have their place I think, but they should probably be the exception rather than the rule.

[chrisvfritz]

@ktsn What are your thoughts on this? I think both Vue School and Vue Mastery are starting to look at some of these examples to form their lesson content, so we might have to decide on some best practices for these examples relatively soon before more video content is recorded. 😅

[ktsn]

Make the example comply with the style guide.

👍 💯

Make mutations responsible for updating one piece of state to make debugging easier.

Sounds good to me.

Use an action (addProductToCart) to commit separate mutations on cart and products modules instead of using a shared mutation.

I agree. It would be easier to grasp the flow for beginners.

Mutation types

I'm not sure about this. I actually don't use mutation type constants recently.

BTW, it may be an off topic but I feel it would be better to use action type constants rather than mutation types because actions are used from different places than mutations - actions are usually called from components but mutations are called from actions in the same module.

Remove getters that return a piece of state as is

While I actually like this pattern since we can decouple components with the shape of state, I agree with removing it from example and the reasoning makes sense. I think it's worth stating in the docs.

cart.added vs. cart.items vs. cart.all

I feel items is the most intuitive. But all can be an option if the cart has different kinds of items. In our example, items would be natural since the cart have only products. 🙂

Namespaced modules in all (or almost all?) module examples

Yes, I think it is a good idea too 👍

[hootlex]

@chrisvfritz @ktsn thanks for the great feedback 😄

cart.added vs. cart.items vs. cart.all

I personally prefer the items convention for the reason that when it's an object, individual items can be accessed like cart.items[id] while cart.all[id] feels a bit weird. What do you think Chris?

Namespaced modules in all (or almost all?) module examples

Sounds good to me

BTW, it may be an off topic but I feel it would be better to use action type constants rather than mutation types because actions are used from different places than mutations - actions are usually called from components but mutations are called from actions in the same module.

I agree with that. I also don't use mutation constants since they require more work while the benefit is controversial.

[chrisvfritz]

I personally prefer the items convention for the reason that when it's an object, individual items can be accessed like cart.items[id] while cart.all[id] feels a bit weird. What do you think Chris?

@hootlex I think either one works. 🙂 Probably no matter which one of those we choose, it'll feel a bit weird to some people sometimes.

[lmiller1990]

I think we should use named constants for actions. I've been using name constants for mutations and found it pretty nice, but often make typos for actions.

[kiaking]

I think we can close this issue since the PRs are merged. Let me know if I'm missing anything 👍