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

WIP Rethinking actions and action creation #182

Open
wants to merge 18 commits into
base: master
Choose a base branch
from

Conversation

lkmill
Copy link

@lkmill lkmill commented Dec 2, 2019

This PR rethinks actions and action creators. An action now simply becomes an
unbound function with two arguments, state and store (or only store, see
discussion below). More commonly a user will use action creators, which are
functions that return an action (function).

// without argument
export const increment = () => (state, store) => ({
  count: state.count + 1,
})

// with argument
export const increment = (amount) => (state, store) => ({
  count: state.count + amount,
})

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 is
often required. The above example would then be:

export const increment = (amount) => ({ getState }) => ({
  count: getState().count + amount,
})

I've also updated devtools to differentiate between unnamed actions and direct
calls to setState. Furthermore, the update argument passed to setState 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.

export const increment = (amount) => function increment (state) {
  return { count: state.count + amount },
}

// OR

export const increment = (amount) => (state) => ({
  type: `increment by ${amount}`,
  action: (state) => ({ count: state.count + amount }),
});

Why?

1. Easier to compose action maps for connected components

Before:

import someActions from './someActions'
import moreActions from './moreActions'

connect('stuffs', store => ({
  ...someActions(store),
  ...moreActions(store),
}))(Component)

// or even worse, if we dont want to expose all actions:

connect('stuffs', store => {
  const { oneAction, twoAction } = someActions(store)
  const { anotherAction } = moreActions(store)

  return {
    oneAction,
    twoAction,
    anotherAction,
  }
})(Component)

Now:

import * as someActions from './someActions'
import * as moreActions from './moreActions'

connect('stuffs', {
  ...someActions,
  ...moreActions,
})(Component)

// or even better:

import { oneAction, twoAction } from './someActions'
import { anotherAction } from './moreActions'

connect('stuffs', {
  oneAction,
  twoAction,
  anotherAction,
})(Component)

2. Easier to call actions from other actions:

Before:

import otherActions from './otherActions'

export default (store) => ({
  anAction() {
    store.action(otherActions(store).anotherAction)()
    
    return {
      stuffs: ['interesting stuff'],
    }
  }
})

Now:

import { anotherAction } from './otherActions'

export const anAction = () => (state, { action }) => {
  action(anotherAction())

  return {
    stuffs: ['interesting stuff'],
  }
}

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.

Bind Bound Create Mapped Create
Firefox
70.0.1
14,317,110 ±0.69%
75% slower
17,701,729 ±1.76%
70% slower
58,379,915 ±0.80%
fastest
45,373,182 ±0.48%
22% slower
Chrome
78.0.3904.108
21,238,688 ±0.79%
56% slower
30,728,667 ±0.80%
36% slower
47,844,616 ±0.90%
fastest
47,107,563 ±1.00%
2% slower
Edge
44.18362.449.0
4,862,331 ±4.09 %
43% slower
8,476,679 ±4.08%
fastest
5,315,648 ±3.16%
37% slower
1,689,862 ±2.47%
80% slower
IE 11 4,251,654 ±1.75%
29% slower
5,965,267 ±1.68%
fastest
4,322,828 ±1.49%
27% slower
1,798,760 ±1.52%
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.

full/preact.js dist/unistore.js preact.js
master 760B 355B 546B
feature/new-action-creators 737B 327B 558B

Notes

I had to disable the prefer-spread eslint rule to allow the modifications of
mapAction where .apply(actions, arguments) is used.

@lkmill lkmill changed the title WIP Feature/new action creators WIP Rethinking actions and their creation Dec 5, 2019
@lkmill lkmill changed the title WIP Rethinking actions and their creation WIP Rethinking actions and action creation Dec 6, 2019
@lkmill
Copy link
Author

lkmill commented Jan 3, 2020

@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...

@Akiyamka
Copy link
Contributor

Akiyamka commented Jan 4, 2020

Looks great to me!
Especially part 2. Easier to call actions from other actions:

I agree that two arguments (state, store) is redundant.
Not sure about direct access to store methods is needed in the action.
What could be the case?

@lkmill
Copy link
Author

lkmill commented Jan 4, 2020

@Akiyamka 😍 really happy to hear you like the changes!

you only really need access to getState and action... access to setState becomes kind of redundant now that calling other actions is easier. i passed in store because that was how it was done before, but one of the following probably makes more sense now:

Since getState will more commonly be used, it should probably be the first argument:

export const myAction = () => (getState, action) => ({ ... })

However, if we want to closer mimic redux-thunk:

export const myAction = () => (action, getState) => ({ ... })

@lkmill lkmill force-pushed the feature/new-action-creators branch from 06663cb to 4dc3fde Compare April 6, 2020 15:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants