Closed nbindel closed 7 years ago
Hi @nbindel
Thanks for the detailed proposal, and kudos for providing a working implementation. I wish everyone was this thorough :)
However, I have to say I'm not sure what the benefits of this approach are. To your section on "key benefits":
Simplified setup/usage:
This actually seems more complex - it introduces a great deal of boilerplate for defining actions. As an example, compare the current action definition approach, 43 LOC for 4 actions with your action definition approach 83 LOC for 3 actions
Also, the extra code to support marshalling and unmarshalling actions whose serialization is not trivial is likely to be a performance concern.
Code organization:
You are correct that all code involved with an action (definition, dispatch, etc.) is now in a single place. However I don't understand why that's beneficial. It depends how you slice your concerns. Is an action a concern in and of itself or simply a message in a CQRS system? My feeling is that actions should remain simple, immutable, serializable messages. A record is a much better fit for this than an OOP class in my opinion.
Even back in my Java/OOP days I ended up adopting an approach that separated data from functions into different classes; data classes were immutable and functional classes were stateless. I don't really buy the philosophy of colocating methods and state any more, which is partly why I'm involved with Redux in the first place :).
Increased type safety - no more switch statements
We don't need to go to these lengths to ensure type safety; switch statements, while ugly from an FP perspective also, don't weaken the ability to type stuff, nor do classes inherently make for stronger typing.
My personal approach to type-safe actions comes from acdlite's flux-standard-action, which includes its own type definitions.
FSAs codify a pattern that has become very common in the Flux and Redux communities and is therefore much more accessible to people doing Redux than actions based on a class hierarchy.
export interface FluxStandardAction<PayloadType, MetadataType> {
type: string | symbol;
payload: PayloadType;
error?: boolean;
meta: MetadataType;
}
Due to the benefits of Typescript's excellent structural typing approach, a FluxStandardAction
is assignable to a Redux Action
because it satisfies the field requirements (it has a compatible type
member).
In general, this kind of 'duck-typing' is preferred over the extremely limiting inheritance-only based polymorphism we're constrained to in Java.
So... what about reducers that accept several different action types? Typescript allows for this with union types:
import { Reducer } from 'redux';
import { FluxStandardAction } from 'flux-standard-action';
interface FooPayload { foo: string }
interface BarPayload { bar: number }
interface MetaData { metadata: Object }
export type FooAction = FluxStandardAction<FooPayload, MetaData>;
export type BarAction = FluxStandardAction<BarPayload, MetaData>;
export const FOO_LOAD_SUCCESS = 'FOO_LOAD_SUCCESS';
export const BAR_LOAD_SUCCESS = 'BAR_LOAD_SUCCESS';
// Typesafe: payload must be of type FooPayload.
export const loadFoo = (foo: FooPayload): FooAction => ({
type: FOO_LOAD_SUCCESS,
payload: foo,
meta: null
});
// Typesafe: payload must be of type BarPayload.
export const loadBar = (bar: BarPayload): BarAction => ({
type: BAR_LOAD_SUCCESS,
payload: bar,
meta: null
});
What about a reducer than needs to handle more than one action type? Consider this approach using union types and type assertions:
interface DataState {
foo?: FooPayload;
bar?: BarPayload;
}
export const dataReducer: Reducer<DataState> = (
state: DataState = {},
action: FooAction | BarAction): DataState => {
switch(action.type) {
case FOO_LOAD_SUCCESS: return {
...state,
foo: <FooPayload>action.payload,
};
case BAR_LOAD_SUCCESS: return {
...state,
bar: <BarPayload>action.payload,
};
}
return state;
};
If, like me, you consider the switch
statement undesirable for aesthetic reasons, I can also suggest a more FP (and equally type-safe) way to remove it using a dictionary:
export const dataReducer: Reducer<DataState> = (
state: DataState = {},
action: FooAction | BarAction): DataState => ({
[FOO_LOAD_SUCCESS]: { ...state, foo: <FooPayload>action.payload },
[BAR_LOAD_SUCCESS]: { ...state, bar: <BarPayload>action.payload },
}[action.type] || state);
Final notes
I must at this point confess to a personal bias. I worked for many years as a Java and C++ developer, doing canonical OOP, and I have grown disillusioned with it. class
and extends
bring with them a host of flexibility problems necessitating dependency injection, careful modelling of inheritance hierarchies, and an awful lot of boilerplate. They also invite side-effects and state synchronization problems by making it much too easy to mutate and segment state in uncontrollable, arbitrary ways.
Since making the jump to FP-style JavaScript I have realized that these problems are much more elegantly solved with pure functions, immutability, and referential transparency.
These days I only really use classes when a framework like Angular forces me to.
Don't get me wrong - I love strong typing. Strong typing !== OOP, and a type !== class. TypeScript has a much richer and more flexible typing system, with structural typing, generics, unions, intersections, and typeguards which make it possible to improve code correctness without building large class hierarchies, which is exciting.
Finally, actions in redux are simple (structurally typed) records. This is the pattern widely used by the community, and I'm reluctant to change it for reasons of clarity and compatibilty.
@SethDavenport, First off, let me start by saying thank you for writing up such a detailed response to my proposal. Frankly, it is far more than I was expecting and I also appreciated the background information that you provided. It definitely helped me better understand where you are coming from. As it turns out, I too come from a background in (C++/Java), but I have yet to become as disillusioned about the OO paradigm as you have (not that it doesn't have its issues, some of which you pointed out).
My general programming philosophy revolves around a strong desire to reduce mistakes and errors in not only in solving the immediate problem at hand, but also when taking into account the full life cycle of the application. As such, I try to implement patterns that rely heavily upon the compiler to not only detect various programmatic issues, but also to convey/enforce intent that will help provide guidance and reduce the cognitive load for both current and future developers looking at the code. Consequently, if I can leverage the compiler to provide a compile-time guarantee or can come up with a pattern to follow that will logically eliminate a certain set of potential errors/mistakes then I almost always prefer that approach, even if it ends up making the code a bit more verbose at times. The only time I find myself diverging from this is when the requirements of the application itself can't be met which usually means performance related reasons.
Setup / usage seems more complex: I will agree with you that it does exchange one sort of boilerplate for another, but I would argue that the new form helps reduce the cognitive complexity by making the pattern to follow more obvious and less error prone as I will describe in more detail in the sections below.
Additionally, I'd like to point out that using the direct LOC comparison between those two files is an apples to oranges comparison. This is because the ReduxAction classes implement both the action creator and the action reducer cases. A more direct comparison would involve comparing the sizes of both the resulting reducer files and the actions which actually comes out much closer. In fact, the only reason there is a reducer class at all in my implementation of the example-app is because of the logic you've added to conditionally use it or not. In general, this will not be the common usage pattern and I believe that it could even be eliminated in the example-app by doing a bit of restructuring.
Marshaling/unmarshaling performance concerns: As I stated in my original post as a note, during normal usage this code is never even executed. Nor does it happen when replaying the actions from within the Redux DevTools. Perhaps, I'm misunderstanding what you meant or are referring to here, but I'm not currently aware of a major usage pattern that would require highly performant action restorations from a serialized state. My understanding is that this is primarily useful to allow for the playing back of events for the purposes of debugging, which on the surface seems to be something that would merit performance considerations.
Actions in redux are simple (structurally typed) records (community concerns): I understand the concerns about not following the current standards within the community. However, I'm not proposing to change how everything works. In fact, fundamentally nothing in my implementation changes what's going on at all, it simply organizes it in a different way that I believe solves a number of potential pitfalls within the existing approach. I think part of the confusion here may be due to my reuse of the term "Action". The Redux version of "Action"s in what I'm proposing are still those same structurally typed records (I just called them IDispatchAction and you stated you used "FluxStandardAction"). The only real difference is in how all theses pieces are tied together. If changing the ReduxAction name to something else helps to better convey this or replacing IDispatchAction with FluxStandardAction will provide better community support then I'm all for making those changes.
Provide a simpler means to update the application's state using Redux Requirements: The fulfillment of this objective at a minimum requires fulfilling the following steps in the Redux methodology:
Eliminate the usage of string values as means to providing referential integrity between an action's "type" field and the corresponding intended state change found in the reducer function
The usage of strings here as a way to provide referential integrity relies entirely on the implementing developer's ability to keep things straight. Accidental usage of the wrong string value will cause a disconnect between the dispatched action and the corresponding intended state change. This will happen regardless of whether said reducer makes use of case statements or dictionaries. Additionally, although the usage of string constants does help mitigate the issue here, it does not eliminate the problem since there is nothing preventing a developer from using raw string values instead.
Improve upon existing patterns to make them less error prone and intuitive Considerations:
Improve upon existing patterns to aid in code creation and maintenance over time Considerations:
Maintain backwards compatibility with the existing Redux infrastructure and ecosystem Requirements: Implementation should not affect or limit existing Redux functionality or features or related tooling in the Redux ecosystem.
Provide a simpler means to update the application's state using Redux Solution:
@ReduxAction
Eliminate the usage of string values as means to providing referential integrity between an action's "type" field and the corresponding intended state change found in the reducer function Solution:
By setting up both the action creation and the reducing process in this way we can guarantee that the action "type" being dispatched always results in the execution of the corresponding state change. This to me is a huge win since getting this correct is so fundamental to the entire process.
Unfortunately, there is one possible minor issue that could not be prevented due to the limitations of Typescript/Javascript and that is the fact that the ActionName could be improperly set to the wrong value in the first place. Originally, I had intended to make use of the class' name directly via reflection as a way to guarantee that the type would always correspond to the current action, but there does not seem to be a reliable way to achieve this in Typescript or Javascript. Javascript does provide a "name" field on each class which does actually return the class' name. However, this only works when the code is not minified. Once the code is minified, the class names returned start becoming single letters (and in a lot of cases even the same letter). The only reliable resolution is to manually provide the class' name and then store it on the class. While this is not perfect, I believe the process for creating a ReduxAction does help to eliminate the possibility of incorrectly assigning an ActionName. Additionally, there is a runtime check in place that will throw an exception at application startup to indicate if an ActionName has been utilized previously. Ultimately though this is a non-issue so long as the ActionName used is unique to the system, the functionality itself will continue to work regardless of the fact that it contains the wrong semantic value.
Improve upon existing patterns to make them less error prone and intuitive Solution: This one is more subjective, but there are some definitive benefits to utilizing ReduxActions in terms of code maintenance. To start with, I would recommend creating one ReduxAction per file and organizing them in folders the way they are currently grouped in .action classes. Seeing them in the file structure this way will visually convey their importance and anyone familiar event driven programming concepts will most likely immediately recognize them as part of such a system whether they are familiar with Redux or not. How to add a new action to the system becomes immediately apparent and the fact that all of the logic surrounding their implementation is now in one place instead of multiple places the pattern becomes easier to replicate. Removing a ReduxAction from the system also becomes trivial, simply delete the corresponding file. This will allow the compiler to identify all of the remaining places where it was referenced which will aid the developer in the cleanup process. An additional benefit here by using the single action per file paradigm, is that import statements will also highlight for removal when they are not longer needed. Imports to action creator classes would not highlight since those classes contain more than one action and their reference is not being deleted. Yes, I understand that there is likely additional IDE tooling can help detect these unused imports, but this approach would do so automatically.
Maintain backwards compatibility with the existing Redux infrastructure and ecosystem
Solution:
So far this appears to have been achieved. Fundamentally, the code is just a wrapper around the existing Redux implementation so it should be fully backwards compatible.
Just to be clear, all I am proposing to do here is add an alternative approach to implementing state changes in angular-store. Some may welcome this approach, while others who don't find this approach useful can happily ignore it.
My apologies for this post turning into a bit of a short novel, but I felt it necessary to better explain what I was trying to accomplish and why. Hopefully, this helps to further clarify things.
I have to agree on this with @nbindel.
I am currently in process of switching from ngrx/store to this project. And first thing i was hoping that i will not have to make 2 files to merge action with reducer and a way to remove "raw strings" as they are just a disaster waiting to happen. In ngrx/store i went around this issues making services and additional boilerplate just to ensure type safety and minimalize chances of semantic errors.
This as @nbindel said is just an alternative approach of how to write actions and reducers.
I will try to put this in simpler terms... your current approach is like a while loop while he is proposing a for loop approach. For while loop you have to take care of counter yourself. For loop allows you to take care of counter for you. So i presume both options should work alongside each other without any performance issues...
My 2 cents.
Circling back to this - sorry for the delay, I've been focusing on bugfixes / paid work for a while.
If people find value from this approach, there's no reason it can't be released as a standalone package - the changes to /store appear to be additive only; I have no problem with you publishing it as something that can be used side-by side with angular-redux/store.
However, I don't think this belongs in angular-redux/store itself. It is too much of a departure from standard Redux patterns and bundling it would reduce the focus of the library.
To @xban1x point, "raw strings ... are just a disaster waiting to happen" - I have to say I disgree, for two reasons:
1) I have worked on very large Redux codebases (on the order of 100,000 LOC) and using strings for action types has never been an issue. Not once. Defining constants for them has been sufficient.
2) I'm afraid I don't understand the desire to make each action constant into its own type. Actions have a type: it's Action
. The value of the type
property is data, not a static type, and I don't see the value of treating it as a static type. Redux is a message passing system. The string is part of the message. While there's no harm in making different actions into different types, there's also very little value.
To counter your for
/while
argument, let me describe how I see it. Creating a tree of static types for each message in your Redux system is in my view akin to creating a different type for each integer. E.g. 1 extends number, 2 extends number, and so on. I simply don't see the value.
This is a...
[X] request for feedback / interest level of integrating this idea into the store
Feature: ReduxActions
What are they?
An attempt at a more object oriented and type safe approach to Redux actions that not only provide a means for better code organization, but also eliminate a lot of the boilerplate coding involved when dispatching and reducing actions.
How do they work? All ReduxAction classes get decorated with
@ReduxAction
. This decorator is used to register the Redux action class under the specified "ActionName" (which also serves the purpose of the "type" in traditional Redux) in a ReduxActionRestorer instance. The purpose of this is to store a reference to the class' constructor so that it can later be retrieved to reconstitute an action class from the plain Javascript object that ultimately gets stored within Redux. NOTE: This reconstitution process is only necessary when restoring the application state from its serialized form such as from a JSON file. Normal usage retains the ReduxAction's methods and allows for their usage without rebuilding the class.ReduxAction classes also implement the following interface:
This is further simplified by extending from the BaseReduxAction:
Example ReduxAction converted from the example-app:
A default reducer implementation is also provided that will apply any dispatched ReduxActions onto the current application state as well as restore any ReduxActions that have been serialized.
Usage:
Key Benefits:
The following repos will better demonstrate the full implementation and usage pattern:
Implementation in the store Modified example-app
Please let me know what you think. Thank you.