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

[Docs Rewrite] New Page: Style Guide #3601

Closed
2 of 3 tasks
markerikson opened this issue Oct 29, 2019 · 11 comments
Closed
2 of 3 tasks

[Docs Rewrite] New Page: Style Guide #3601

markerikson opened this issue Oct 29, 2019 · 11 comments
Labels
Milestone

Comments

@markerikson
Copy link
Contributor

markerikson commented Oct 29, 2019

This is a tracking issue to cover all work related to adding a new "Style Guide" page / section.

Tasks

  • Outline desired topics
  • Define recommendations
  • Write content
@markerikson
Copy link
Contributor Author

Initial Style Guide Research Notes

The Redux docs have always been very unopinionated. The purpose of a "Style Guide" page is to specifically list our recommended approaches for various concepts, to provide guidance to the community on best practices.

I'm explicitly modeling this page on the Vue docs page at https://vuejs.org/v2/style-guide/ .

Vue Style Guide - Summary and Notes

Vue Rule Categories

  1. Priority A: Essential
  • Prevents errors, so follow them all the time (only rare exceptions)
  1. Priority B: Strongly Recommended
  • Improves readability and DX; justify exceptions
  1. Priority C: Recommended
  • Multiple good options exist, but we recommend a default
  • Prefer the community standard
  1. Priority D: Use with caution
  • Potentially risky features or meant for edge cases

Vue Rules

Priority A

  • Multi-word component names
  • data must be a function
  • Detailed prop definitions (defaults, validation)
  • keyed v-for
  • avoid v-if with v-for
  • scope styles
  • private property names

Priority B

  • File per component
  • Component filenames should be PascalCase or kebab-case
  • Start base component names with Base
  • Singleton components start with The
  • Started coupled components with parent component name as prefix
  • Words in names should be general to specific
  • Self-close component tags
  • Component name casing in templates and JSX
  • Use full words in component names
  • Prop names should be declared camel-cased, but kebab-cased in templates
  • Spread multi-attribute elements across multiple lines
  • Simple expressions in templates
  • Split up complex computed properties into smaller ones
  • Quote attribute values
  • Use directive shorthands

Priority C

  • Ordering for instance options and element attributes
  • Empty lines in instance options
  • Ordering in SFCs

Priority D

  • v-if without key
  • Element selectors with scoped
  • Implicit parent-child communication
  • Prefer VueX for global state management instead of an event bus

Observations

  • A lot of the rules are truly "style"-related (naming, ordering, etc)
  • Page is grouped by priority categories
  • Rules show highlighted examples of "bad" and "good" usage
  • Rule titles have additional priority tags at the end (like "Component Data Essential")
  • Some rules have expandable "Detailed Explanation" sections

I'm curious how we can actually implement some of the highlights and formatting that exist in the Vue page using Docusaurus. @wgao19 , @endiliey , @yangshun : any ideas?

Redux Rules

This is an initial list of rules that I would want the docs to express an opinion on, with initial categories.

I am totally open to suggestions for improving this list: additional topics we should cover, specific points of emphasis, altering categories, etc.

Priority A (prevents errors):

  • Do not mutate data
  • No side effects in reducers (including generating random values like UUIDs or timestamps)
  • Do not put non-serializable values in state, and only in actions if it will be stopped by a middleware before reaching the reducers.
  • Only one store per app

Priority B

  • Use Immer for immutable updates (preferably with RSK)
  • Folder structure:
    • Prefer use of feature folders or ducks, vs folder-by-type
    • Suggest naming "duck files" as someFeatureSlice.js
  • Business logic location
    • Prefer doing as much as possible in reducers
    • Minimize "blind spreads/returns" like return action.payload or return {...state, ...action.payload}
    • Try to treat reducers as state machines that only update if appropriate based on the current state.
  • Action semantics
    • Model actions as "events in the app", not "remote setters"
    • Prefer dispatching one action handled by multiple reducers
    • Minimize multi-dispatch sequences (batch if necessary)
    • Define many meaningful actions for a readable history log, vs just a "SET_DATA" or "UPDATE_STORE" action
  • Action type strings:
    • Prefer "domain/eventName"
  • Use the FSA action format
    • May model actions with separate error types vs error: true
  • Evaluate each piece of state to determine whether it should live in Redux or not
    • Keep form state outside Redux by default
  • Use static typing
  • If using connect, use the "object shorthand" for mapDispatch
  • If using hooks, create multiple smaller useSelectors, and don't bind action creators
  • Connect/select from the store in more components at a more granular level
  • Normalize complex/relational data in the store

Priority C

  • Use action creators for consistency
  • Async middleware
    • Default to thunks; add sagas or observables if you have truly complex async workflows
  • Use selector functions to read store state
    • Use Reselect
    • But, find a middle ground for granularity - don't define selectors for every field
  • Prefer putting more complex sync/async logic outside the component (thunks, etc)

Thoughts?

@yangshun
Copy link
Contributor

I took a quick look at the Vue Style Guide. Having an official battle-tested opinion would be a great addition to the docs.

Having a background for the code might be harder to do in D1 but it's very easy in D2. Maybe we could port the website to D2 while rewriting the docs. The Docusaurus team could help with that.

@markerikson
Copy link
Contributor Author

@yangshun : yep, I figure we can migrate RSK -> RR -> Redux core, in that order. But, I don't want to hold up getting new content in place waiting for a revised publishing approach. We can always format stuff better later. (Or, if necessary, add some manual HTML tags to the Markdown to get formatting.)

Besides, this is probably one of the first pages I want to put together.

