-
-
Notifications
You must be signed in to change notification settings - Fork 15.3k
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
Style Guide: Ducks or Rails Style? #3639
Comments
Yes, I do agree that using "ducks" can obscure the 1:N nature of actions. That was always my biggest concern about ducks, along with the potential for circular reference import issues. That said, the majority of actions really do end up being handled by a single reducer, and far more folks complain about "having to touch a dozen files to work on one feature". I see that as the much more important problem to solve, so that's why I'm now recommending that folks use ducks. In addition, RTK's Part of the issue here is that the FAQ currently attempts to be unopinionated, while the style guide is opinionated. The FAQ is also due for some revising in general. I personally don't care whether the Redux logic for a feature is separated from the components or not. In the projects at my day job, we're currently putting both slices and components in the same feature folder, and while we're still in relatively early development, it's working out okay for us so far. I've definitely also seen folks who wanted to separate out the "data layer" from the "UI layer". Both are valid, and I don't want to prescribe one or the other. All I'm really suggesting here is that users should probably try to keep actions+constants+reducers together instead of in separate folders, and it's probably easiest if you just use My other observation is that I personally have never had a problem with writing code for feature A that depends on data or logic from feature B. I'm fine with just doing that explicit import directly. After all, there is a dependency, you're just acknowledging it. But, I can understand that other folks want to try to write things in a more "encapsulated and isolated" approach. We do plan to add a page specifically discussing file and folder structures as part of our larger docs rewrite. The parent issue for the "Real World Usage" section is #3598 , and we don't have a specific issue yet to track discussion of that page. |
What comes to my mind is something like this: Imagine an authenticated app with the Redux state being persisted on local storage. Whenever the user signs out, I may want to remove some of (but not all) the state from the store. In that case, many different reducers might need to listen to the Using ducks in the "canonical" way might give us the problem of circular dependencies if the I have never faced this very problem before. I had one similar use case once, but as I was using |
Thanks @markerikson, extremely helpful reply. Challenging my mental model I've held since watching https://egghead.io/lessons/javascript-redux-colocating-selectors-with-reducers (https://github.com/gaearon/todos/tree/10-colocating-selectors-with-reducers/src)! I can buy into feature A depending on feature B for the reasons you describe (there is a dependency). As part of converting across, one thing I'm struggling with is where entities would go (in
Thinking in your day job context ( 🙂) (putting both slices and components in the same feature folder):
|
Good question! I actually did something very much like this in my "Practical Redux" tutorial series, which is directly based off the first real React+Redux project I built at work. In that series, I had an You can see the example source here: https://github.com/markerikson/project-minimek That particular work app really did have a lot of relations between the entities, so there was a strong benefit to keeping them in a single For the other couple apps we're working on atm, most of the fetched data types are independent of each other. Also, these apps expect to do initial fetches of each data type, then get a series of "diff" updates over a websocket. For that use case, I whipped up a small generic import {DeepPartial} from "./types";
export type NormalizedEntities<EntityType, IdType extends string | number> = Record<IdType, EntityType>;
export interface UpdateDescription<ItemType, IdType extends string | number> {
id: IdType;
fields: DeepPartial<ItemType>;
}
export interface SubscriptionUpdate<ItemType, IdType extends string | number> {
added: ItemType[];
updated: UpdateDescription<ItemType, IdType>[];
removed: IdType[];
}
export type GetIdFunc<ItemType, IdType> = (item: ItemType) => IdType;
export function updateNormalizedEntities<ItemType, IdType extends string | number>(
entities: NormalizedEntities<ItemType, IdType>,
updateMessage: SubscriptionUpdate<ItemType, IdType>,
getId: GetIdFunc<ItemType, IdType>,
) {
const {added, updated, removed} = updateMessage;
removed.forEach(id => delete entities[id]);
added.forEach(item => (entities[getId(item)] = item));
updated.forEach(entry => {
const existingItem = entities[entry.id];
if (existingItem) {
Object.assign(existingItem, entry.fields);
}
});
} and then we just reuse that in various slice reducers: type ItemsTable = NormalizedEntities<Item, string>;
type ItemUpdate = SubscriptionUpdate<Item, Item["id"]>;
interface ItemsState {
itemsById: ItemsTable;
}
const initialState: ItemsState = {
itemsById: {},
};
const itemsSlice = createSlice({
name: "items",
initialState,
reducers: {
itemsLoaded(state, action: PayloadAction<Item[]>) {
const newItemsById: ItemsTable = {};
action.payload.forEach(item => (newItemsById[item.id] = item));
state.itemsById = newItemsById;
},
itemsUpdated(state, action: PayloadAction<ItemUpdate>) {
const {itemsById} = state;
updateNormalizedEntities(itemsById, action.payload, (item: Item) => item.id);
}
}
}) |
What docs page needs to be fixed?
What is the problem?
The Style Guide 'strongly recommends' Structure Files as Feature Folders or Ducks; this might not be desired in some projects, and may discourage sticking to other recommendations.
Action creators and reducers colocated
While Ducks does provide colocation of action creators and reducers, if my understanding is correct, this works against the 1:N nature of actions to the reducers that handle them. The style guide 'strongly recommends' Allow Many Reducers to Respond to the Same Action - by structuring action creators and reducers together using Ducks, this seems to be impeded.
As stated in https://redux.js.org/faq/actions:
I'd argue that a codebase that sometimes uses Ducks and sometimes doesn't is less maintainable and less understandable than a codebase that uses a single structure consistently. Using Ducks, for a new reducer to start responding to an existing action, this would require a "break out of the paradigm" refactor. My fear here would be that those who don't realise they need to go to the effort of making the refactor just dispatch another action to target its corresponding reducer, going against Avoid Dispatching Many Actions Sequentially.
Components colocated with actions creators and reducers
In addition to structuring action creators and reducers together, the Style Guide also shows components (
Todos.tsx
) structured alongside the action creators and reducers (todosSlice.ts
). I've always thought of the 'tree of Redux state' as independent to the 'tree of React components' - being able to use a single version of truth piece of state from Redux across many components in the application is where I've extracted lots of value from Redux.If many different components want to use a single version of truth piece of state then the tendency would be for more and more code to migrate to
common
. This feels like abuse ofcommon
, and perhaps a smell that Ducks is not the optimal structure for some projects.What should be changed to fix the problem?
If my reasoning is not flawed, state at least that a deliberate choice must be made between Ducks style or rails-style (see https://redux.js.org/faq/code-structure), and outline pros/cons of each. My own opinion (at present) would be to advise that if unsure, prefer the rails-style.
I'd be happy to create a pull request with proposed edits if this is deemed a valid direction.
The text was updated successfully, but these errors were encountered: