-
Notifications
You must be signed in to change notification settings - Fork 8
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
Add a simple but reasonably fast Redux version #1
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,61 @@ | ||
import Immutable from 'immutable' | ||
import React from 'react' | ||
import { Provider, connect } from 'react-redux' | ||
import createStore from './createStore' | ||
import Pixel from './Pixel' | ||
|
||
// Reducer | ||
const store = createStore((state = Immutable.Map(), action) => { | ||
if (action.type === 'TOGGLE') { | ||
const key = action.i + ',' + action.j | ||
return state.set(key, !state.get(key)) | ||
} | ||
return state | ||
}) | ||
|
||
const ACTIVE_PROPS = { active: true } | ||
const INACTIVE_PROPS = { active: false } | ||
|
||
// Connected pixel | ||
const ConnectedPixel = connect( | ||
(initialState, initialProps) => { | ||
const { i, j } = initialProps | ||
return state => { | ||
const active = state.get(i + ',' + j) || false | ||
return active ? ACTIVE_PROPS : INACTIVE_PROPS | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Memoization. Could do the same with a library like reselect but this seemed simpler. |
||
} | ||
}, | ||
(initialState, initialProps) => (dispatch) => { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm just curious. Should the first parameter be (dispatch, initialProps) => () => {
const { i, j } = initialProps
return {
onToggle() {
dispatch({ type: 'TOGGLE', i, j })
}
};
} I just want to make sure I didn't miss anything :D There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. To benefit from memoization the most, should the function be generated outside the returned function? (dispatch, initialProps) => {
const { i, j } = initialProps
const props = {
onToggle() {
dispatch({ type: 'TOGGLE', i, j })
}
}
return () => props
} EDIT: See this comment ↓ There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm pretty sure that the Since the returned function doesn't have 2 arguments defined, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh that’s right. Thanks for catching this! 😄 We’re just using the factory function just so that |
||
const { i, j } = initialProps | ||
return { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same thing: no use recomputing this when props change. |
||
onToggle() { | ||
dispatch({ type: 'TOGGLE', i, j }) | ||
} | ||
}; | ||
}, | ||
(stateProps, dispatchProps, ownProps) => ({ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This wasn't essential but since we know all the keys it might be more efficient to hand-roll it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Curious why the need for a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The default one has to loop over keys of each object when |
||
i: ownProps.i, | ||
j: ownProps.j, | ||
active: stateProps.active, | ||
onToggle: dispatchProps.onToggle | ||
}) | ||
)(Pixel); | ||
|
||
// Root component | ||
function ReduxCanvas () { | ||
const items = [ ] | ||
for (let i = 0; i < 127; i++) { | ||
for (let j = 0; j < 127; j++) { | ||
items.push(<ConnectedPixel i={i} j={j} key={i + ',' + j} />) | ||
} | ||
} | ||
return ( | ||
<Provider store={store}> | ||
<div> | ||
{items} | ||
</div> | ||
</Provider> | ||
) | ||
} | ||
|
||
export default ReduxCanvas |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,11 @@ | ||
import { createStore as originalCreateStore } from 'redux' | ||
|
||
export function createStore (reducer) { | ||
const extension = window.__REDUX_DEVTOOLS_EXTENSION__ && window.__REDUX_DEVTOOLS_EXTENSION__() | ||
return originalCreateStore(reducer, extension) | ||
let enhancer | ||
if (process.env.NODE_ENV !== 'production' && window.__REDUX_DEVTOOLS_EXTENSION__) { | ||
enhancer = window.__REDUX_DEVTOOLS_EXTENSION__() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You don't want to slow down the app in production even if user has DevTools There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I agree for real apps. I kept it here so that the readers can try out the DevTools right on There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Fair enough as long as we don't measure with them enabled. I don't think anybody optimized Redux DevTools for perf. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes. I have DevTools turned off when I measure it. I only enabled the DevTools before publishing to GitHub pages. |
||
} | ||
return originalCreateStore(reducer, enhancer) | ||
} | ||
|
||
export default createStore |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We know
i
andj
never change so we use a fast path: return a selector that doesn't look at its prop. Selectors that look atownProps
are much slower.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @gaearon, not really getting this change, you're still using
i
andj
here, how is this not looking atownProps
?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because it pulls
i
andj
out ofownProps
from the initial call, but then returns a function that only uses a single(state)
argument. That way,connect
will only run the returned function when the store state has actually changed, and not when the component's props have changed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markerikson Surely if props have changed then the whole
mapStateToProps
would be re-run returning a new instance of that function?Is the fact that the function returns always the same
ACTIVE_PROPS
orINACTIVE_PROPS
of any significance in all of that?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, that's the point of the "factory function" syntax.
If the first call to
mapState
returns a function instead of an object, thenconnect
will use the returned function instead of the original function.In either case, if the final
mapState
function has a signature of(state, ownProps)
, it will be called whenever either the store state or the wrapper's props have changed. If it has a signature of(state)
, it will only be called when the store state has changed.So, in this example, the first
mapState
function has a(state, ownProps)
signature, but the returned function only has a(state)
signature, so it will be called less often.As far as actually re-rendering,
connect
just checks to see if the object returned frommapState
is shallow-equal to the previous result. Defining these results as constant references doesn't really matter, other than avoiding an object allocation each time.Please see the React-Redux docs on using
mapState
functions for more details, as well as my blog post Idiomatic Redux: The Implementation and History of React-Redux.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@markerikson Many thanks for quick response. I was confused thinking that the optimisation is done by returning
ACTIVE_PROPS
andINACTIVE_PROPS
instead of the object, whereas the real optimisation is turned on by using amapStateToProps
function taking one argument instead of two.I understand the same optimization would apply if
mapStateToProps
returned an arrow function taking one argument instead of two, as was the case originally?You are right, it's all written in the documentation but I read it so many times yet failed to understand how the optimization works. I guess it's because it's the first time I am seeing some functionality/optimization enabled by the fact that a callback takes one instead of two arguments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's the case.