-
Notifications
You must be signed in to change notification settings - Fork 139
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
WIP Rethinking actions and action creation #182
base: master
Are you sure you want to change the base?
Conversation
@developit i know you have a lot of more important stuff to focus on, but would love to get some feedback on the changes i propose here... |
Looks great to me! I agree that two arguments |
@Akiyamka 😍 really happy to hear you like the changes! you only really need access to Since export const myAction = () => (getState, action) => ({ ... }) However, if we want to closer mimic export const myAction = () => (action, getState) => ({ ... }) |
06663cb
to
4dc3fde
Compare
This PR rethinks actions and action creators. An action now simply becomes an
unbound function with two arguments,
state
andstore
(or onlystore
, seediscussion below). More commonly a user will use action creators, which are
functions that return an action (function).
I'm beginning to question whether the state argument is really needed. Calling
getState()
isn't that much extra work, and to avoid stale states it isoften required. The above example would then be:
I've also updated devtools to differentiate between unnamed actions and direct
calls to
setState
. Furthermore, theupdate
argument passed tosetState
is now passed on to the subscribers. This was done to show the update object in devtools.Caveats
The only problem with this change is we no longer get naming of actions in
devtools for free. Now you have to name both the action creator and the action
function if you want to get a descriptive name for the action in devtools.
I've also implemented the ability to return an action object instead
of an action function, merely for naming purposes.
Why?
1. Easier to compose action maps for connected components
Before:
Now:
2. Easier to call actions from other actions:
Before:
Now:
3. A little easier to test
Since actions no longer need to be bound it becomes a little easier to test.
Performance
I was initially worried creating new action functions every time an action is called would have negative performance impacts, but I was glad to see that the performance actually greatly improved. At least in modern browsers. The results in the table below are from https://jsperf.com/binding-vs-action-creators.
70.0.1
75% slower
70% slower
fastest
22% slower
78.0.3904.108
56% slower
36% slower
fastest
2% slower
44.18362.449.0
43% slower
fastest
37% slower
80% slower
29% slower
fastest
27% slower
70% slower
Build Size
The full preact and dist builds have shrunk quite a bit, while somehow the isolated preact build increased. File compression is a dark magic.
Notes
I had to disable the
prefer-spread
eslint rule to allow the modifications ofmapAction
where.apply(actions, arguments)
is used.