-
-
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
Recommend that Action constants be named in the past tense #384
Comments
This sounds sensible to me! |
I agree 👍 |
I am not convinced (yet), but open to be convinced. |
In the case of BUTTON_PUSHED, it makes sense - but I dont agree that INCREASE_COUNT should become COUNT_INCREASED - the count isnt increased when the AC creates that action. |
I wasn't immediately convinced. I'll have to think about the idea. |
Actually, in that case the full name would be COUNT_INCREASE_REQUESTED, COUNT_INC_REQD? On Sun, Aug 2, 2015, 04:12 Kier Borromeo [email protected] wrote:
Wout. |
I think that the ActionCreator has the obligation to check if an action is Reducers have the obligation to always keep the returned state valid. The action log is the truth of what happened, and the state is just a On Sun, Aug 2, 2015, 07:07 Wout Mertens [email protected] wrote:
Wout. |
How about a recommendation to write action types in the "imperative mood" (a la the famous Chris Beams' Git commit message style guide). So in this case |
I tend to see |
Personally, I'm getting to a conclusion that we are mixing two design patterns and two models how to think about a system. Both are valid in separation and probably they can be mixed in some applications. The one is based on EventSourcing where Action object represents what happened. Reducers then interprets the past into a state, different reducers can interpret it into different views on the past. The second is based on I think that vanilla flux was more like WAL. I think that both are valid approaches and it just might depend on type of application which one makes more sense. Possibly Redux should stay agnostic to them. My problem with past tense is that it is sometimes tricky to name action objects in past tense. My problem with imperative mood is that it is bound to WAL design pattern. Hence I'm currently investigating an option to use nouns to describe my action objects. They seem to work for both design patterns and allows tricky names. They are similar, sometimes the same as imperative mood, INCREASE_COUNT changes to COUNT_INCREASE, VISIT is VISIT etc. As an example we can take DOM events. Try to name 'mousemove', 'mouseup', 'mousedown' in past tense, I think it will not take long to find the same tricky actions in your domain. It allows 'contextmenu' and such. Redux can just describe all approaches with their pros and cons and let developer to choose which model fits his app and thinking better. I believe that Redux can be agnostic and easily support both. |
Maybe both are acceptable for both design patterns - COMMENT_DELETE or COMMENT_DELETION, i.e. OBJECT_VERB or OBJECT_NOUN as far as OBJECT_VERB is not understood as imperative mood, which is not that much compared to DELETE_COMMENT. |
It is the reason why having both in docs might be confusing (while technically correct in separation): The only way to mutate the state is to emit an action, an object describing what happened. (Three Principles) An action is a plain object that represents an intention to change the state. (Action in Glossary) |
@vladap I guess I see that in same way. |
I like the idea to use nouns! This - apart from easy to name - makes them timeless, they are fact and intent/instruction at the same time. While reducers might be pretty dumb, they still - in my opinion - could contain business logic, like: "count should never become negative". While this is a pretty academic discussion, as long as we do not have a action-type-is-a-noun-validator ;) +1 for nouns |
Couter example it too simple and does not represent this use case. Consider that we sync some database. And we want to show progress bar of that process.
The code is not complete but it shows the idea. Cause we have async it is even imposible to do this in reducer, so it forced to be dump here, but user might be tempted to make some action like SYNC_STARTED in other cases and write some business logic in reducer.
I use immutablejs but some other fellow may preffer plain js and remove all immutable js from reducers, and app still would work. And it can be easily done because all business logic located in action creator. @ioss that what I mean by dump reducer. So there is separation of concerns. |
One more thing is that it would be awesome to dispatch both instructions and facts. But facts should be dispatched in way that they preceded action creator execution as this make possible to generate tests for action creators which I think is the most interesting part for testing. |
@Lapanoid: I am against the totally dumb reducers. This means: If the action.payload is never processed or decided upon by the reducers (because they should not have business logic), the action creators might as well just pass mergeable payload, thus becoming kind of it's own reducer and the "real" reducers could be removed. Also if they are that dumb, why have hotreloading for reducers then? They would hardly ever change. |
These are different use cases. For state that "lives" on server, reducers are dumb because there is little business logic. In fact it makes sense to generate reducers from a schema. For state that "lives" on client (e.g. complex post editor), business logic is described in reducers. |
@Lapanoid Compared to my understanding I think you have facts shifted one layer up towards an user. dispatch(createAction(STATUS_SYNC_DB)('syncing')); These are facts (actions as object) which happened in your AC logic. Set of facts describe execution of a logic in AC.
When user clicks it is not yet a fact by default. When he clicks he executes action as a function (ActionCreator) and some logic which results in facts (actions as an object). Usually synchronous operations are translated 1:1 to actions as facts and whole logic is done in a reducer. Async wrecks havoc on this and forces us to move all async code with all its dependencies to ActionCreators which can result in artificial business logic separation, especially in cases when logic needs to move back and forth between action creator and a reducer. Reducers based on facts are loose model. Reducers can do whatever makes sense based on facts. They can be smart, they can be dumb (just reducing AC results to state). You can use action as object to describe instructions but it is the other model of thinking which is more coupled (but it can be preferred). I would be fine having ACTION AS FUNCTION (ActionCreator) and ACTION AS OBJECT. |
@gaearon: agreed. I didn't mean that there is no place for dumb reducers, just that I don't see that all should be logic free. |
My action objects (facts) are arrogant, they have no intentions, they just say - "this was done" and you, dear reducer, do your business. I don't care. I won't tell you what you should do, it is your business. My action objects might care if reducer kindly asks to better describe what happened but generally he is hesitant to change (to preserve existing kind of API). Having 100 reducers it might be harder to find everything what happens on a given fact though. Which is the case with all event driven programming and highly separated architectures. |
This is a current design, but I think it is wrong actually. If user iteraction generated some immutable by default and only then AC would triggered by that we could test AC execution. This could be done in core transparently I think, pushing user to make additional action on every AC would be brutal.
How it can be done?
AC is also hot-realoded - so this is not an argument
they receive INCREMENT_COUNTER, they don't make any decisions, just make operation. So there is no business logic here. They are dump, but you still need them even in such simple case. They know how to maintain state on actions. |
I think the problem is the word "Action" (in Action and ActionCreator, and that ACs are often kind of used as DomEvent handlers). At the moment i am thinking about going: DomEvent -> ViewActionCreator ("left" part of ActionCreator with VL) -> ViewAction/ExternalAction -> (optionally) middleware/store with ViewActionHandler ("right" part of ActionCreator with BL and possible sideeffects) -> (Store)Action -> reducers. |
Thing is that business logic(BL) in general case is not synchronous, so AC is better place for it anyway. Putting BL sometimes in AC and sometimes in reducers is confusing(maybe it is confusing just for me but anyway = )) |
In terms of event naming style, in my opinion, the past tense style works best for the sort of intermediate ChildView to ParentView events you often write when building Backbone apps, e.g. As an aside, this transformation from DOM event into app domain action that often occurs in a click or change handler implicitly leaks some business logic into the view layer imo. I'm not sure how I feel about that, but as long as its naturally confined to smart/connected components it seems OK since they will be much more tightly coupled to the state layer anyway. We know that once an action strikes the reducer's surface it must be dumb, compromised of only a type and an optional payload. I'm comfortable with action creators and middleware containing business logic, but it seems helpful to stipulate that such business logic strictly only relates to the transformation of a smart action into a dumb action, nothing else. Other business logic would most likely take place on the server, or alternatively the reducer. Fwiw I would support a re-name of Actions to Commands since conceptually it seems to fit quite closely with the command pattern. For me, an action gives the impression that the how of the operation has already been pre-determined which doesn't really fit well with the core business logic happening elsewhere (server or reducer). In reality in Redux all we've done is transformed a DOM event into one of the available app-domain command types, supplied it with any necessary data and forgotten about it ... in other words we issue a command. If anything should be called an "action" it would the description of the event that takes place when a dumb command hits the reducer at a specific point in time. So it's actions that we replay during time travel not commands and their potential side effects. The reducer acts on a command, hence performing a concrete action, but this would be internal implementation terminology abstracted away from library users, who only have to worry about issuing commands ... i.e. intents that something should happen. From Wikipedia on the Command pattern:
Anyhow, sorry if renaming to Commands has been discussed before and decided against, feel free to ignore. Also sorry if I'm taking this off-topic feel free to point me in the right direction. Just my 2 cents worth :-) |
It was just a metaphor:) Trying to convey that action objects should be well thought-out as it can be hard to change them once defined and some reducers are already hooked to them. |
@jedrichards haha, now we have fact, action, instruction, intent and command =) It is funny how people see same thing differently I see that dump action/command/fact actually became smarter in some way as action/intent/instruction prescript what should happened next and carry some info in themselfs about it. And BUTTON_CLICKED as fact does not do that, so it is dumper. A fact interpretates to some set of instructions you may say. |
Yep. Because of that I want some clarity with actions, approach to them really influences on whole app. |
It is the price currently paid for writing hot-reloadable code.
If they contain async code, they are not hot-replayable, you can't do it deterministically. Redux DevTools ignore them on replay and replay operates on results from async AC - on action objects only => hot-replaying code is in reducers only. |
@vladap That makes sense, I did not know that DevTools behave this way, thanks. So they are replayable only if sync? Then we can locate BL always in AC trying to make them as sync as possible |
@jedrichards Command pattern explicitly defines which command to execute, in other words which exact behaviour to execute. It is 1:1 relation. Flux has generally 1:M (many) relation between Action and executed behaviours. Pattern with 1:M relation is called Observer pattern to my knowledge. Renaming actions to commands wouldn't help solve the root cause of the confusion (which is terms used vs. design patterns they suggest by default). Actions are used instead of Commands in some Command pattern implementations. In our context they are practically the same thing. When I'm told it is the Command pattern I expect that one side defines what to execute, give it to other side which blindly execute it. If turnOnRadio command is defined it would be strange to turn on lights. The original Flux is either an overloaded Command pattern where action objects are capable to trigger more and possibly unrelated behaviours, which can be confusing. Or it is an overloaded Command pattern where execution of a single behaviour can be spread across more stores. So to turn on a radio, one store has to plug it while the other store will waitFor till plugged and then turn it on. Or it is an Observer pattern with events strangely named as actions objects, which is confusing. Than I expect that one side just signals something happened without knowing what will happen next. If one side signals that radio was turned on then the other side can turn on lights if wish so. In all cases it can mean that some logic was already executed which resulted either in commands or events. Commands vs. events just define level of coupling. |
|
@vladap could you elaborate on your last sentence? I didn't get it. |
@ioss: Forget it, it is wrong. I think, it can't be done with just combineReducers. |
Like vladap mentioned, it is at the moment not consistent. This should be fixed, see:
|
A proposal: Actions: As some people have mentioned, an action is seen as stating an intent to do something. Currently action creators return intents intend of a user but somehow later these intents are seen as facts. Events: In Event Sourcing events are seen as facts that happened in the past. There is already a concept of an event store (not an action store). Events instead of actions It's just a simple rename but I think it would help. What do you think? |
This my statement doesn't make sense either. Other actions would still go through. I'm not sure why I thought there is kind of a lock-step like in games. It is good to realize because multiple execution of action creators dispatching same actions can potentially lead to race conditions and unpredictable state. |
I think this should be closed as well as #377 (comment) for same reasons @gaearon. (however this discussion was quite enlightening, at least for me) |
Well, l think that simply recommending something in docs is quite different On Tue, Aug 4, 2015, 21:25 Sergey Lapin [email protected] wrote:
Wout. |
Ultimately we start to discuss the same thing - nature of actions which is fine because it is all connected. And I don't see any concensus here. If action is intent it should not be it past tense and if it is fact it should. But there would not be any renaming for reasons from #377 |
Action actualy used in both ways now, because it can be both. Like photon is both wave and particle it depends on perspective. |
yes but by the time it reaches the store the Intents have collapsed into On Wed, Aug 5, 2015, 10:25 Sergey Lapin [email protected] wrote:
Wout. |
I definetelly don't think so. |
I think that Intents are generally understood as user's intents, they represent user's interaction with UI. Intents are then translated either to events/facts or actions/commands whichever pattern you decide to base your system on. I would rather avoid to use Intent term in any other context. User intent can be showMeTop10Todos which is translated to TODOS_LOAD_BEGIN and TODOS_LOAD_SUCCESS/FAILURE. User doesn't care about any of them, they are internal events/facts or actions/commands. User has intents and generally cares about displayed results, he doesn't care what has to be done in between. As @adri mentioned, the common term in EventSourcing is Event or DomainEvent. Event is the common term in Observer pattern as well which is more appropriate seeing TODOS_LOAD_BEGIN and TODOS_LOAD_SUCCESS/FAILURE as what happened and when there is 1:M relation. FACT term is used in OLAP analytics where it is used for fact tables in a star schema, together with dimension tables. I would say that Fact is more general term representing whatever is analysed, it can be an event, measurements.... I'm not aware it is used directly in some programming design pattern, though I don't know them all. |
User intents are one level higher I would say. User intents are transformed to ActionCreators in React components. We can take a user intent, take the current Redux.state and/or the current local state maintained in React component and decide if we actually execute ActionCreator or not. The flow is UserIntent (virtual term, represented by DOM event handlers) > ActionCreator > Action object. |
Maybe there should be Four Principles in docs.
|
This is controversial as proven by this discussion. We won't prescribe any specific naming scheme in the documentation. We have also frozen the terminology now, so we're not going to introduce "events", "facts" or something else. Thanks to everyone for participating ;-). Closing as non-actionable. |
I agree with @Lapanoid that there is very little consensus in this thread. Should a smart component dispatch CLOSE_BUTTON_CLICKED (EventSourcing, Observer pattern) or CLOSE_MODAL (Command pattern) actions? Personally I like @jedrichards's points that smart components can translate events into generic/reusable app domain actions. I think this creates a better separation between ui and business logic. When there are multiple events that results in the same actions this also limits the amount of actions. |
I think it is quite clear: there are things that happened (observer However. The only things that make sense to store in the state are observations, At most, the command can result in the observation that a request was made, Therefore, any action that makes it to the reducers should be written in On Wed, Aug 19, 2015 at 6:25 PM Peter Uithoven [email protected]
Wout. |
As for the CLOSE_BUTTON_CLICKED vs CLOSE_MODAL, imho the actual action that On Wed, Aug 19, 2015 at 11:32 PM Wout Mertens [email protected]
Wout. |
As for me the best idea of this thread is call action nouns without any tense like ITEM_CREATION I am totally fine this this. I think tense itself is the source of confusion. |
I would be more general and say that the main source of confusion is the application of a conflicting mental model. This truly is a matter of perspective; I can argue for past tense easily because the mental model that is most comfortable for me is Event Sourcing, but I have come to understand the perspective of the other suggestions as well. I think that we should refrain from pushing for a firm stance on this because we must understand that people from different backgrounds will benefit from different explanations that fit their natural mental model. To select one suggestion over another is to make it harder for some even if easier for others. To leave this up in the air "officially", but to tailor the explanation of actions to the specific audience of a blog post, tweet or podcast will serve everyone more directly I would think. |
reduxjs/redux#384 There is not a consensus on this issue, but it seemed right. "Actions" could be better thought of as events, they describe a change that has already occurred. The job of the reducer is to make the state reflect that change. Action creators (the functions below) are still written in the imperative.
Since #377 was closed (for valid ecosystem reasons), how about recommending in the documentation that instead of INCREASE_COUNT, it is better to write COUNT_INCREASED?
That way one will be less tempted to put side effects in reducers.
The text was updated successfully, but these errors were encountered: