[RFC] Cart Revamp

[lukeromanowicz]

Feature Name: Cart Revamp
Start Date: 2019-03-01
RFC PR: -

Summary:

Rewrite Cart to meet modern standards and good practices while keeping the backward compatibility.
With this module revamp we'd like to implement also some architectural and coding standards changes to keep the project feature-reach, tough flexible and easy to analyze.

Motivation

Because of the fast growth of Vue Storefront and adding multiple features over time, the core Cart module became extremely unmaintainable. It's not following many of basic standard, it is hard to test and finally, the code is hard both to read and unpredictable. It's a high time to redesign it and implement best practices which can be later spread across other modules.

Action plan:

1. Define required changes

• architecture design - collect, review and choose architectural changes to be implemented
• code quality changes - fully utilize ES6+, follow best practices
• code style and dev environment - choose a coding standard once and for all, guarded by software that will enforce its use (i.e. linter, prettier)

2. Write integration tests that will help keep the backward compatibility after revamp
3. Implement the changes including writing tests (TDD is welcome)
4. Wrap the job up with docs and recommended solutions for often faced problems

Architecture changes

• [RFC] Refactor Vuex API calls to separate layer #2514 - additional API layer responsible for calling API with dedicated adapters interchangeably, instead of performing requests directly from actions.
• make use of Vuex global actions driven development instead of event bus, read more: https://vuex.vuejs.org/guide/modules.html#namespacing events still should be available though deprecated. Move events to functions where the logic really is, instead of calling them before the method - Issue to be crated
• store notification definitions in dedicated files to make them reusable
• Refactor how current cart id and current user token are obtained #2513 - refactor retrieving and storing current cart id/user token

Code quality changes:

• hard typing everything accordingly to Typescript Action plan
• fully utilize ES6+ features like async and await, maps etc; use const instead of let where let is not necessary
• keep in mind that actions are asynchronous and usually should return a promise whenever there is an async dispatch in an action
• avoid keeping all the business logic store
• no direct getting values to a global state from a scoped store - use getters
• modifying state only through mutations
• alphabetical sorting of vuex functions
• keep a consistency of parameter types and naming
• avoid using loggers - mutations and Vuex time travel are really powerful tools to help debugging the app, especially with a little love
• if you really need to use loggers, place them in proper places (i.e. loging that shipping is going to be updated inside action updating shipping, not before that action)

Code style and dev environment

• Enforce coding guidelines with ESLint + TSLint + Prettier #1477 - Choose and implement software enforcing equal code styling rules for javascript and typescript files, suggested eslint, tslint and prettier - research to be done and issue to be created.

---

Please share your thoughts as every input is welcome and needed. Feel free to propose any changes that should be implemented in next-gen VSF modules. If you want to discuss the technical details regarding each point with a linked issue, please, post your thoughts in that issue instead of this topic to keep it clear. I'll update this RFC with conclusions as the discussion goes on.

[pkarw]

I'm OK with this plan as we discussed it before :)

[filrak]

Looks awesome and It's a really good plan! Please start first with an abstract overview of how every action should look like before doing actual implementation. We should also keep c ;)

I don't agree with two points though:

avoid using loggers - mutations and Vuex time travel are really powerful tools to help debugging the app, especially with a little love

Right now it's not DX friendly due to massive number of mutations happening on every action therefore Logger is more useful right now. I really like the examples from the article you sent tho but we need to keep in mind that mutations and actions will be merged in the future versions of Vuex. I suggest making use of action subscribing in Logger as the extension to vue devtools mutations time traveling but this is totally separate topic to discuss.

Also thing that is really important is consistency in names and params of mutations/actions across different modules responsible for similar operations (like listing single product/category, reading/writing cache, receiving the data without mutating state etc). We should have dedicated helpers for such typical use cases and common name/param conventions so every Vuex module is similar on some level of abstraction

Overally great job! I'm really looking forward to see first results!

[ResuBaka]

Some news on the Code Style/Rules. Because i would like to implement prettier. When you have information about it. I would be glad to implement it.

[lukeromanowicz]

@ResuBaka it's not done in 1.9 and there is only a slight chance that I will find enough time to discuss and do it in 1.10.

[ResuBaka]

I have made a small POC where i need some feedback of how the eslint settings should look like.
Should i make a PR for the changes and in there the changes that still need to be done to the eslint or should these be done here in the Ticket?

One of the settings that could be changed is space-before-function-paren. This setting could be changed to ["warn", "never"] or ["error", "never"] as prettier removes all spaces between function name and the opening paren.

[pkarw]

@lukeromanowicz we need you in here I guess :)

[lukeromanowicz]

@ResuBaka
The early version looks very promising. Changes made by prettier look similar to what our ESlint would expect. Please implement also the TSlint and make a PR. We can implement it as solution to #1477

[pkarw]

Partially done within https://github.com/DivanteLtd/vue-storefront/pull/2887/files:

• fully utilize ES6+ features like async and await, maps etc; use const instead of let where let is not necessary
• keep in mind that actions are asynchronous and usually should return a promise whenever there is an async dispatch in an action
• no direct getting values to a global state from a scoped store - use getters
• modifying state only through mutations
• keep a consistency of parameter types and naming

[pkarw]

Closing with the #2887 as for now. Looking for the first developers' feedback to adjust the further refactoring plans. We still need to upgrade the unit tests - which topic has been covered by the new issue #3023