@phryneas
Copy link
Member

phryneas commented Nov 11, 2019

I love this. One thing that I personally would add to the "Evaluate each piece of state to determine whether it should live in Redux or not", but don't really know your opinion on:

API Cache state.

Personally, I believe that in most cases this should not be handled by redux, as it will people start at zero/almost at zero every time, leaving them with problems like

  • cancellation
  • cache expiration (which is tightly coupled with the information if there are components mounted that currently use that cache
  • normalization
  • in the future: most likely some problems with concurrent mode
  • explosion of code as usually one api endpoint <-> one slice

Honestly, I haven't seen one implementation in the wild that does all of those right.

Either we should be able to recommend a good middleware for that (if there is one I don't know it), or a few libraries that live outside of the redux space (as for most people, those will suffice and leave redux for the business logic).

What's your take on this?

@markerikson
Copy link
Contributor Author

@phryneas : I definitely don't want to address that kind of topic in the style guide page. It may be worth discussing it in a separate page under the "Real World Usage" category, but tbh those topics are pretty well out of my area of expertise.

@phryneas
Copy link
Member

@phryneas : I definitely don't want to address that kind of topic in the style guide page. It may be worth discussing it in a separate page under the "Real World Usage" category, but tbh those topics are pretty well out of my area of expertise.

Good call. It's loosely related to the form-advice, so I was a bit triggered there, but it shouldn't be in a "best practice" style guide I guess.
If you want to add a page like that somewhere else, feel free to ping me in the issue for that page. I've got loads of opinion on that topic ;)

@markerikson
Copy link
Contributor Author

Sure. Added another task entry to #3598 for a "Data Fetching and Caching" page.

If you'd like, go ahead and create an issue specifically to start planning the contents for that page.

@markerikson markerikson changed the title [Docs Rewrite] Meta-Issue: Style Guide [Docs Rewrite] New Page: Style Guide Nov 17, 2019
@markerikson
Copy link
Contributor Author

I've put up an initial outline for the "Style Guide" page.

For now, I'm keeping it as one single page, but I'm totally open to splitting it up into multiple sections somehow. I've also got the recommendations grouped by priority (same as Vue), but could see good arguments for grouping them by concept somehow (especially if the list was split into pages by priority).

I also borrowed the "flair"-type styling from the Vue docs page for marking each rule header as "ESSENTIAL", "STRONGLY RECOMMENDED", or "RECMMENDED".

Next step is to actually fill out the contents of each rule entry.

I'm interested in feedback on the list of topics and recommendations, the content (once it's written), and the structure.

@wand3r
Copy link

wand3r commented Nov 17, 2019

It's a great idea to put this together but also a challenging one. Thanks for undertaking it anyway :) I'm not sure if this is the correct place for this kind of detailed discussion but let's try anyway.

Normalize complex/relational data in the store

I think we should distinguish two things: normalizing data and having a single source of truth. We don't need to have the first one to fulfil the second one and the latter is much more important. I saw quite a bit redux apps that became unnecessarly complex just because they tried to normalize the whole state (especially the one from the server) when they would have been fine with the more colocated state which is much easier to keep consistent during updates (and with Immer we don't have issues with updating it in an immutable way) and easier to select/read (we don't need complex selector to glue together many pieces of data). What we should strive for is to split our state into aggregates what is my next point :)

Prefer use of feature folders or ducks, vs folder-by-type

I think when we talk about slices or ducks, we just rediscover aggregates. Instead of reinventing the wheel we could try to apply what is already a standard in IT and is well defined. I'm not saying that we should change the naming, but at least we could mention and promote the original concept which could help with better understanding how to slice our state to avoid "overnormalization" and "undernormalization". In my opinion, it is the single most important aspect when working with Redux (and any other data storage) and most problems come from not following principles behind aggregates.

Connect/select from the store in more components at a more granular level

For me, it is more like an optimization than good practice. Connecting components to redux come with a great cost. We sacrifice reusability, testability and readability. Redux is sill a global state and the less we tangle our app with it the better. Clear boundaries between what is connected and what not are really helpful when come to understanding how data flows through our app.

@markerikson
Copy link
Contributor Author

@wand3r : thanks, appreciate the feedback.

I'll agree that with Immer it's a lot easier to update nested state than it used to be. But, there's still benefits for keeping more state normalized, especially when the UI tries to read that data (particularly being able to easily extract a single item from the state based on its ID).

I'm very hesitant to introduce any "new" terminology that isn't already being used with Redux. As it is, the "slice" term is something we sort of came up with ourselves, but it's been in the docs for a while. I get what you're saying about similarities with terms from outside of Redux, but given that there's already a ton of terms in the docs, I'd rather avoid throwing any more terminology in there.

As for the "connect many components" thing, it's a tradeoff. The early docs suggested you should only connect once at the root of your app, but at that point you're forcing the entire app to re-render on every update, which kills perf. If you use connect many times, it's better for perf, but you've got lots of wrapper components. If you use useSelector many times, you drop the wrapper components, but you're embedding the dependency on the store. So, there's a balance to be had, and it's up to the user to decide what that is. I talked about this in my post Thoughts on React Hooks, Redux, and Separation of Concerns, and my ReactBoston 2019 talk on "Hooks, HOCs, and Tradeoffs".

@markerikson
Copy link
Contributor Author

Aaaaand we're live!

https://redux.js.org/style-guide/style-guide

I made some initial progress on the outline and the first couple entries over the weekend, and was able to crank out the rest of it tonight.

We can always make further edits down the road, but this should hopefully be a valuable addition to the docs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

5 participants