Let's use this thread to discuss actual design considerations for an actual hooks API.
Prior references:
Along those lines, I've collated a spreadsheet listing ~30 different unofficial useRedux-type hooks libraries.
update
I've posted a summary of my current thoughts and a possible path forward.
Based on my experiments I've come up with following wishlist for the official redux hooks api:
Provide low level primitives
useMapState() - with setState like === check based render bailoutuseDispatch() - just return the dispatch functionuseStore() - Too powerful?Maybe these higher level APIs
useActionCreators() takes an actions creators object and binds it to dispatch (memoized)useSelector() - reselect like helperDesigned for good TypeScript support. TypeScript is growing like crazy and the HOC based connector is and has been pain for TypeScript users. This is an awesome opportunity to serve TS users propertly.
For the curious I would engourage you to try the hook bindings here
https://github.com/epeli/redux-hooks
It's more than a toy as it attempts to actually implement all the performance requirements needed for real world usage and I would really appreciate any feedback because feedback on it would help the design of these official ones too.
There's a similar project in the Facebook incubator: https://github.com/facebookincubator/redux-react-hook
Personally I lean towards not providing an API that's expected to produce objects at all, but rather a separate invocation for each selector or action creator you want to use.
// user augments this from outside,
// or we need some other trick to pass out-of-band type information
interface StoreState {}
// 2nd argument for these is like a useMemo argument,
// but defaults to [1st argument]. The reasoning is that
// you usually use selectors that were defined outside the
// component if they're 1-ary / creators defined outside
// the component if they're 0-ary.
// one useSelector per value you want to get
// it, of course, also implicitly depends on the
// context store's getState().
function useSelector<T>(
selector: (state: StoreState) => T,
deps?: ReadonlyArray<unknown>
): T
// these return their argument but bound to dispatch
// the implementation is just a memoized version of something like
// return typeof arg1 === 'function'
// ? (...args) => dispatch(arg1(...args))
// : () => dispatch(arg1)
// but the types are way more complicated
// first overload for thunk action creators
function useAction<
T extends (
...args: any[]
) => (dispatch: any, getState: () => StoreState, extraArgument: any) => any
>(
actionCreator: T,
deps?: ReadonlyArray<unknown>
): T extends (...args: infer A) => (...args: any[]) => infer R
? (...args: A) => R
: never
// second overload for regular action creators
function useAction<T extends (...args: any[]) => any>(
actionCreator: T,
deps?: ReadonlyArray<unknown>
): T
// lastly expect a regular action
function useAction<T extends { type: string }>(
action: T,
deps?: ReadonlyArray<unknown>
): () => T
This does have the benefit of never giving you direct access to dispatch, though! Always using bound dispatchers feels way more ergonomic to me. If you want to improve usability further (such as binding certain arguments of a multi-argument action creator) you could always wrap either the input or the output in another useMemo.
This would also have the side-effect of creating a separate subscription per useSelector, though. I don't know if that's a relevant performance consideration or not.
I had an idea to share subscriptions between useSelector calls but it feels redundant:
// fake const, only exists for creating a named type
declare const __SubscriptionToken: unique symbol
type Subscription = typeof __SubscriptionToken
// creates a ref (what the Subscription actually is) and returns it
function useSubscription(): Subscription
// adds itself to a list of selectors the subscription updates which is...
// ...reimplementing subscriptions on top of a subscription?
function useSelector<T>(
subscription: Subscription,
selector: (state: StoreState) => T
): T
The complicated part is when you have a subscription value that depends on the result of other subscriptions -- but you only need _one_ of the subscriptions to update for the component to rerender, and at that point the other selectors will be re-invoked when the useSelector is reached.
If you really want to you can also just return an object but then you have to handle memoization yourself and you can't use useMemo (directly) for it.
const mySelector = useMemo(() => {
let previous
return (state: StoreState) => {
const result = { a: state.a, b: state.a && state.b }
if (!previous || previous.a !== state.a || previous.b !== state.b) {
previous = result
}
return previous
}
}, [])
const { a, b } = useSelector(mySelector)
I also thought of a possible effect-like API but it feels dirty to use. It's "too global" as it's not necessarily coupled to your component; or even if it is, what would it mean to have multiple copies of this component mounted?
function useStoreEffect(
effect: (state: StoreState) => void | (() => void | undefined),
// defaults to () => undefined
deps?: (state: StoreState) => ReadonlyArray<unknown> | undefined
): void
It's like a useEffect but it'd also be invoked _outside_ the React render cycle if the store state changed. Probably too low-level / dangerous, but is roughly the equivalent of getting the store from the context and calling subscribe yourself.
Thinking about this as well and would suggest:
useSelect which would copy the select effect API from sagas. That would let you use your existing map state to props functions with no real changes.useDispatch which would wrap a call to bindActionCreators letting you pass either an action creator, or object to create dispatch functions.Both hooks would use an identity function as the default first argument so the effect of calling them without arguments would be to return the entire state, or a dispatch function respectively.
I think there's lots of room for building on top of these two base hooks but why not start super simple and let the community evolve some patterns?
Partial typescript API (doing this from my phone, so excuse any oddities)
interface useSelect {
<S>(): S;
<S, R>(selector: (state: S) => R): R;
<S, P, R>(selector: (state: A, params: P, ...args: any[]) => R, params: P, ...args: any[]): R
}
interface useDispatch {
(): Dispatch<AnyAction>;
<A extends Action = AnyAction>(actionCreator: ActionCreator<A>): ActionCreator<A>;
<O extends ActionCreatorMap>(actionCreators: O): O;
}
Full implementation (sans tests, examples, etc.) in this Gist - https://gist.github.com/chris-pardy/6ff60fdae7404f5745a865423989e0db
Here's an interesting API idea: Passive state mapping hook that does not subscribe to store changes at all. It only executes when the deps change.
Implementation is basically this:
function usePassiveMapState(mapState, deps) {
const store = useStore();
return useMemo(() => mapState(store.getState()), deps);
}
It makes no sense as a standalone hook but when combined with an active hook it opens up a whole new world of optimization techniques.
Example:
const shop = useMapState(state => state.shops[shopId]);
// Shop products is updated only when the shop itself
// has been updated. So this generates the productNames
// array only when the shop has updated.
const productNames = usePassiveMapState(
state => state.shop[shopId].products.map(p => p.name),
[shop],
);
I don't think you can get more efficient than that. Pretty readable too.
Pretty much a microptimization but avoiding new references can save renders downstream from pure components.
This is available for testing here.
Personally I lean towards not providing an API that's expected to produce objects at all, but rather a separate invocation for each selector or action creator you want to use.
// user augments this from outside, // or we need some other trick to pass out-of-band type information interface StoreState {} // 2nd argument for these is like a useMemo argument, // but defaults to [1st argument]. The reasoning is that // you usually use selectors that were defined outside the // component if they're 1-ary / creators defined outside // the component if they're 0-ary. // one useSelector per value you want to get // it, of course, also implicitly depends on the // context store's getState(). function useSelector<T>( selector: (state: StoreState) => T, deps?: ReadonlyArray<unknown> ): T // these return their argument but bound to dispatch // the implementation is just a memoized version of something like // return typeof arg1 === 'function' // ? (...args) => dispatch(arg1(...args)) // : () => dispatch(arg1) // but the types are way more complicated // first overload for thunk action creators function useAction< T extends ( ...args: any[] ) => (dispatch: any, getState: () => StoreState, extraArgument: any) => any >( actionCreator: T, deps?: ReadonlyArray<unknown> ): T extends (...args: infer A) => (...args: any[]) => infer R ? (...args: A) => R : never // second overload for regular action creators function useAction<T extends (...args: any[]) => any>( actionCreator: T, deps?: ReadonlyArray<unknown> ): T // lastly expect a regular action function useAction<T extends { type: string }>( action: T, deps?: ReadonlyArray<unknown> ): () => TThis does have the benefit of never giving you direct access to
dispatch, though! Always using bound dispatchers feels way more ergonomic to me. If you want to improve usability further (such as binding certain arguments of a multi-argument action creator) you could always wrap either the input or the output in anotheruseMemo.This would also have the side-effect of creating a separate subscription per
useSelector, though. I don't know if that's a relevant performance consideration or not.I had an idea to share subscriptions between
useSelectorcalls but it feels redundant:// fake const, only exists for creating a named type declare const __SubscriptionToken: unique symbol type Subscription = typeof __SubscriptionToken // creates a ref (what the Subscription actually is) and returns it function useSubscription(): Subscription // adds itself to a list of selectors the subscription updates which is... // ...reimplementing subscriptions on top of a subscription? function useSelector<T>( subscription: Subscription, selector: (state: StoreState) => T ): TThe complicated part is when you have a subscription value that depends on the result of other subscriptions -- but you only need _one_ of the subscriptions to update for the component to rerender, and at that point the other selectors will be re-invoked when the
useSelectoris reached.If you really want to you can also just return an object but then you have to handle memoization yourself and you can't use
useMemo(directly) for it.const mySelector = useMemo(() => { let previous return (state: StoreState) => { const result = { a: state.a, b: state.a && state.b } if (!previous || previous.a !== state.a || previous.b !== state.b) { previous = result } return previous } }, []) const { a, b } = useSelector(mySelector)
I'm for this API a lot. On occasions, you need the dispatch (for dynamic actions that can't be treated with actionCreators), so I would add useDispatch.
I think this library should focus on the basic API to allow developers to extend with custom hooks. So caching/side-effect etc. should not be included
Personally I lean towards not providing an API that's expected to produce objects at all, but rather a separate invocation for each selector or action creator you want to use.
100% Agree on this, I think this is the direction things should generally be going with hooks, and it seems to jive with what facebook did with useState.
// these return their argument but bound to dispatch // the implementation is just a memoized version of something like // return typeof arg1 === 'function' // ? (...args) => dispatch(arg1(...args)) // : () => dispatch(arg1) // but the types are way more complicated // first overload for thunk action creators function useAction< T extends ( ...args: any[] ) => (dispatch: any, getState: () => StoreState, extraArgument: any) => any >( actionCreator: T, deps?: ReadonlyArray<unknown> ): T extends (...args: infer A) => (...args: any[]) => infer R ? (...args: A) => R : never // second overload for regular action creators function useAction<T extends (...args: any[]) => any>( actionCreator: T, deps?: ReadonlyArray<unknown> ): T // lastly expect a regular action function useAction<T extends { type: string }>( action: T, deps?: ReadonlyArray<unknown> ): () => T
This feels overwrought, I suggested a simple wrapper around bindActionCreators but even if that's not exactly the API, just getting a dispatch function feels like the right level of simplicity. Something that needs to handle Thunk action creators feels overwrought.
I think it's worth going all the way back to issue #1 as a reference. Dan laid out a list of constraints that the new in-progress React-Redux API would need to follow. Here's that list:
Common pain points:
- Not intuitive how way to separate smart and dumb components with
<Connector>,@connect- You have to manually bind action creators with
bindActionCreatorshelper which some don't like- Too much nesting for small examples (
<Provider>,<Connector>both need function children)Let's go wild here. Post your alternative API suggestions.
They should satisfy the following criteria:
- Some component at the root must hold the
storeinstance. (Akin to<Provider>)- It should be possible to connect to state no matter how deep in the tree
- It should be possible to select the state you're interested in with a
selectfunction- Smart / dumb components separation needs to be encouraged
- There should be one obvious way to separate smart / dumb components
- It should be obvious how to turn your functions into action creators
- Smart components should probably be able to react to updates to the state in
componentDidUpdate- Smart components'
selectfunction needs to be able to take their props into account- Smart component should be able to do something before/after dumb component dispatches an action
- We should have
shouldComponentUpdatewherever we can
Obviously a lot of that isn't exactly relevant for hooks, but which ones _are_ useful, and what other constraints might be good goals?
Feels like most of those original criteria are still relevant. I would rephrase:
- Smart components should probably be able to react to updates to the state in
componentDidUpdate- We should have
shouldComponentUpdatewherever we can
As "shouldn't impact performance".
I'm concerned that hooks would be the ultimate foot-gun for:
- Smart / dumb components separation needs to be encouraged
But I'm not sure there's a good solution other than lots of evangelizing about the benefits of separation of concerns.
- Smart / dumb components separation needs to be encouraged
I think this actually becomes less clear with hooks regardless. I think hooks makes it easier to understand and separate smart container vs dumb presentational components but the effort has to be conscious.
PresentationalComponent.js
export default function PresentationalComponent () {
return // ...
}
connect HOC
// connect container
import PresentationalComponent from 'blah/PresentationalComponent';
export default connect(
// etc...
)(PresentationalComponent);
hooks
Also addressing
There should be one obvious way to separate smart / dumb components
This is it for hooks imo:
// hooks container
import PresentationalComponent from 'blah/PresentationalComponent';
/** with hooks, you need to manually create a "container" component */
export default function Container() {
const props = useReduxState(/* ... */); // not proposing this API btw, just a place holder
const action = useReduxAction(/* ... */);
return <PresentationalComponent {...props} onEvent={action} />;
}
Because you have to manually create the container component, it's less obvious that you should separate container and presentational components. For example, some users will probably think, "why not just put useReduxState in the presentational component"?
export default function PresentationalComponent () {
const props = useReduxState(/* ... */); // not proposing this API btw, just a place holder
const action = useReduxAction(/* ... */);
return // ...
}
I still think the separation of container and presentational components is important but I'm not sure it's possible to create an API where we can make it obvious to encourage the separation.
Maybe this is a problem solely docs can solve?
When using custom hooks predictability is an issue on all fronts.
If you see:
const user = useCurrentUser();
in your component, it's not straightforward whether this component is aware of Redux or not, unless you enforce conventions in your team, like:
const user = useCurrentUser();
@adamkleingit Not knowing that the component uses Redux or not is actually better for your business logic design. Redux is an implementation detail. If your hook is called useCurrentUser, the only thing that the hook consumer should rely on is the fact that the current user will be returned. If later on you decide to switch Redux for something else, you only have to work on your custom hooks, and nowhere else.
@markerikson on the related by different topic of releases would it make sense to work one (or all) of these proposals into a 7.0.0-alpha and put it up with a @next tag. Assuming that also included API compatible implementations of connect, connectAdvanced, Provider than it would be possible to use it as a drop-in replacement, and get some real world testing in order to find any lackluster APIs or performance issues.
@chris-pardy : fwiw, my focus right now is coming up with a workable internal implementation that resolves the issues described in #1177, particularly around perf. (At least, my focus _outside_ work. Things at work are really hectic and draining right now, which isn't helping.)
I personally am ignoring the "ship a public hooks API" aspect until we have a 7.0 actually delivered. Please feel free to bikeshed and experiment with actual implementations. Goodness knows there's enough 3rd-party Redux hooks to serve as starting points and comparisons.
I will point out that any assistance folks can offer with the tasks I listed in #1177 will ultimately result in us getting to a public hooks API faster. (hint, hint)
I've just made an example of use store hooks
Codesandbox UseReduxStore hooks
I've tested it on my application and it works well as I see.
useMappedState example
_Do we have to change mappedState if mapper function is changed?_
export function useMappedState<
S = any,
T extends any = any,
D extends any[] = any[],
>(mapper: (state: S, deps: D) => T, deps?: D): T {
const depsRef = useRef<D>(deps);
const mapperRef = useRef<any>(mapper);
const storeReference = useContext<RefObject<Store<S>>>(ReduxStoreHolderContext);
const [mappedState, setMappedState] = useState(mapper(storeReference.current.getState(), deps));
const currentMappedStateRef = useRef<T>(mappedState);
// Update deps
useEffect(() => {
const store = storeReference.current;
const nextMappedState = mapperRef.current(store.getState(), deps);
const currentMappedState = currentMappedStateRef.current;
depsRef.current = deps;
// Update state with new deps
if(!shallowEqual(currentMappedState, nextMappedState)) {
setMappedState(nextMappedState);
currentMappedStateRef.current = nextMappedState;
}
}, [deps]);
// Update mapper function
useEffect(() => {
mapperRef.current = mapper;
}, [mapper]);
useEffect(
() => {
const store = storeReference.current;
function onStoreChanged() {
const nextState = store.getState();
const nextMappedState = mapperRef.current(nextState, depsRef.current);
if(!shallowEqual(currentMappedStateRef.current, nextMappedState)) {
setMappedState(nextMappedState);
currentMappedStateRef.current = nextMappedState;
}
}
return store.subscribe(onStoreChanged);
},
[], // prevent calling twice
);
return mappedState;
}
useActionCreator example:
export function useActionCreator(actionCreator) {
const storeReference = useContext<RefObject<Store>>(ReduxStoreHolderContext);
return useCallback((...args) => {
storeReference.current.dispatch(actionCreator(...args));
}, [actionCreator]);
}
Create context to hold store reference
export const ReduxStoreHolderContext = React.createContext(null);
export function ReduxStoreProvider({ store, children }) {
// Store object isn't changing? So let's pass only reference to it.
// Don't affect react flow each action
const storeReference = useRef(store);
return React.createElement(
ReduxStoreHolderContext.Provider,
{ value: storeReference },
children,
);
}
And backward compatibility connect might looks like
export function connect(mapStateToProps, mapDispatchToProps, mergeProps?, options = {}) {
const {
pure = false,
forwardRef = false,
} = options;
return (BaseComponent) => {
let Connect = function ConnectedComponent(ownProps) {
const mappedState = useMappedState(mapStateToProps);
const actionCreators = useActionCreators(mapDispatchToProps);
const actualProps = useMemo(
() => (
mergeProps
? mergeProps(mappedState, actionCreators, ownProps)
: ({
...ownProps,
...mappedState,
...actionCreators,
})
),
[ownProps, mappedState, actionCreators],
);
return React.createElement(BaseComponent, actualProps);
};
if (pure) {
Connect = React.memo(Connect)
}
if (forwardRef) {
Connect = React.forwardRef(Connect);
}
return hoistStatics(Connect, BaseComponent);
}
}
Regarding smart/dumb components, Dan recently updated his stance on the subject ... https://medium.com/@dan_abramov/smart-and-dumb-components-7ca2f9a7c7d0, promoting hooks as an equivalent
Update from 2019: I wrote this article a long time ago and my views have since evolved. In particular, I don’t suggest splitting your components like this anymore - from Dan's article
@jbrodriguez oh very interesting. in general, i still think the separation leads to more readable components but I find it fascinating that he doesn't suggest splitting components into presentational and container components anymore.
i think we can use dan's statement to no longer consider "There should be one obvious way to separate smart / dumb components" from his original criteria. doesn't make much sense to consider it anyway i guess?
very interesting and good find
Hey ! I've been working on a package that can help 👉 https://github.com/flepretre/use-redux
Initially it was a basic hook implementation of react-redux with the new API context, but recently I've got a recommandation to use react-redux context's so it's easily plug into a existing react-redux app.
This may be a stupid question but would react-redux hooks depend on the new implementation of connect at all or would a hooks API require another implementation? I.e. can you use connectAdvanced to implement useRedux (or similar)?
My thought is no. Is that right?
No, you cannot use HOCs to implement hooks because HOCs wrap components and hooks are just function calls inside components. But as the new react-redux 7.0 alpha uses hooks to implement the HOCs internally it can probably share some of those internals with the hooks API too.
@epeli , @ricokahler : yeah, it's _possible_ that we may be able to extract some of the logic from our v7 implementation of connectAdvanced, turn that into part of the public hooks API, and then reuse it inside of connectAdvanced. Maybe.
Then again, it may also be different enough that there's nothing to reuse.
Either way, though, you wouldn't use connectAdvanced _inside_ the useRedux implementation itself. Function components can contain/use hooks, but hook wouldn't make use of components internally. (Technically you could _generate_ a component type in a hook, but that's not the same as somehow _using_ the component in the hook.)
Since it's hooks API, would it be a good idea if we implement the API in a hooks way?
i.e.: Avoid wrapping the component by sending the store data to React component state directly rather than via props.
It will be something looks like the following:
function useReactRedux(props, options = {}) {
const { initState, stateSelector } = options;
const [state, setState] = useState(initState);
const context = useContext(ReactReduxContext);
const contextToUse = useMemo(() => {
const propsContext = props.context;
return propsContext &&
propsContext.Consumer &&
isContextConsumer(<propsContext.Consumer />)
? propsContext
: context;
}, [props.context, context]);
const store = contextToUse.store;
/**
* Setup store subscription
* Will unsubscribe when component unmount
*/
useEffect(() => {
return store.subscribe(() => {
const newState = stateSelector(store.getState());
// --- only update when state changes
if (newState === state) return;
setState(newState);
});
}, []);
return [state, store.dispatch];
}
// --- When use the this API, we can:
function MyComponent(props) {
const [state, dispatch] = useReactRedux(props, {
initState: {
count: 0
},
stateSelector: state => state.myComponentState
});
return (
<div>
Count: {state.count}
<button onClick={() => dispatch({ type: "CLICK" })}>Click</button>
</div>
);
}
props, we won't need to mapStateToProps. Consequently, we don't need to worry about the zombie child component issue (mentioned in #1177 ) as there won't be any stale propsHooks wayforwardedRef & wrapperProps etc. as there is no extra higher order component. User can handle those directly if they need. Make react-redux function more pure & cleanI used the same approach to implement my own library fractal-component and here is the hook API implmentation useComponentManager.
Potentially, the similar approach could work for class components as well (so that we can have consistent way of connecting components).
e.g.
class MyComponent extends React.Component {
constructor(props) {
super(props);
this.state = {
count: 0
};
/**
* We can't use React Hooks in Class Components
* But we can manually retrieve context & setup subscription
* at right timing via lifecycle methods
*/
connectToRedux(this, options);
}
render(){
...
}
}
@markerikson I understand that this likely premature given that v7 beta was just released, but do you have any thoughts on a timeline for when the public hooks API might conservatively be released? I ask because I am starting a new application and I'm trying to decide if I should delay the react-redux aspects of it or if I should continue full speed ahead and refactor later. I'd like to avoid some churn this early on if possible. Thanks! And thank you for all of the amazing work on this project.
@klandell : no timeline yet. To be entirely transparent:
connect right now. We need to make sure that works right as our highest priority. Hooks can wait a bit longer.So, frankly, I don't see it happening very soon, and I don't even want to speculate on actual timelines.
@markerikson Refactor later it is then. Thanks again!
@klandell My personal plan to use hooks now is just to use my own redux bindings for now which exposes an hooks api. The idea is to port it to use the official bindings once it ships public hooks primitives which allows me do it. So I won't be forced to refactor anything if want to use something from react-redux in future. Also it would be foolish to not use react-redux when it is possible: The work @markerikson has done with it is highly impressive related to the edge case handling and performance.
The caveat in this is currently that it should not be used in projects that already uses react-redux and connect() because they implement store subscriptions bit differently which can cause subtle bugs if data from connect() is mixed with my useMapState(). I documented here how I went about it.
@epeli : yeah, "top-down updates" and mixing different subscription approaches is one of the biggest concerns I have about a potential hooks API.
To answer your footnote from the linked issue: batchedUpdates is insufficient for correctness purposes for us because the subscription callbacks run immediately, thus referencing whatever the previous saved wrapperProps are. It's entirely likely that this store update would cause parent components to update and thus provide _new_ wrapper props, or possibly even stop rendering this component. We can't know that for sure until the nearest connected ancestor has committed its render, because that means all unconnected components between it and this one have rendered too and therefore we have the latest props accessible.
batchedUpdates only ensures that all queued updates in the same tick get handled in one render pass. We need to _delay_ running mapState until after those renders are done, basically.
Ignore this sorry.
@markerikson Can you explain to me why top-down updates is necessary per se? I understand how the zombie bug happens (thanks to your great descriptions) and I see how that's an unintuitive thing to run into.
However, now I'm wondering if top-down updates are actually required to have working code or if it's possible to program without the assumption of top-down updates (e.g. check and early return if props are stale).
The reason I ask is because I can't really think of clean way to enforce top-down updates and I'm wondering if it's worth the effort in enforcing it.
(AFAIK,) The way top-down updates were enforced previously was via the connect HOC adding a sort of subscription layer by wrapping the components below it with another provider. The HOC gives a nice and clean way to add the subscription layers and I don't know how to do that solely hooks.
So that leads me to the question: Do we really need it?
What do you all think? Am I missing something?
Ignore this sorry.
To backtrack a bit, I have this dream of ridiculously simple redux hooks where the convention is that they should only be selections with minimal calculations. e.g.
import _get from 'lodash/get';
import { useMemo } from 'react';
import { useRedux } from 'react-redux';
import PresentationalComponent from './PresentationalComponent';
function Container({ id }) {
const selection = useRedux(state => state.subStore.something, []);
const otherSelection = useRedux(state => state.otherStore.something, []);
const joinedData = useMemo(() => {
const displayName = _get(selection, [id, 'displayName']);
const title = _get(otherSelection, [id, 'title']);
return { displayName, title };
}, [id, selection, otherSelection]);
return <PresentationalComponent joinedData={joinedData} />;
}
useRedux because it's very clear that it's the thing that makes pushes happen from redux.connect or reselect to memoize, the convention would be to useMemo etc.The issue with the above comes with how something like useRedux would be implemented. Thinking about it naively, every useRedux call could amount to one subscription to the redux store which doesn't scale very well. E.g. if there are 1000+ subscriptions then each unsubscribe would take O(n) time.
To reduce the amount of redux subscriptions, I have this somewhat crazy idea of batching updates via selection so that if two or three different components (no matter where in the tree) select the same pointer, then they'll create only one redux subscription and we can also use ReactDOM.unstable_batchedUpdates to further speed up updates.
For example:
function Foo() {
const selection = useRedux(state => state.foo.a, []);
// ...
}
function Bar() {
const selection = useRedux(state => state.foo.a, []);
// ...
}
function Baz() {
const selection = useRedux(state => state.foo.a, []);
// ...
}
Behinds the scenes, useRedux would see that each Foo, Bar, and Baz all selected the same pointer so it can assume that they can be batched into the same redux subscription.
This requires:
I just think redux hooks should be _simple_ and easier to understand than connect. I think make a simple hook API will lead to better conventions (e.g. making custom hooks for joining data with props).
I also think it echos the philosophy of hooks too. Hooks exist to create a simpler state primitive so maybe a simpler redux subscription primitive would be better too.
Let me know what you think!
Top down order updates are needed because if a child updates before a parent, the same child might receive outdated props from a connected parent after it has already rendered.
@ricokahler : first, it's worth going back and reviewing the original design constraints for the React-Redux connect API (which I quoted earlier in this thread).
In particular, these two constraints are highly relevant:
- Smart components'
selectfunction needs to be able to take their props into account- We should have
shouldComponentUpdatewherever we can
In other words:
<ConnectedListItem id={123} />It's certainly possible to create a hook that reads from a larger chunk of the store, and then lets you handle pulling out a specific piece based on props in your component's render logic. You can mimic that right now:
const mapState = (state) => {
return {todos: state.entities.Todo}
}
render() {
const {todos, todoId} = this.props;
const todo = todos[todoId];
// render it
}
The issue is that will cause this component to re-render unnecessarily when any other todo is updated, thus causing perf overhead. The same would be true in a hook that just extracts that bit of state and calls setState(todos).
Now, it's possible that the use of unstable_batchedUpdates() would lessen the impact of that somewhat, but based on what I've seen with v5, v6, and v7, the best approach for minimizing perf impact is to _only_ force a re-render when we know that the exact state (extracted and derived) that _this_ component needs has actually changed. Getting React involved on every store update is simply more expensive, period. That's why v5 and v7 are faster than v4 and v6.
The next issue is potentially mixing connect and useRedux. If connected components are updating in one sequence, and components calling useRedux update in a different sequence, that's going to cause both unnecessary re-renders _and_ inconsistent behavior.
It's hypothetically possible that we could somehow come up with a way to enforce top-down updates by manipulating subscriber ordering based on the tree structure. At that point, though, you're actually sort of reinventing React's reconciliation algorithm :)
Top down order updates are needed because if a child updates before a parent, the same child might receive outdated props from a connected parent after it has already rendered.
@saboya yeah this all makes sense now. ignore my previous "does order really matter?" thing.
I'm still for the useRedux API though. I think if we can get that API to be performant, that's the way to go.
Smart components' select function needs to be able to take their props into account
I think I implied that useRedux couldn't accept props but that's the reason for the second argument of the dependencies array useRedux(state => /* ... */, []) // 👈.
Without the "no order guarantee thing", this is actually the same API as @epeli's useMapState and @Jessidhia's useSelector, just renamed to useRedux because I like the aesthetic.
@markerikson I'm spit-balling here so I apologize if things don't make sense but maybe we could have an overload for connect that would enable useRedux with top-down updates?
So the new "container" could look like this:
import { connect, useRedux } from 'react-redux';
import PresentationalComponent from './PresentationalComponent';
function Container({ id }) {
const todo = useRedux(state => state.todos[id], [id]);
return <PresentationalComponent todo={todo} />;
}
export default connect(Container);
Throwing the HOC back into the mix would allow for another Provider to wrap the Container and then useRedux can pick up on the new context.
Maybe something like that would work??
maybe we could have an overload for
connect?
OH PLEASE NO NO MORE OVERLOADS FOR CONNECT AHHHHHHHH
runs away screaming
So asking a more serious question:
Not sure I follow, what do you mean by "component props"? How would you extract specific parts of your store if not by using your props to cherry-pick from your state?
@saboya : yes, exactly.
In other words, how do other Redux hook libs translate this:
const mapState = (state, ownProps) => {
return {
todo : state.todos[ownProps.id]
}
}
into hooks form?
@markerikson
How critical is it to be able to use component props as part of your state extraction logic?
I think using component props as part of your state extraction logic is not the root cause to the zombie child problem.
Connecting Redux store data to component via Component Props is the actual culprit.
i.e. If you want BOTH using component props as part of your state extraction logic & Connecting Redux store data to component via Component Props, you will have to provide an API like:
const mapState = (state, ownProps) => {
return {
todo : state.todos[ownProps.id]
}
}
Keep in mind that the Component Props are only up to date during rendering staging (i.e. when render() function is called or function component is rendered), you will always have to deal with outdated Props.
On the other hand, if we connect Redux store data to component via Component State (as I suggested in my post), we have still have the using component props as part of your state extraction logic feature. However, it will not provided as part of the API and be done at rendering staging.
i.e.:
We only need provide an API like:
const mapState = (state) => {
return state.todos
}
And the API user can still use component props as part of your state extraction logic by:
function MyToDo(props) {
const todos = useRedux(mapState);
// As it's in render stage now, `props.id` is guaranteed to be an up to dated one
return <PresentationalComponent todo={todos[props.id]} />;
}
Consequently, the following
How do any of the other "unofficial" Redux hooks libs handle working with component props?
won't be a problem now as it's React's reconciliation algorithm makes sure
return <PresentationalComponent todo={todos[props.id]} />;
never cause issues (either never be reached / called due to component unmount or only passing up to date props when render)
@t83714 : as I said a few comments ago, the problem is that wouldn't be good for performance. This component would be forced to re-render any time state.todos changed, not just when state.todos.123 changed.
I'm not saying that makes the approach impossible, just pointing out that it's likely to lead to worse performance overall. That _does_ make it a less likely candidate approach.
@markerikson
Thanks for pointing out the performance issue.
Understood that's a hard decision to make but thought solving the performance issue might be easier than fix the top down update issue as:
PureComponent or React.memo)Connect Redux via Component Props apporach as we always have to call setState to trigger the renderingCould we fix the simple example above by:
function MyToDo(props) {
const todos = useRedux(mapState);
// As it's in render stage now, `props.id` is guaranteed to be an up to dated one
const PurePresentationalComponent= React.memo(PresentationalComponent);
return <PurePresentationalComponent todo={todos[props.id]} />;
}
I mean, probably, it should be something very common and up to user to fix within user space?
If we really want to include it as part of the API (although I think it should be user's job to handle it), nothing stop us to provide API like:
function MyToDo(props) {
return useRedux(mapState, todos => {
return <PurePresentationalComponent todo={todos[props.id]} />;
}, true);
// --- here `true` indicates a Pure component and by default should be false
}
@t83714 : the immediate issues with those suggestions are :
And the difference with what connect does now is that we only force a re-render if the derived value has changed.
I still desperately wish that the v6 approach had worked out. If we were getting the store value from context, none of this "stale props" stuff would be an issue at all. Unfortunately, the combo of perf issues and inability to bail out of context updates made that turn out to be a dead end for now.
I do agree that this is the perfect time to reevaluate things and maybe come up with a very different API. But, it's also true that the current API exists because these are exactly the kinds of things people want to do.
@markerikson
Thanks a lot for your feedback.
I didn't mean to avoid re-rendering the MyToDo component. I was trying to stop the re-rendering of
component PresentationalComponent.
The shallow comparison result of todos[props.id] should be unchanged. Thus, component PresentationalComponent won't be rendered, right?
Probably only the only mistake I made in my example was that creating a Pure version of the component should be outside the function component (or using the React hook API):
// create a Pure version of Component. This probably should be the user's job
const PurePresentationalComponent= React.memo(PresentationalComponent);
function MyToDo(props) {
const todos = useRedux(mapState);
// As it's in render stage now, `props.id` is guaranteed to be an up to dated one
return <PurePresentationalComponent todo={todos[props.id]} />;
}
Can I confirm whether it would still be a performance hit if we can avoid re-rendering PresentationalComponent?
Just re-rendering MyToDo component seems won't create too much work as it leads to no new work to the commit phrase (because MyToDo returns the same tree (PresentationalComponent) that doesn't require re-render)?
@markerikson
So asking a more serious question:
* How do any of the other "unofficial" Redux hooks libs handle working with component props? * How critical is it to be able to use component props as part of your state extraction logic?
It's very critical.
I create components like this with @epeli/redux-hooks all the time
function Avatar(props) {
const user = useMapState(state => state.users[props.userId]);
return <img src={user.avatarURL} />;
}
No issues with tearing so far that I known of.
@t83714 Here's how I handle the perf issues if you are interested in a working implementation: https://github.com/epeli/redux-hooks/blob/master/docs/optimizing.md No need to use pure components or React.memo()
So, as @markerikson knows, I have been thinking about this quite a lot recently (he had to suffer through my ramblings in the other issue ;) )
Most of the solutions I see posted in here still will suffer from the zombie child problem.
@t83714 Your solution also won't work unconditionally, since your assumption that in the render phase all props are up to date is only true if the update that causes the re-render is using batching, since otherwise the setState call inside the store subscription callback will cause an immediate re-render of the component before the parent component could handle the store update and update the props. I think it will be easier to see this in action yourself (see the console output). I slightly adjusted your code, but the idea is still the same. As you can see, the invariant should be that the count from the props is always the store count + 1, but without batching you see an inconsistent render. With batching (i.e. useProxy = true inside App) it works correctly.
With all this being said, react-redux is already using batching for the tiered subscription approach, so for your suggestion to work it just needs to be ensured that the store the hook is subscribing to is also using batching for notifying subscribers. Maybe you already took this into account, when writing your suggestion. If so, I apologize.
In response to this
Just re-rendering MyToDo component seems won't create too much work as it leads to no new work to the commit phrase (because MyToDo returns the same tree (PresentationalComponent) that doesn't require re-render)?
You are right, that only MyToDo's render executes which the children being memoized. However, I remember reading somewhere (can't find it right now sadly) that in terms of performance impact the render phase is actually the expensive one, not the commit phase (since behind the scenes much more than just the function call to MyToDo happens during render). That means if you have many many components using the hook, you will still feel the performance impact.
@epeli Indeed, your solution does not suffer from the classical zombie child problem. However, it is easily possible to show that in your version sometimes the mapState is called with inconsistent props and state. This is still a stale props issue that can cause the mapState to throw.
@MrWolfZ Damn. I have a test for this case. Not sure why it passes actually now... But I probably need implement similiar mapState delaying as react-redux does. Thanks for the heads up!
@epeli your version of hooks is really similar to how I imagined it could look like. My idea for fixing the stale props issue was to just catch all errors when calling mapState in the subscription callback, and then just force a re-render if an error was caught. This will cause the mapState to be executed again and throw again (if the issue still persists), or return the correct value. In theory this should work, but I feel dirty just swallowing errors like that.
Also, it has timing issues that make this impossible with concurrent mode, but no hooks solution I saw so far is compatible with that anyways due to the way the store is accessed.
Alright, I've been doing some soul searching, I've finally read the v7 code, and I think I have a general direction for how I think these redux hooks thingies should work:
useRedux(selectorFn, dependencies) is the simplest and most versatile API. It's more primitive than some other suggestions but I think it'll serve as the best building block for custom hooks that useRedux (and look how cool that pun is!!). Also, many of the suggestions above included this API so I think it's an API most people would expect. We can include others, but useRedux seems to be the best building block and starting point.connect. The only way I can foresee this being possible is by re-using the same tiered Subscriptions that connect uses.connect propagates those subscriptions using context by wrapping the wrapped component's children with another Provider. The way I see, the hooks version of connect will still require an HOC but this one doesn't need to contain any parameters. It'll look something like this:// `enableReduxHooks` is a placeholder function name
import { enableReduxHooks, useRedux } from 'react-redux';
function Todo({ id }) {
const todo = useRedux(state => state.todos[id], [id]);
return // ...
}
// still need to wrap in order to propagate context and tiered subscriptions
export default enableReduxHooks(Todo);
useRedux more than once in the function component, it makes sense that the context that enableReduxHooks provides will create one redux subscription and all the hooks on that level would share that subscription.useRedux run in one tick.Remarks:
useRedux _and_ connect to play nicely together since we're using the same tiered subscription mechanism.enableReduxHooks wouldn't actually affect how people use hooks. People can still create custom hooks using useRedux and not have to think about enableReduxHooks. e.g.// this custom hook isn't aware of `enableReduxHooks` and everything is fine
function useTodo(todoId) {
const todo = useRedux(state => state.todos[todoId], [todoId]);
return todo;
}
const Todo({ id }) {
const todo = useTodo(id);
return <div>{/* ... */}</div>
}
export default enableReduxHooks(Todo);
useRedux calls and ensures the closest component wraps with enableReduxHooks. To make adding the HOC less of a thought.enableReduxHooks is not the best name in my opinion. I originally thought overloading connect would be cool but that made @markerikson run away screaming 😅. Please come up with a better name.What does everyone think about that approach?
Vote with the 👍 or 👎reactions.
@markerikson I'd be willing to work on a PR off the v7 branch with this approach if you're open to the idea. I wouldn't expect you to pull it in immediately or consider it too seriously. At the very least, I think it would start a good topic of discussion. What do you think?
@ricokahler : Please, go ahead! I'd absolutely welcome proof-of-concept PRs for discussion, especially now that we have a better idea what some of the constraints are.
The discussions over the last couple days have gotten my brain to start chewing on this a bit. No specific ideas atm, but thoughts are at least circulating.
I would really like to figure out something that doesn't require use of a HOC at all. I don't know what that might be atm or if it's even feasible, but I assume most hooks users are looking to drop HOCs entirely.
I think a dependencies array is the obvious way to deal with props, it’s a well known pattern in hooks land and minimal effort, especially with the lint rule. Redux is all about being more explicit over being most succinct anyway.
Needing to wrap components to use hooks is obviously extremely unintuitive. Do none of the other libs have a better solution to this issue? Since it only is needed to handle mixing and matching connect and hooks, there should be a way to opt out for projects that will not mix and match, then it sounds like a workable intermediate workaround.
Did some reading on the v7 code too and came to pretty much to the same conclusion as @ricokahler: It doesn't seem like it's possible to implement hooks without some kind of wrapping component unless React itself would come forward and give us something like
My hooks implementation is indeed way too naive.
My hooks implementation is indeed way too naive.
@epeli If it makes you feel better, I have an even more naive useRedux implementation in prod right now lol
I agree with everyone that the hooks still requiring the HOC is annoying/gross but I think in practice, it won't change how people use the redux hooks. See this example pasted from my previous post:
// this custom hook isn't aware of `enableReduxHooks` and everything is fine
function useTodo(todoId) {
const todo = useRedux(state => state.todos[todoId], [todoId]);
return todo;
}
const Todo({ id }) {
const todo = useTodo(id);
return <div>{/* ... */}</div>
}
export default enableReduxHooks(Todo);
I would also like to figure out a way to do it without a wrapper but I don't think it's possible.
For those who haven't read the code: the only reason we need the wrapper is to add that subscription layer to enable top-down updates. See here from the v7 branch:
https://github.com/reduxjs/react-redux/blob/79982c912d03c55ac7059b61806a1387352e91f3/src/components/connectAdvanced.js#L387-L394
This Provider propagates the Subscription instances to the next component wrapped in connect:
By the nature of HOCs, it's easy to wrap the component with another provider seamlessly but with hooks, there is no way to do that 😭
After I figured that out, I admitted defeat and started seriously thinking about how to implement useRedux (aka useMapState, useSelector) with the constraint of the wrapper and it's actually kind of nice from an implementation standpoint.
Earlier in the thread, @Jessidhia brought up the same API as useRedux just named useSelector and she said:
[The
useSelectorAPI] would also have the side-effect of creating a separate subscription peruseSelector, though. I don't know if that's a relevant performance consideration or not.
With the wrapper, instead of having one redux subscription per useRedux (aka useSelector) call, it would be one redux subscription per enableReduxHooks call.
Internally the enableReduxHooks wrapper could create one subscription and manage pushes to each useRedux call inside that "subscription layer" (where each enableReduxHooks call is one layer).
I think this would bring performance on par to how connect works now because this would make it work like how connect works.
It's unfortunate and gross that the HOC is still needed but with this constraint there is a clear path to fully function redux hooks that work alongside with connect 🎉 (in my mind at least)
Maybe I'm not thinking outside of the box enough but if we can figure out how to get the tiered subscriptions (e.g., the "order matters") _without_ the wrapper then we can ditch the HOC.
@ricokahler your posts reflect my own thoughts almost exactly. I also got to the conclusion that currently the HOC will still be required. Instead of enableReduxHooks my idea was to just call it connectHooks to stay in line with existing naming.
I think this code (basically your example with some inlining) is quite concise and readable.
export const Todo = connectHooks(({ id }) => {
const todo = useRedux(state => state.todos[id], [id]);
return <div>{/* ... */}</div>
})
Regarding the alternative approach of "enforcing" that no props are used for selecting the state (which would only be possible with linters as far as I can see), I think it would definitely be worthwhile to do some performance tests to see how big the impact of those additional renders really is. I have some free time on my hands the next couple of weeks, so I may try my hand at it.
Lastly, I also plan to take a deep dive into the fiber implementation. Maybe I'll find some hidden nugget that would allow us to have a proper hooks-only implementation.
Giving up the possibility of filtering state with props is not an option in my opinion. Maybe for some, but for some cases it's going to cause a huge amount of unnecessary renders.
I think the hybrid approach (HOC + hooks) is the way to go for now. The main issue with the HOC isn't the HOC itself IMO, but the code clutter and ergonomics. And, if in the future there's a HOC-less solution, the migration path will be almost pain-free, and the HOC can be updated to a NOOP.
I _really_ don't like the idea of having to use a HOC just to use hooks.
Unfortunately I do agree that it looks like it may be a necessity atm, but I would really like to try to come up with another option.
The vague notion I'm tossing around in my head is if there's some way we can construct our own in-memory tree structure in <Provider> that mirrors the shape of the store subscriptions in the React tree, so that we can trigger updates in the appropriate sequence. Problem is, we don't actually know where each component is in the tree.
If y'all have any ideas on how to pull off such a feat, I'm all ears.
@saboya One of the suggestions above was something like the following code, which would put some burden on the library users to always remember to use useMemo, but should keep performance problems minimal due to not everything being re-rendered (again, only tests with concrete numbers will verify or contradict this).
export function Todo({ id }) {
const todos = useRedux(state => state.todos);
const todo = todos[id]
return useMemo(() => (
<div>{/* ... */}</div>
), [todo])
})
@MrWolfZ : the question of _how many_ components render is only part of the issue.
In that snippet, yes, the child components memoized by the useMemo() would only re-render when todo changes, because React will bail out when it sees the === elements in this spot as last time.
However, the Todo component itself will still have to render even if todo hasn't changed, and it's the aspect of React getting involved _at all_ that's ultimately a perf issue.
Now, we ought to try building these options out, and then updating the benchmarks suite to have some hook scenarios for comparison with our existing HOC scenarios. (No idea how _that_ will work out).
I don't think using dependencies to useRedux is a good idea, suppose we use redux like:
const User = ({id}) => {
const user = useRedux(
state => state.users[id],
[id]
);
return <div>{user.name}</div>;
};
It looks very concise and readable by specifying id as a dependency to mapState function, however this implies a fact that the selector won't change around renders, which is not a stable consumption:
const User = ({userType, id}) => {
const selectUser = userType === 'admin'
? state => state.admins[id]
: state => state.users[id];
const user = useRedux(selectUser, [id]);
return <div>{user.name}</div>;
};
User won't get correct result from redux if userType is changed without id change, and lint rules like exhaustive-deps is not able to detect the fact that selectUser actually depends on userType variable.
We have to tell developers to modify this example code to:
const User = ({userType, id}) => {
const selectUser = state => userType === 'admin' ? state.admins[id] : state.users[id];
const user = useRedux(selectUser, [userType, id]);
return <div>{user.name}</div>;
};
This may be OK in a self controlled app but is not the case as a general purpose library like react-redux, we should try to satisfy all kinds of developers without having implicit restrictions.
I'd prefer to manage my selector via useCallback myself:
const User = ({userType, id}) => {
const selectUser = useCallback(
userType === 'admin'
? state => state.admins[id]
: state => state.users[id],
[userType, id]
);
const user = useRedux(selectUser);
return <div>{user.name}</div>;
};
This is more straightforward and is able to utilize exhaustive-deps lint rules
Unfortunately I do agree that it looks like it may be a necessity atm, but I would really like to try to come up with another option.
@markerikson a babel plugin? idk
last time I checked. create-react-app didn't play nicely with those though
Edit: there are these things called babel macros that work with create-react-app. I'm not sure what they do exactly but maybe we can babel some HOCs in magically.
Edit edit: In order to use babel macros, the importing line has to match /[./]macro(\.js)?$/.
Is that cool or gross? import useRedux from 'react-redux/macro' is gross.
Eh that's about as far as I'm willing to go for considering babeling enableReduxHooks.
@otakustay this is a bad example. Just like with any hook, functions also need to be specified as dependencies, so if you pass selectUser as a reference, then you need to specify it as a dependency and the exhaustive-deps rule would also capture that missing dependency on the function in useMemo or useCallback. In your example id is actually not a dependency of that hook but just the function is. See https://reactjs.org/docs/hooks-faq.html#is-it-safe-to-omit-functions-from-the-list-of-dependencies and also https://overreacted.io/a-complete-guide-to-useeffect/
@MrLoh
Just like with any hook, functions also need to be specified as dependencies
Except useCallback. What I'm discussing is actually "should useRedux includes the capability of useCallback", as a result I said NO, we should just keep selector function as a dependency of useRedux
@MrWolfZ
Maybe you already took this into account, when writing your suggestion. If so, I apologize.
Thanks for the sample code. I noticed React-Redux's Subscription implementation but I think we can avoid that by connecting redux via state 😄
My statement all props are up was talking about one component.
I think why the zombie child component is unintuitive & really need a fix is because that's the situation where data of the SAME component are consistent.
i.e. the stale props (props.id) and state (todos[props.id]) of the same component could be not synced (at a point in time)
The case of your sample code is two components' internal state could be not synced at the same time.
I think we probably shouldn't need to worry about this case too much as you probably hardly see it in a real-life example? --- If you subscribe to the same state, there would be no point of passing the state via props. And if you don't pass the state data to the child component via props, it will be no chance that two copies of states exist in the SAME component.
@markerikson Regarding this comment
@MrWolfZ : the question of _how many_ components render is only part of the issue.
In that snippet, yes, the child components memoized by the
useMemo()would only re-render whentodochanges, because React will bail out when it sees the===elements in this spot as last time.However, the
Todocomponent itself will still have to render even iftodohasn't changed, and it's the aspect of React getting involved _at all_ that's ultimately a perf issue.Now, we ought to try building these options out, and then updating the benchmarks suite to have some hook scenarios for comparison with our existing HOC scenarios. (No idea how _that_ will work out).
I found an interesting statement in the official react dooks regarding useReducer having a similar behaviour. Let me quote it here:
If you return the same value from a Reducer Hook as the current state, React will bail out without rendering the children or firing effects. (React uses the Object.is comparison algorithm.)
Note that React may still need to render that specific component again before bailing out. That shouldn’t be a concern because React won’t unnecessarily go “deeper” into the tree. If you’re doing expensive calculations while rendering, you can optimize them with useMemo.
So they seem to think that having that single component re-render without any children also re-rendering shouldn't be too much of an issue.
That said, I completely agree that both API designs I posted are ugly and should be prevented if possible.
@t83714 if you look at the example @markerikson gives for the zombie child problem, you will find a pattern that is likely to occur in real applications (otherwise, how would they have noticed the zombie child problem in the first place?).
Sorry for the spam, but just for completeness' sake, here my proposed hooks implementation that requires neither a HOC nor use of useMemo and does not suffer from zombie children, but has the downside of calling the state selector with inconsistent props and also calling it on each render and twice for each relevant store change. You can see it in action here with the zombie child example from @markerikson. Of course it is not fully performance optimized yet and I am sure I am missing some edge cases, but this should demonstrate the idea of just ignoring errors during the subscription callback.
import { ReactReduxContext } from "react-redux";
import { useContext, useReducer, useRef, useEffect } from "react";
export function useRedux(selector, deps) {
const { store } = useContext(ReactReduxContext);
const [_, forceRender] = useReducer(s => s + 1, 0);
const latestSelector = useRef(selector);
const selectedState = selector(store.getState());
const latestSelectedState = useRef(selectedState);
useEffect(() => {
latestSelector.current = selector;
}, deps);
useEffect(
() => {
function checkForUpdates() {
const storeState = store.getState();
try {
const newSelectedState = latestSelector.current(storeState);
if (newSelectedState === latestSelectedState.current) {
return;
}
latestSelectedState.current = newSelectedState;
} catch {
// we ignore all errors here, since when the component
// is re-rendered, the selector is called again, and
// will throw again, if neither props nor store state
// changed
}
forceRender();
}
checkForUpdates();
return store.subscribe(checkForUpdates);
},
[store]
);
return selectedState;
}
@MrWolfZ
Thanks for the example link. I used a much simple array based sample app to reproduce the problem 😄
I think you're right. As long as we set up separate subscriptions for components, it's unavoidable unless we use batchUpdates or custom subscription logic to enforce top-down updates 👍
useStoreSelector((state, [a, b]) => selectSome(state, a + b), [a, b]).How critical is it to be able to use component props as part of your state extraction logic?
Here are my two cents on this ^ discussion point:
@markerikson said
However, the Todo component itself will still have to render even if todo hasn't changed, and it's the aspect of React getting involved at all that's ultimately a perf issue.
I agree with this. If you have 100 or so connected components in a list and all 100 of those update, that's still 100 updates regardless if those updates are fast or not. I acknowledge that you can useMemo to speed up all 100 of those updates… but it's still 100 updates.
For example, if you have 100 <Todo />s in a list and you allow the user to edit the title of the todo using an <input />, then you will _feel_ the 100 updates with every keystroke. I've been in that situation before and I've learned my lesson. From my experience regarding components, it's never a good idea to turn an O(1) problem into an O(n) problem if you don't have to.
However, @gaearon in this facebook incubator issue has also gave the notion of an API that doesn't take in props.
Here is a full quote of his post:
To be clear I'm not suggesting this particular API. Just that the notion of running a selector that involves props to determine a bailout has a lot of pitfalls (such as handling stale props), and can cause other issues in concurrent mode (such as the current code which mutates refs during rendering). It doesn't really map to the React conceptual model nicely. From React's point of view, props don't really exist until you render — but this Hook is trying to "bail out" at arbitrary times outside rendering.
One alternative design could be to allow selectors but only static ones:
import {createSelector} from 'react-redux-hook' const useTodosLength = createSelector(state => state.allTodos.length) function Todos() { const todosLength = useTodosLength() return <h1>Total: {todosLength}</h1> }If you need to select based on a prop value, you can do this with a component layer:
```js
import {createSelector} from 'react-redux-hook'const useTodos = createSelector(state => state.allTodos)
function Todos() {
const todos = useTodos()
return todos.map(todo =>)
}const Todo = React.memo(() => {
// ...
})Essentially, in this case the selecting component is your mapStateToProps. :-)
His post echos what @t83714 had said previously just lifting the selector out of the actually hook.
From an API standpoint, the static creatorSelector makes sense if you don't want to include props but in general, I still think using props to select a specific piece of state is necessary to make things scale.
BUT theory is theory. Until we build it out and benchmark it, there's no way to know. Maybe something like createSelector is the way to go? I'm skeptical but always open to different ideas (e.g. I'm still thinking about babeling that HOC in).
I finally have some numbers for y'all. I implemented two alternatives for useRedux which you can see here. Alternative 1 is my proposed implementation from above with some slight adjustments. Alternative 2 is the approach of a static selector and useMemo.
To benchmark this I have created a copy of the deeptree benchmark for each alternative and made some minor adjustments for each alternative. For alternative 1 I replaced ConnectedCounter with this:
const Counter = ({ idx }) => {
const value = useReduxAlternative1(s => s.counters[idx], [idx])
return <div>Value: {value}</div>
};
For alternative 2 I replaced ConnectedCounter with this:
const selector = s => s.counters;
const Counter = ({ idx }) => {
const counters = useReduxAlternative2(selector)
const value = counters[idx]
return useMemo(() => <div>Value: {value}</div>, [value])
};
Below you can find the results. Assuming I have correctly implemented everything, alternative 1 is on par with 7.0.0-beta.0. However, alternative 2 is massively slower (much slower than I expected).
@markerikson Which of the benchmarks do you think would be best suited to test the hooks implementations? I don't really want to re-implement all of them for all alternatives, so it would be great to know which your preferred one is (e.g. because it reflects real world usage best).
Results for benchmark deeptree:
┌──────────────┬─────────┬──────────────┬───────────┬───────────┬──────────┬─────────────────────────────────────────────────┐
│ Version │ Avg FPS │ Render │ Scripting │ Rendering │ Painting │ FPS Values │
│ │ │ (Mount, Avg) │ │ │ │ │
├──────────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────┤
│ 5.1.1 │ 14.55 │ 109.9, 0.1 │ 6466.00 │ 8835.47 │ 3001.32 │ 14,15,14,15,14,15,14,15,14,15,14,15,14,15,14,14 │
├──────────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────┤
│ 7.0.0-beta.0 │ 24.11 │ 111.8, 0.8 │ 337.77 │ 13320.03 │ 4784.63 │ 24,25,24,23,24,25,24,24 │
└──────────────┴─────────┴──────────────┴───────────┴───────────┴──────────┴─────────────────────────────────────────────────┘
Results for benchmark deeptree-useReduxAlternative1:
┌─────────────┬─────────┬──────────────┬───────────┬───────────┬──────────┬──────────────────────────────────────────────┐
│ Version │ Avg FPS │ Render │ Scripting │ Rendering │ Painting │ FPS Values │
│ │ │ (Mount, Avg) │ │ │ │ │
├─────────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼──────────────────────────────────────────────┤
│ 7.0.0-hooks │ 24.05 │ 111.7, 0.7 │ 536.28 │ 13591.96 │ 4872.18 │ 23,22,23,24,23,24,23,22,24,25,24,23,24,25,25 │
└─────────────┴─────────┴──────────────┴───────────┴───────────┴──────────┴──────────────────────────────────────────────┘
Results for benchmark deeptree-useReduxAlternative2:
┌─────────────┬─────────┬──────────────┬───────────┬───────────┬──────────┬────────────┐
│ Version │ Avg FPS │ Render │ Scripting │ Rendering │ Painting │ FPS Values │
│ │ │ (Mount, Avg) │ │ │ │ │
├─────────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼────────────┤
│ 7.0.0-hooks │ 7.96 │ 104.6, 4.6 │ 15053.94 │ 4996.25 │ 1724.94 │ 8,7,8,8 │
└─────────────┴─────────┴──────────────┴───────────┴───────────┴──────────┴────────────┘
Update: got some great review feedback from @MrWolfZ - I'm definitely on the drawing board still. But really happy to have this issue known to me now. Shall feedback with any updates as and when.
Hi all, joining this discussion as @MrWolfZ informed me that my library would suffer the same "zombie child" issue as discussed here.
I've managed to get around this by lifting the redux store subscriptions for the hooks to the root context, which allows me to execute the mapStates in an order that is more likely to represent the component hierarchy.
PR is here: https://github.com/ctrlplusb/easy-peasy/pull/137
Demo: https://codesandbox.io/s/pm39mzn13m
I still need to do a lot more tinkering. But wanted to put this on your radar. Hoping to get some early feedback on whether this is a crazy idea or not. 👍
Here is a very preliminary proof of concept branch with the HOC + hooks and here is a forked demo of the zombie child with the connect hooks
Don't take the code too seriously, I didn't spend much time on it (or test it). The only important thing here is the idea. I think we all knew that something like this would work but here is something a little more concrete.
Here's what it's doing (or at least trying to do):
connectHooks, creates a "subscription layer" where each call to useRedux will subscribe to a "hooksManager" and then the hooksManager will propagate to each useRedux hook in the "layer"useRedux subscriber in the layer is batchedCheckout react-hooks-easy-redux by @dai-shi. It seems to me it dodges these issues entirely. The api in it is simply
const state = useReduxState();
No map state etc. It uses proxies to monitor what parts of the state is used in a given component and just forces rerender for the component if a monitored part of the state changes. So any state mapping you need to do you can do directly in the render. So in this case batchedUpdates should be enough to prevent issues with stale props?
At least it passes this test https://codesandbox.io/s/2p4kxnxj1j
Here's some more discussions at https://github.com/dai-shi/react-hooks-easy-redux/issues/10
Not 100% sure how performant this solution is.
Also obvious downside of it is the requirement for Proxies so IE11 support can be a challenge.
An even simpler API and it doesn't suffer from inconsistent props? What a gem.
Also obvious downside of it is the requirement for Proxies so IE11 support can be a challenge.
Unfortunately, I have to support IE 11 :( . Since we have the properties we want to proxy from the store, we can iterate through them and then use Object.defineProperty for the proxy mechanism instead of ES6 proxies.
Yeah, I think immer does it like that if there's no Proxy support https://github.com/mweststrate/immer#immer-on-older-javascript-environments
If I understand correctly another downside in react-hooks-easy-redux is that you cannot bail rendering if you only use derived state to produce output.
Ex.
function HasComments(props) {
const state = useReduxState();
const hasComments = state.posts[props.postId].comments.length > 0;
return hasComments ? "yes" : "no";
}
This component would always render when comment count changes. Not just when it goes from 0 to something else.
@epeli not with the fork of redux i'm working on where selectors are created at store creation time: createStore(reducer, selectors, etc).
See:
@epeli I should note that react-hooks-easy-redux also has issues with Concurrent Mode, since it uses side effects in render (by attaching side effects to property accesses).
react-hooks-easy-redux also has issues with Concurrent Mode, since it uses side effects in render
It should work in Concurrnt Mode. It only updates refs in commit phase.
@dai-shi I really like the elegance of the proxy approach. Here are my two cents on the pros and cons:
pros:
useReduxState call per component while with alternative approaches people may use multiple hook invocations which will in turn create multiple subscriptions etc.cons:
Object.keys(state.items))ref.current inside effects but e.g. trapped.reset() etc. during render mutate the objects inside those references); that said, while this means it is not strictly pure, all mutations your library currently performs are "predictable" and reproducible, so the render still has the pure property of same inputs = same outputsOne way to solve the intermediary value accesses without needing a redux fork or similar is to still use an API which passes the selector to the hook, but then uses proxies to trace property accesses inside the selector to determine equality. This would alleviate the stale props issue since something like items[id].prop will not throw an error if the item with id does not exist anymore.
Final note regarding concurrent mode: As @markerikson noted in a source code comment, I believe as well that any solution which accesses store.getState() during render will have issues with concurrent mode, which includes your library.
@MrWolfZ Thanks for your attention to the library and your detailed pros/cons!
I will refer your comment in my issues for further discussion.
@MrWolfZ
it makes render impure since through the proxy
That's not so much unlike the react-redux hoc which is impure as a parent component doing the work. There's gonna be impurity somewhere until React builds a global state store into React itself (and passes it down to all as a 2nd argument to all components), and it's very clear they have gone the opposite direction of Redux (probably because its lack of modularity).
One way to solve the intermediary value accesses without needing a redux fork or similar is to still use an API which passes the selector to the hook, but then uses proxies to trace property accesses inside the selector to determine equality
@epeli was saying that the closure is the real problem. So if he's correct and if the selectors weren't defined in the component function, or at least didn't enclose any variables, and rather the variables were passed separately, I think you're right, and selectors could work without a fork! Eg:
const hasComments = (state, postId) => {
return state.posts[postId].comments.length > 0
}
const MyComponent = props => {
const deps = [props.postId]
const state = useReduxState(hasComments, deps)
return <div>{hasComments ? 'yes' : 'no'</div>
}
That should work, as there is no difference between passing hasComments there or on store creation.
lot of good brainstorming happening around @dai-shi's library! (and not to mention @theKashey's lib: proxyequal which is the basis for all this!!
Lately, I'm designing an experimental selector based hook in my project https://github.com/dai-shi/reactive-react-redux (the project name changed.) I ended up with developing something similar to what @MrWolfZ proposed in https://github.com/reduxjs/react-redux/issues/1179#issuecomment-476105825 with Proxy.
If Proxy-based approach is still immature for react-redux v7, and I suppose so, @MrWolfZ 's hook would be good for the official react-redux hook as it keeps the same mental model. Just my two cents.
Yeah, unfortunately, I don't think I can justify using anything that relies on Proxies completely atm. I ran a poll the other day, and it looks like 50% of React-Redux users are still targeting ES5 environments: https://twitter.com/acemarke/status/1111050066532265984 .
Not all proxy features are needed. And what is needed can be addressed under the hood (aka polyfilled), so that IE11 can work with no different work required of the end user. Proxy support isn’t in fact a barrier.
To be clear, not all of proxy features can be pollyfilled. But all that’s needed for this solution can be polyfilled.
https://github.com/GoogleChrome/proxy-polyfill - only ownKeys trap is not supported
@faceyspacey @theKashey there is one edge case where the polyfill doesn't work: if you have an optional property on the root state and add that property later on it will not notice this new property. Here is a reproduction. In this example, if you click "Add property", it will not cause Child to re-render.
However, this issue is very unlikely to surface in a real application, since it only happens for optional properties on the root state. For more nested properties it will simply be less performant, since it will stop the access detection chain at the parent of the optional property and then re-render Child whenever the parent changes. Here is an example showing this. In this example, the render count will not increase when clicking "Add property 2" without the polyfill but will increase with the polyfill.
(You can force the polyfill to be used even in modern browsers by just adding a <script>delete Proxy</script> to public/index.html's <head/>.)
@Jessidhia thank you for the hint. I tried deleting the global proxy object inside index.js but couldn't get it to work properly. With your suggestion it works perfectly, so I adjusted the sandboxes.
@MrWolfZ - the example is awesome - short and sound. And I have no idea why it's not working.
So - if you are accessing some prop, which does not exist - the access would not be recorded, thus the changes in that prop would be ignored.
That might be a big issue, actually, @ctrlplusb already created the same issue, but with another way to reproduce. I am afraid, but any change in object keys should be considered as a major update. And, huh, that would cause some performance implications.
@MrWolfZ : "optional properties on the root state" sounds _exactly_ like a typical "dynamically loaded code-split reducers" scenario.
I don't see any technical way to solve this case without proxies, but there is a non-technical way.
replaceReducer later, or ask to use Object.keys and _mark_ a selector as dependent o keys, and once the got changed - it would be recalculated.Honestly - the first variant is more acceptable.
I've spent the last few hours doing research. I've re-read #1063 and this issue, put together a list of existing unofficial Redux hooks libs, looked at some of the code snippets pasted in these threads, and glanced through the source of reactive-react-redux and proxyequal.
I don't have any grand insights or solutions atm - I'm still trying to absorb a lot of info.
My immediate random thoughts, in no particular order:
The best of the existing unofficial hooks libs is easily https://github.com/facebookincubator/redux-react-hook , both in terms of actual popularity and how much thought has gone into it. Runners-up:
Pros and cons of the most promising approaches I've seen so far for hooks implementations that try to handle the "stale props / zombie child" issues:
connectreactive-react-redux lib:Interestingly, the "Zombie Child" demo I'd created that fails in v7-alpha.5 actually appears to work okay with v7-beta.1. Not sure what specifically changed to make that happen.
connect() itself would serve that purpose, particularly with the v7 implementation. Right now, I'm pretty sure that connect() overrides the Subscription in context even if it doesn't subscribe to the store itself. (Which, come to think of it, might not even be the behavior we want?)So here's a hypothetical suggested path towards actually building this:
useSelect(): subscribe-onlyuseActions(): action creators onlyuseDispatch(): just dispatchuseStore(): get the store if you really need ituseRedux(): roughly equivalent to connect() nowuseTrackedSelect() or something like that that would be based on the work from reactive-react-redux.connect() and even useSelect() in an ES5-only if you want. proxyequal is a few K by itself. I wouldn't want to force folks to pull that in unless they're using useTrackedSelect(). Would need to figure out how to get that set up well for tree-shaking.Thoughts?
Yeah. I have one _Thought_, not related to this topic, but better be answered here.
In short - __I have no idea how and why people are ok with redux__. Unless you are an experienced developer with quite a _special mindset_, it's quite hard to create a proper boilerplate around __selection__ from state and derived data creating in mapStateToProps.
To be clear - the problem is not with redux, the problem is with memoization it requires to live happily ever after without cascade updates.
Reselect is fragile, and not component-architecture compatible by design. re-reselect had never been an option for me. React.useMemo could be a great solution, but it has to live within React render cycle, which affects API.
I've tried my best - kashe as a solution for derived data generation/memoization and memoize-state (proxyequal) to remove than _engeneering_ aspect from memoization.
So - my thoughts are simple - we need not a _hook-based_ API, but an API there is would be or __harder to make a mistake__, or __easier to write more sound code__.
99% redux customers are doing it wrong, not knowing/checking what they are doing, or where/why memoization fails. Every time I convince someone to install why-did-you-update-redux and actually test "quality" of their "redux" - it's 🤯. Using custom equality check to perform deep comparison in reselect or areMergedPropsEqual - easy. Not using any memoization at all - even easier. Babel optional chaining (returning a new "empty" object every time) - a new trend.
A generic _hook_ solution, like redux-react-hook is good, and looks quite similar to the "old" redux, but useMappedState like a _forcing_ you to continue use hooks, especially useMemo, inside mapStateToProps, while you cant. It's +10 WTF per minute.
What if _first__ try to __create an addition to react-dev-tools__(aka reselect-tools), to help you build/visualize selectors according to your factual state and factual usage (dev tools could track both), and __then build _hooks_ API__ in a way to easier utilize results of such tool.
PS: your concerns about proxyequal size are not quite correct. It's a low-level library which does not handle edge cases. Memoize-state, which build on top of it to handle those cases is twice bigger.
@theKashey amen, preach!
Basically, the only way the bell curve of developers (according to skill level) will ever be able to build properly rendering apps is through a proxy-based connectless future. Having to configure connect is also an absolute waste of developer time, given there is a path forward where we don't need it. connect will inevitably die, and the sooner the better.
I've been talkin about this for 2 years. It's time to level up the Redux world. I personally have chosen to focus on other things, FYI, because this is inevitable. It's like how Marc Andreesen talks about industries like the music industry (and now many others) inevitably being toppled: "technology is like water; it inevitably fills all the gaps and there's nothing we can do to stop it; we're better off supporting the natural direction of growth." That's not the exact quote, but you get the idea: we're best off facilitating a proxy-based connectless future.
So the most important thing the Redux ecosystem needs isn't a "hooks" API. HOCs still work beautifully. What needs to be determined is if hooks truly provide much value here (which they don't) or if we're just doing things because the React team opened up a new shiney API.
The thing is this:
mapStatesToProps is needed)If we want to "be like water" (this time in the Bruce Lee way), and hit a few birds with one stone, react-redux 8 should be hooks + proxies. 7 remains for everyone that can't support proxies yet.
Lastly, there are workarounds for supporting proxies. Namely what MobX always did where keys must be known upfront, as @theKashey mentioned. So in 8.5, perhaps that's introduced so even IE11 can make use of the transparent proxy-based approach. @theKashey has a few ideas how to handle this. But obsessing over it is not the path of the people that actually get this done. Build it first without supporting the non-proxy approach, and then circle back, like the Vue team is doing with Vue 3:
yes, out the gate, Vue 3 will not support IE11 and browsers that dont support Proxies.
In conclusion, the best use of available energies is toward helping @dai-shi and @theKashey optimize their proxy-based approach in their 2 libs:
They are doing something truly groundbreaking. I usually don't poke my head out in these threads--so the whole purpose of my post here is to make it known how important their work is. The other hooks approaches are all following traditional approaches. Where's the revolutionary spirit? That's the whole point of software after all.
There's some remaining perf issues, but it's clearly achievable. What those 2 have done has proved a lot already. The perf for a great number of cases is comparable to react-redux 7. The one that's sub-par is a contrived case. It needs to be addressed regardless. The point is: we're close, and would be closer if more from the community got serious about this approach, rather than treating it like some far off thing. Of course current react-redux users are tied to IE11. But it's a totally different story for apps starting today. Not offering a futuristic approach, and not making that the main focus, will only mean Redux falls more and more out of favor. Bold action is required. Long live Redux!
@theKashey , @faceyspacey : fwiw, I definitely appreciate the work you and @dai-shi are doing investigating various Proxy-based approaches, especially given that I don't have time to try any of that myself.
That said, given React-Redux's "official" status, and the fact that it's one of the single most widely used libraries in the React ecosystem, we have to carefully balance a lot of different concerns: API design, browser compatibility, familiarity, teachability, community usages, versioning, and more. That means we _do_ have to move a bit slowly and favor a conservative approach overall.
There's a very clear interest from the community in having us add some kind of hooks-based API, hence this issue. connect() is extremely well known, and there's a straight path from an API like mapState to useSelect() or similar. Adding something like useSelect() now doesn't preclude us from adding more APIs down the road.
But, anything we add now _does_ have to continue to work and be supported going forwards. I refuse to put out new public APIs without ensuring they're tested and rock-solid, and I'm not going to force our users to keep bumping major versions (or even worse, break their code) because we did something bad. I feel bad enough about the v6 -> v7 change as it is - I'm the one who pushed for the v6 context-based state propagation. Granted, at the time it really did look like the way to go, but the churn here is basically my fault for choosing an approach that didn't pan out.
I agree that IE11 is (slowly) on the way out, and that use of Proxies is only going to increase over time. I think there's some great possibilities around using them, particularly for React-Redux. But, I'm not ready to jump that chasm yet.
I'd encourage you to continue your experiments and to work out the rough edges around these approaches. I've already been keeping an eye on what y'all have been doing, and I'm excited to see how it all turns out. When the time comes to try pulling some of those ideas into React-Redux itself, I will gladly learn from what you've done, give all of you full credit for any inspiration we take, and ask you to help contribute if possible.
Until then, the most straightforward path to getting something out the door is to build on the hooks-based code we've got in v7 right now, and provide hooks APIs that closely resemble use of connect().
Mark, I updated my post, make sure to check out what Vue 3 is doing where they wont initially support proxies:
https://medium.com/the-vue-point/plans-for-the-next-iteration-of-vue-js-777ffea6fabf
I wouldn't be closed off to a react-redux going a similar route sooner than it seems you're currently considering. Vue after all is the whole framework, whereas react-redux is just one choice of many libs you can use with React.
There's 3 ways react-redux could get serious about it sooner:
Basically it would be nice if react-redux got serious about supporting this endeavor sooner than later. Right now @dai-shi and @theKashey need a lot more eyeballs than yours and mine.
Everyone else, HELP WANTED HERE:
We need some _established_ pattern for redux, and right now it does exist. Then we could try to create an eslint plugin, like react-team created for hooks(really - that's the deal breaker) and mitigate the majority "👎"s.
So - let's focus not on the implementation details, but on the sound API, it would be possible to name as a _pattern_ and create an infrastructure around.
My personal opinion - I love to separate concerns as much as possible (ie Smart/Dump) to make tests/storybooks easier. Right now all _hooks_ realization are tightly coupled, they just form one big component, and does not answer this call.
So - __Hooks vs Tests: the engage__.
So the most important thing the Redux ecosystem needs isn't a "hooks" API. HOCs still work beautifully. What needs to be determined is if hooks truly provide much value here (which they don't) or if we're just doing things because the React team opened up a new shiney API.
@faceyspacey I don't necessarily agree with this. I care a lot about React hooks and a redux API because it simplifies composing together and reusing reactive dependency injectors (i.e. things that provide data + have the ability to case a re-render). Historically, I've been composing reactive dependency injectors via HOCs and that made the rest of my team scratch their heads e.g.
const Container = compose(
withRouter,// get the `location` from react-router
connect(/* ... */), // use the location + redux state to derive props
lifecycle({
componentDidUpdate() {
// watch for changes in those props and refetch if necessary
}
}),
)(PresentationalComponent);
export default Container;
with hooks, composing reactive dependency injectors looks much better/is more readable and useEffect is particular helpful
export default function Container() {
const { location } = useRouter();
const exampleState = useRedux(/* ... */);
const calculatedProp = useMemo(() => {
return // ...
}, [location, exampleState]);
useEffect(() => {
// refetch data or similar
}, [calculatedProp]);
return <PresentationalComponent exampleProp={calculatedProp} />;
}
I think the above is a very "react" way of addressing the concerns @theKashey has with redux memoization. Don't get me wrong, @theKashey is 100% correct with the pains of memoization but hooks with useMemo might also be a good way to deal with it??
It's not fool proof but at least better than the current state of composing HOCs.
I like @markerikson proposed possible path forward simply because it gives me hooks and it's great incremental approach to improved patterns.
hooks with useMemo might also be a good way to deal with it??
That's a problem! __They could not__! 99% redux-hook implementation uses _subscription_ mechanics to react to store changes and skip unnecessary updates, so your useMemo would be called by redux __out of React rendering cycle__. "WTF!", you can't use hooks in redux-hooks API, which is a bit controversial.
@ricokahler I agree managing HOC order with recompose was horrible, and being able to achieve the same within the function body is better. However the point is not having to mapStateToProps reduces developer time FAR MORE than a hooks API.
More importantly, since a proxy-based approach doesn't require you to pass props to your mapStateToProps function, order no longer matters. Eg. you don't need to put effort into insuring router state comes before it. In a vision I imagine that makes sense for this library, you only pass args to selectors where they're used:
const { state, dispatch, selector } = useRedux(pureSelector);
<div>selector(props.arg)</div> // memoized selector called here
Ideally though, Redux too is upgraded, and you provide all selectors at store creation:
const store = createStore(reducer, selectors, etc)
so you can do this:
const { state, selectors, dispatch } = useRedux();
<div>selectors.foo(props.arg)</div>
So again, we can hit a few birds with one stone by fully buying into the proxy-based approach. A lot of doors will open. Again, the entire Vue platform bit the bullet and made this decision. Innovation happens at the edge, and the edge is proxies for the Redux community. Obsessing over hooks in the Redux world is just being average, and doing what everyone else is doing, which is following the React team down whatever alley they drag us down. meh. It's no big deal. It will be done. Let's do something great though.
If you're using redux-first-router you're hardly going to waste your time with hooks, certainly not for fetching application data. It's far less code and far faster to do it at the route level, where you don't even need the modularity/composability of components. Linear orchestration of effects trumps random discovery of effects in the component tree for so many reasons. In summary, one is surprises and the other is predictable.
Data fetching is 99% of the time specific to your application, and doesn't benefit much by composeable patterns. Such composability makes the most sense for other things like DOM effects, e.g. scrolling listeners. For example, so you can make a reusable component that uses scroll event listeners. That's a good use case for hooks, and the component primitive in general. Fetching has made far more sense attached to a route primitive (at the top level of your app when you transition to a URL).
And with hooks it does more than ever, given the React team has such promotional pull, and given that the result is that they're ushering generations of developers into a smaller picture approach. If you take a bigger approach than what React is able to consider, other primitives--routes--help you develop on the order of 3-5x faster. Yes, routes. If you have been using Redux without redux-first-router, you've likely been developing far slower than you could. And ultimately, you even could build a React Native app where you dont do deep links and therefore have no URLs, and the conventions enforced by RFR will still increase developer productivity several fold...I wish I had more time in the past 2 years to promote that approach, but I chose to work on something new even better.
It's too bad even the maintainers of this repo have no clue of the enormous benefits of the Redux-First Router approach. Yup, that's you, Mark. Being the lead maintainer of Redux, you really should be more aware of the best ways to Redux. I know your app at work doesn't do "routes"--well, that's a glaring hole in your Redux wisdom. In the way RFR results in 1-2 dispatches per route transition, rather than tons of dispatches, it pulls developers down the "pit of success" in a way no other libs ever did. Don't get me started on sagas. It's called "thin/few actions, fat/smart reducers." I can't tell you how many apps I've seen where developers are using the store as a giant mutable store, dispatching dozens of actions per a single user-triggered event. No amount of messaging and communication can change that. Better tools have been needed in addition to plain react-redux. RFR automatically helps the bell curve of developers avoid multiple dispatches, makes SSR trivial, circumvents the pitfalls of dispatching actions in lifecycle handlers, and so much more. It's a library the community would have been far better off had you taken an interest in.
There I said it, man. I appreciate the links you (or someone) added to the Redux docs. What's really been needed the past few years has been for the most vocal Redux people to pump up some additional libraries and patterns, instead of just obsessing over this one damn library. Everybody's developing like it's 1984 with this connect strategy, and based on what I'm hearing will continue to do so even with hooks! If you're stuck in the past, and generally go so slow, and dont look to the future, and don't even signal winners of primary extension categories such as routing, your precious library is going to become a thing of the past. Just watch.
ps. I'm in love with neither the hooks approach nor the hoc approach (nor even my idea above). A redux-like store built-in to React is the way we should be developing, rather than all this jumping through hoops. The patterns of the ecosystem have solidified; we've learned our lessons through the ability to try things out that low level modularity gives us; it's time React gets its Rails. Only if someone would build it :)
And it's absolutely not NextJS, as NextJS does very little, and walls you into a garden in the exact area of the stack where pro apps want full customizability
Awright, let's keep things on track here, folks.
This isn't a thread to discuss pros and cons of React's addition of hooks, alternate libs, and various other hypotheticals.
I _do_ intend to add a hooks-based API to React-Redux, and the goal here is to nail down the immediate approach for doing so.
If you've got other topics to discuss, please open up separate issues.
react-redux will be obsolete very soon with that attitude, Mark.
Anyway - redux was always very opinionated library with strong reasons to work that way. Some decisions are hard to understand for the first time, but later you could only agree with them.
So - let's make it a bit more official - we need a comparison table!
| Thing | Redux v5 | Redux v7 | redux-react-hook | context API |
| ------------- | ------------- | ------------- | ------------- | ------------- |
| separation of concerts | 5 | 5 | 0 | 1 |
| testing | 5 (due to separation) | 5 (due to separation) | 2 (just need a new pattern) | 2 |
| speed | 4 | 5 | ? | 3 |
| easy of use | 3 | 3 | 4 | 5 |
@markerikson - I hope you got an idea, and I hope that as long as you have written a dozen articles about decisions made - you have a list of "why redux", with some important points we could track in the table.
Probably I fair comparison would reveal a true way we shall go, cos, you know, that should be one more tough, but a right decision to make.
@theKashey : I'm not sure what point you're trying to make here, and what it has to do with nailing down a hooks API for React-Redux, which is the focus of this particular thread.
I already had to lock #1063 for getting too far off-topic. I'd really rather not do that again here, but we're drifting rather noticeably and I don't like that.
As I said already: if you've got other topics to discuss that are not directly related to designing and implementing the hooks API, please file a separate issue for those and we can discuss there.
Here's a collection of my thoughts on various things posted in this issue recently.
@markerikson regarding this:
@MrWolfZ : "optional properties on the root state" sounds exactly like a typical "dynamically loaded code-split reducers" scenario.
I had the same thought (I even had something in my post about it, but edited it out afterwards). While this is indeed exactly the scenario for lazy loaded reducers, it would still be very unlikely to occur since it would require a component to access the lazy loaded slice before the slice was loaded. In usual use cases I believe only the components that are loaded together with the slice will access it. But yes, something like this would not work with the polyfill:
function MyEagerComponent() {
const { lazySlice } = useReduxState()
// check if the slice has been loaded
if (!lazySlice) {
return null
}
return <div>...</div>
}
I agree with @theKashey that it may be possible to detect these cases during development and print a warning and/or just add a statement to the docs that if IE11 needs to be supported then stubs for all lazy slices need to be added to the store when being initialized.
I have updated my alternative 1 to fix one race condition and also get rid of the deps parameter, since it wasn't required. This makes for a really simple API without the need to remember to provide correct deps or use a linting plugin for this. Yes, swallowing the error feels icky, but I still believe an approach where the store subscription is only responsible for _heuristically_ determining when to re-render is best, since it is anyways almost impossible to be perfect with the change detection (even with proxies multiple cases have been pointed out that cause issues, e.g. simple things like Object.keys(state.items) will cause many false positives that in the worst case can lead to wasted re-renders of whole subtrees).
I also think @ctrlplusb's "window of opportunity" is not really required, since it doesn't really change anything. Why delay re-rendering the component to throw the error? It will make debugging even harder. The only case in which the error is not thrown is if another render without an error happens. Why not force this re-render immediately?
Lastly, it would also be possible to log a (suppressable) warning during dev mode instead of completely swallowing the error.
@markerikson regarding this:
If we somehow had a way to track subscriptions in correct nested order, without relying on overriding a context value, I think that would resolve most of the issues we're discussing around stale props and stuff. That said, I truly don't know if that's even possible, given that we don't know where a given component is in the tree when it tries to subscribe.
I did some experiments on this some time ago, and it is possible in a very limited way, but I abandoned it since I couldn't figure out how to make it fully work. I'll still post my thoughts here, since maybe someone will have an insight on how this may work out after all.
So, there is an observation we can use to infer the relative component tree shape of redux-aware components. This observation is that the order of renders being top-down and then the following effect invocations being bottom-up(-ish) taken together allow to infer the tree.
Let's illustrate this with an example. Take this component tree:
<Comp id={1}>
<Comp id={2}>
<Comp id={3} />
<Comp id={4} />
</Comp>
<Comp id={5}>
<Comp id={6} />
<Comp id={7} />
</Comp>
</Comp>
With this we get the following order of renders and effects:
Based on this we can infer the tree by observing that for each component the first subsequent effect with a smaller id comes from the parent component:
Here's a sandbox if you want to play around with the example yourself.
Now the issues of this approach:
Also, this is only a thought experiment and I have no idea how the code to properly track these relationships would look like.
I would like to take a step back regarding the API design and discuss a fundamental issue: is all of this work really required? As @markerikson mentioned before, it would have been great if the v6 context approach worked out. My provocative question is this: has it not? I acknowledge, that there are serious performance issues and that real apps have suffered from those. But those cases are probably the vast minority of users of the lib (sadly it is almost impossible to get accurate numbers for this, since any poll etc. will suffer from the same response bias you get when basing your decisions on github issues, since only very few people create/actively participate in these).
There are some horribly inefficient (and buggy) react-redux derivatives out there that still get 1000+ stars, because most users of those libs never hit these issues.
So, why not take the hooks API as a chance for a somewhat new beginning and go with the v6 and v7 approach in parellel? For hooks use the v6 approach and make the API const state = useReduxState() while keeping the v7 approach for HOCs. Then just mention in the docs that people should use hooks as long as possible and switch to HOCs for the performance critical parts of their application. I think react users are already trained to do this since it is basically exactly the React.memo approach (which is of course easier to apply than having to switch a component from hooks to a HOC, but the idea is still the same). This API design would also allow a simple switch to proxies once IE finally vanishes (or just already go with proxies now and instead of the polyfill just fall back to triggering re-renders for each state change).
Sorry for yet another wall of text, but as we all know, this issue is quite complex.
@MrWolfZ : yeah, your component tree example is what I was trying to suggest as a hypothetical alternative to nested subscriptions. Where it gets _really_ bad, and seemingly impossible to track, is when you start dynamically adding and removing components in the middle of that.
There's a couple existing issues in the React repo asking them to expose their internal keypath values, but I don't see that ever happening.
Context propagation problems are one of the two main reasons why we're abandoning the v6 approach in the first place. Not only is it relatively inefficient, but you can't bail out of context-caused updates from within a hook. See all the writeup at the start of #1177 for the details. Also, no, I'm not going to try to implement multiple approaches in parallel in one version.
(Also, what particular "React-Redux derivative" are you referring to?)
OK, finally throwing my hat in the ring here!
So, my biggest problem with the Proxy based approach is speed. They are roughly 20-30x slower for simple property access, primarily because they can't be JIT-ed to hidden classes by the VM. I actually don't really care about the "magic" nature of it; in fact, that's really cool. But I do worry about it putting us behind where we were with 6.0.
One of my absolute _favorite_ Hook libraries so far is react-spring. One thing they have to do is make you render a special component for the useSpring hook:
import { useSpring, animated } from 'react-spring'
function App() {
const props = useSpring({ opacity: 1, from: { opacity: 0 } })
return <animated.div style={props}>I will fade in</animated.div>
}
It's a unique requirement of their API, but it's never been a problem. You can even use it with styled-components. What if we adopted that approach to get around the need for a HoC wrapper on any Redux Hook-consuming component?
import { useRedux } from 'react-redux'
export default function App() {
const { state, actions, Connect } = useRedux(
state => ({
items: state.items
}),
{ addNewItem }
)
const { addNewItem } = actions
const { items } = state
return (
<Connect>
{items.map(item => (
<Item key={item.id} item={item} />
))}
<hr />
<button onClick={() => addNewItem()}>Add New Item</button>
</Connect>
)
}
That Connect component would be able to Provider a new parent store to override the subscription system and let it form a proper subscription tree.
I believe that would solve the issue at hand. I don't think asking folks to include an extra component in the tree would be a huge issue. We could also provide the HoC form to give users an alternative.
It’s easy to forget to add, and hard to enforce this requirement. It shall be less fragile and makes a sense. Still possible to track Connect creation, and inform where it was skipped, use eslint, and so on. I am not sure that would be a good experience.
The hooks approach is: 24 lines long, harder testing, requires to keep Connect in mind. It's easier to make a mistake a harder to find.
While the old approach is: 18 lines long, easy testing, ---. It's harder to make a mistake and easier to find.
import { useRedux } from 'react-redux'
export default function App({items, addNewItem) {
return (
<>
{items.map(item => (
<Item key={item.id} item={item} />
))}
<hr />
<button onClick={() => addNewItem()}>Add New Item</button>
</>
)
}
connect(
state => ({
items: state.items
}),
{ addNewItem }
)(App)
So, let's change hooks API just a bit. Let's get a ref from useRedux and pass it into Connect, to actually connect to the store. Now the usage of Connect is enforced - without Connect it would just not work.
export default function App() {
const { state, actions, ref } = useRedux( // get ref
)
return (
<Connect to={ref}> // pass ref into `Connect`
)
}
This one, actually, is even worse, as long as "connecting" would take a place after the current component got rendered, with all values set to undefined, and any way to "fix" it would make it more and more complex.
So - what if keep hooks API simple to use, might be even without _proper_ memoization (like ContextAPI), with actions made asynchronous to break strange behaviours, and provide another API("the old one?") for critical or complicated situations, or just for "old" code as @MrWolfZ proposed? Two APIs are not a bad idea.
I'm reaffirm that the fact that you must use a specialized component with react-spring has never been an issue in my usage. It also wouldn't be strictly necessary to use, just if there are connected components in the subtree under this one. Lastly, I'd say a LoC comparison for a simplified example isn't really important, as most real-world components are more complex and the percentage increase would be minimal.
That being said, the testing issue is most certainly something to think through. I know we have enough blowback from removing the connect() store prop, but this might just be a function of how Hooks are normally tested by end-users.
Remember that the subscription nesting issue is only critical for cases where you need to guarantee that potentially changeable wrapper props are always up to date for use with mapState. If you don't need the equivalent of ownProps in your mapState, or the props you do need aren't changing, then it's irrelevant - the component is fine even if it subscribes to the root of the subscription tree directly.
A couple other thoughts:
useSelect(selector) no longer _has_ to return an object, because that isn't being turned into props. So, in Tim's example, it can be useRedux(state => state.items)addTodo has been imported at the top of the file, and is passed to connect in the outermost scope at the bottom. With a hook, though, you'd be doing const {addTodo} = useActions({addTodo}) inside the function, which is really weird now that I look at it.I'm not sure where the mentions of testing are coming from. Did I miss part of this discussion somehow?
Also, @theKashey : can you clarify your last paragraph? I don't think I understand any of what you're trying to propose with this:
So - what if keep hooks API simple to use, might be even without proper memoization (like ContextAPI), with actions made asynchronous to break strange behaviours, and provide another API("the old one?") for critical or complicated situations, or just for "old" code as MrWolfZ proposed? Two APIs are not a bad idea.
Sorry @markerikson, we are missing something here, and I don't clearly understand what. Right now we have 2 sets of APIs:
useRedux, which works somehow like the old redux. It's not bad, but it not better than an old approach.useSelect + useActions, you may use multiple times in one component, which could be more interesting. And look like you are looking into this variant.My intuition does not like the first one - it is not as _hooks shall be_. I'll better use _decorator_ to achieve the same result with almost the same syntax.
The second one, with scattered API, sounds more interesting, and much more _like hooks_, especially for selectors - useSelector is the same useMemo and could use the same eslint rule to get it working right.
but I still not clearly understand what I am looking for
let me not soil the discussion
The difference between useSelect and useMapState(redux-hooks) or useStore(easy-peasy) is crucial - a one selector per one command.
const {state, useSelector} = useRedux(); // get useSelector from useRedux to track the usage (need only dependencies)
const items = useSelector(() => state.items);
const visibleItems = useSelector(() => state.items.filter(i => i.visible), [state.items]);
In the same way - you have to expose state here, which removes all the sence of having selectors. Plus, as long as it's a closure variable, you cant do early-bail-out, and have to render all the stuff.
So it should be a way to not expose state, and still provide a way to use to specify selector. That lurkes us back to reselect-like API, which is not _hook_ compatible, so lets _invert_ it.
useReselector(state => state.items.filter(i => i.visible), [state => state.items]);
// is equal to
useReselector(filterVisible, [getItems]);
// is equal to
createSelector(getItems, filterVisible);
In this form, I hope, it would be a bit close to the hooks. Let me clarify my position - proper memoization and reflow optimization are real problems for me and a dozen projects around. I would like to have an API where it _could not_ be a problem. For example - you cant access store without selector.
Still:
mergeProps is not possible.Most of the time we need selectors and actions, so one hook that gives access to both could be interesting 🤔
The problem to allow multiple useSelect in the same component is that it will make the implementation more complex to avoid multiple subscription to the redux store...
But I agree, multiple usage of a simple hook rather than one big complicated call looks like to be more in _the hook's spirit_ for now.
I think it's important to keep the hooks flavor with array return value instead of object. The more I use it the more I like it, the ability to directly name variable on destructuring (and not rename it like when it cames from an object) is a really cool feature. And for newcomers to React and ES language, you will use react-redux hooks like you use useState from react.
const [dispatchAddTodo] = useActions({addTodo});
And it also solve the name clash issue for you, I mean the user has to solve it for you 😅
I'm fine with it more piecemeal. In fact, that better fits what a Hook should be. Here's how that might look:
import { useStoreState, useActions } from 'react-redux'
export default function App() {
const { state: items, Connect } = useStoreState(state => state.items)
const { addNewItem } = useActions({ addNewItem })
return (
<Connect>
{items.map(item => (
<Item key={item.id} item={item} />
))}
<hr />
<button onClick={() => addNewItem()}>Add New Item</button>
</Connect>
)
}
Being able to call multiple useReduxSelector hooks in one component is crucial, not because it's the hooks flavor, but because of what hooks where build for: sharing logic just like we share components. The ability to have multiple useState calls in a component allows to create complex behavior that involves state and side effect and hide and share it in a custom hook. The same would apply to redux state. The hooks design is optimized for custom hooks, because that is the real purpose behind it and what makes it powerful.
Yup, here's an example of that:
import { useItems, useCurrentUser } from './my-redux-hooks'
export default function App() {
const [items, { addNewItem }, Connect] = useItems()
const user = useCurrentUser()
return (
<Connect>
<Header>Welcome, {user.name}!</Header>
{items.map(item => (
<Item key={item.id} item={item} />
))}
<hr />
<button onClick={() => addNewItem()}>Add New Item</button>
</Connect>
)
}
(BTW, the array vs object destructuring choice is up to you!)
@timdorr hmm, that API is a bit interesting... if I follow correctly, the <Connect> here would be for "all children below this component, as opposed to the current/HOCconnect()` which renders a wrapper around/above the component in question? I mean, you could accomplish the same "division into layers" of the tree and guarantee the order of re-rendering either way, it's just an interesting change in perspective.
So if I understand correctly, calls to useStoreState would return Connect components, and if you useStoreState several times within your component (perhaps as part of custom hooks shared between several components), you're only really expected to use any one of the returned Connects to wrap the rendered subtree? (the library could even feed the same component to you in all cases, I think)
Would useCurrentUser in your example just ignore the returned Connect, and expect you to use one from another useStoreState call?
@timdorr Do you think it would be commonplace for custom hooks needing to pass down a Connect wrapper? I definitely wouldn't want to need to put 3-4 connect wrappers around the component.
If the serve the same purpose & we only need 1 do you get _exactly_ the same result if you used any of the given <Connect /> components?
Lastly, if this is something that covers an edge case (and rarely needs to be used as @markerikson is saying) maybe it could just be it's own hook, with it's own documentation.
export default function App() {
const [items, { addNewItem }] = useItems()
const user = useCurrentUser()
// In most cases you wouldn't need to do this?
const Connect = useConnect();
return (
<Connect>
<Header>Welcome, {user.name}!</Header>
{items.map(item => (
<Item key={item.id} item={item} />
))}
<hr />
<button onClick={() => addNewItem()}>Add New Item</button>
</Connect>
)
}
If it's a rare use case I would want it to be an explicit choice when it has to be used and probably give some inline comment around the why it would be need to be there.
@charrondev I think the point is that you'd want to use the <Connect>...</Connect> in each component, much like you connect() in each component, precisely because the bugs (stale props/zombie children issue mentioned earlier in the thread) are subtle and hard to debug otherwise!
So, whilst I agree with you that something about the API doesn't feel quite right, I think splitting it out risks people not using the <Connect> around their rendered result, and running into subtle bugs as a consequence of that. I'm not sure what the ideal API here is, but I think it'd want to encourage wrapping your rendered output in exactly one <Connect> when using the Redux hooks...
@charrondev Unfortunately, if you wanted to create a Connect externally from the useStoreState hook, it would need to know about the subscription created by the selector. The point of the component is to override subscribe on the store to ensure the right ordering of those subscriptions being fired and processed (top down), so it would need some way of connecting to the created subscription.
I guess in that respect, the all-in-one useRedux approach is probably best because it will only ever produce a single Connect. I agree it's not ideal, but there doesn't appear to be an ideal case for this library with how Hooks work 🤷♂️
Another thing could be to just defer to connect() when top-down data flow is a concern. I'm running a Twitter poll to see if that's an issue others are seeing a lot. Could we detect the problem somehow?
@FireyFly That makes sense.
I don't particularly like the idea of having to return through the connect from every custom hook if you only ever need one.
To me there's a lot of utility in making hooks for various resources, and most components are likely to use 2-3 of them. It would be easier to understand if there was just 1 that you had to use (eg. Something like useConnect()).
This would like also improve things if this could get to the point where the <Connect/>s aren't necessary.
Please correct me if I'm wrong, but this is a side effect of moving back to direct subscriptions for performance reasons, and not using React's own context mechanism (because we need to be able to bailout?). A future react update might make this unnecessary, and then the <Connect /> wrapper wouldn't be necessary. In this case the wrapper is something we have to live with for a time, but maybe shouldn't infect the primary API & all custom hooks using it.
@charrondev : yes, this is all a result of moving back to direct subscriptions. v6 didn't have the stale props / "zombie child" issue, because everything was handled as part of React's rendering process.
I think having multiple independent subscriptions in one component (one per useRedux() / useSelect() call) should be okay, because the component updates would get batched together.
Considering the main issue with the v6 implementation was performance, how feasible it is to work this upstream? Are there design issues that make React.Context inherently expensive or could it be implemented in a different way?
@saboya : yes, it's inherently "expensive", relatively speaking. See https://github.com/facebook/react/issues/14110 and https://github.com/facebook/react/issues/13739 for prior discussion (warning: long issue threads).
Let's inverse the pattern - move Connect from a wrapper for JSX to a wrapper for a function.
const connect = component => (props) => {
const result = magicMagic(component(props));
if (!magicMagic.shouldBailOut) {
lastResult = <Connect settings={magicMagic.information}>{result}</Connect>
}
return lastResult; // returning the same result is a way to bailout
}
const Component = () => {
useRedux(); // throw new Error('Please wrap with `connect`)
}
const ConnectedComponent = connect(Component); // Ok
The result is somewhere in the middle between HOC and hooks.
ConnectConnect@theKashey That's what's being suggested above my comments. I'm personally not against a HoC, but others don't seem to be digging it.
I'll be honest - all these theoretical comments are starting to feel somewhere between useless and confusing.
I think it's time that we started implementing some concrete ideas in forks and seeing how they look.
This is what I was proposing as a _fair comparison table_ two days ago. We have got too many opinions, realizations and strange edge cases to use a rule of thumb.
@theKashey : tbh, I still don't understand what that table was trying to compare, or how it relates to this API design discussion.
I think it's time that we started implementing some concrete ideas in forks and seeing how they look.
They would look like a comparison table. Performance? It's a row. Maintainability? It's a row. Verbosity? Row.
There are too many ideas, and the only way to properly compare apples and oranges is to _fairly_ compare pros, cons and tradeoffs. I mean - we could not pick solution cos we _like_ it. There is a responsibility on you to make a quite thoughtful decision.
So - I agree with you that we should (just)talking here and try to implement concrete ideas. Ideally, pick some existing solution and try to use it in a real project, when it would be clear what is missing.
Don't forget - it's not just about creating a new API, it's about _"Drawbacks", "Adoption strategy", "How we teach this"_ and the rest bad questions, so common for React RFC, but yet skipped here.
the table didn't look like it had anything to do with hooks.
I'm getting pretty mentally worn out after dealing with both the v7 thread and this thread.
I'm tired of issue comments and arguments. I want to see code.
build hooks, file PRs, let's see how stuff actually works, then make decisions based on that.
I have been thinking about this for a bit and made a minimalistic useRedux hook for my current internship project at the European Space Agency. For @markerikson's mental well-being I will post mostly code 😄. Sorry that its not a PR but I don't have the time right now to make a proper PR for this, but I still want to try and help.
// hooks.js
import { createSelector } from 'reselect'
const getPropsSelector = mapStateToProps => {
const selectors = Object.values(mapStateToProps)
const keys = Object.keys(mapStateToProps)
const propsSelector = createSelector(
...selectors,
(...args) =>
args.reduce((prev, cur, i) => ({ ...prev, [keys[i]]: cur }), {})
)
return propsSelector
}
// store object could be injected via the react context or
// by wrapping useRedux in a makeUseRedux function
export const useRedux = mapStateToProps => {
const propsSelector = useMemo(
() => getPropsSelector(mapStateToProps),
[mapStateToProps]
)
const [props, setProps] = useState(() => propsSelector(store.getState()))
useEffect(() => {
const handleChange = () => {
const state = store.getState()
const newProps = propsSelector(state)
setProps(newProps)
}
const unsubsribe = store.subscribe(handleChange)
return unsubsribe
}, [propsSelector])
return Object.assign(props, { dispatch: store.dispatch })
}
// some-component.js
// if you want to use props in the selectors you can move
// this object in Component and wrap it in a useMemo
const mapStateToProps = {
inputs: state => state.foo.inputs,
outputs: state => state.bar.outputs,
points: pointsSelector,
}
const Component = () => {
const { inputs, outputs, points, dispatch } = useRedux(mapStateToProps)
// use dispatch in combination with useCallback to create event handlers.
return ...
}
For the exact implementation reference see my current project's hooks file. I'm happy to elaborate if needed.
@mikeheddes Thanks for posting this! I do believe however that your implementation has the same issue as many of the others I've seen, in that each component that uses useRedux makes its own subscription to the store. I think ideally we would have a context provider that does the subscribing once, and every useRedux hook could use useContext instead of subscribing itself.
Please take this with a grain of salt because I am no expert
@TSMMark I made the decision to subscribe every component individually to the store when I read the 'Direct Component Subscriptions' section of the v7.0.1 release notes. Although I didn't do any benchmarking myself to see if it is actually better.
@TSMMark : no - v6 was built around propagating state updates via context, and we just switched _away_ from that approach in v7 specifically because it wasn't performant enough.
There's nothing wrong with having multiple subscriptions in one function component now that we're using unstable_batchedUpdates(). If I have two different calls to useSelect() in a component, and both of them trigger re-renders when an action was dispatched because the extracted data changed, the batching will ensure the component only re-renders once.
Great to know, thanks for clarifying ❤️
I have created a proof-of-concept npm package that contains my alternative 1 hooks implementation. In contrast to all other hooks packages (at least that I've seen) instead of being standalone this one depends on react-redux and uses its provider and subscription mechanism. Therefore, you can just add a dependency to the POC package in your app and start using the hooks immediately and concurrently with react-redux without the need for adding providers etc.
Here is the source. The API consists of three functions:
useRedux works in the style of useReducer and returns a tuple of a state slice and the dispatch function (the slice selector is optional and will return the root state if omitted)useReduxState only returns the state slice (again, selector being optional)useReduxDispatch only returns the dispatch functionIt would be great if anyone was willing to test this in their app. I have written some basic tests and created a sandbox to show it works. I will also fork the benchmark repo and convert all benchmark scenarios to use the hooks.
Any feedback is highly appreciated.
@MrWolfZ Looks like that's still susceptible to the top-down subscription problem. It either needs a HoC or wrapper component to redefine the subscription hierarchy.
@timdorr as I have mentioned in a comment buried somewhere deep in this thread, my proposal simply accepts that there will be cases in which props and state are out of sync inside the subscription callback and defers computing the final value to the render phase. This all hinges on the assumption that the selector is pure.
Let's look at the stale props scenario. A state update occurs, which causes the props and state to change. The child callback is executed first with the old selector (which may close over stale props), and may or may not cause a re-render (depending on what the stale props cause the selector to return). However, since a props change occurs, the component will re-render regardless of whether the subscription callback forced it or not. Since v7 uses batched updates, the component will not be re-rendered immediately, even if the callback forces it, but only after any parents have processed the store update as well. Then, during render we re-apply the selector with the latest props. The wrong result of applying the selector in the subscription callback does not matter at all at this point since it was used only as a heuristic to determine whether a render should be triggered.
The only edge case is when the stale props cause the selector to throw. My suggestion simply ignores errors when applying the selector in the subscription callback and forces a re-render. If the component gets unmounted before the render occurs, nothing happens. If it gets re-rendered the selector is applied again. Since it is pure (as per my assumption above) if the condition which caused the error in the callback (be that stale props or whatever) is still present, it will throw again (and allow error boundaries etc. to handle the error). However, if something changed, it will just work.
Personally, I can't come up with any case in which you would care about those hidden errors. However, one option I considered is to log the error in development mode (and allow suppressing it once a dev looked at the cause and determined it was harmless, e.g. useRedux(state => state.items[id], { suppressErrorsInSubscriptionCallback: true })).
In fact, the example sandbox I posted has the stale props situation and works just fine. This fork logs the error, so you can see what is happening.
EDIT: @timdorr your comment did however make me notice another staleness issue my code had, which I just fixed :)
So fwiw, I actually threw together a branch Tuesday evening with the five hooks I listed, with useSelect() based on @MrWolfZ 's "alternative 1", and useActions() based on @epeli 's library.
No tests yet (like, I literally haven't even tried to execute the code), and I didn't get around to pushing it up or publishing it yet, but hopefully I can throw it up tonight.
@MrWolfZ , @timdorr : yeah, as much as I dislike not having things always be correct, this approach seems like it might be acceptable.
I adapted code from use-redux to make a simple implementation for selectors and action creators (in TypeScript). Gist Here
Usage:
const users = useSelector(selectors.users);
const updateUser = useActionCreator(actionCreators.updateUser);
Okay, I just pushed up my utterly and completely experimental branch:
https://github.com/reduxjs/react-redux/tree/hooks-experiment-1
I'll see if I can maybe publish it as an alpha or something later.
First question: does it even run? :)
@markerikson A couple of comments:
useActions:
bindActionCreators is missing, no?I think memoizing on the actionCreators might be a bad idea. I imagine this hook to be used like this (from App.js of the deeptree-nested benchmark):
import { incrementRandomCounter } from "./counters";
import { appendRandomCharacter, appendRandomCharToMany } from "./strings";
const {
incrementRandomCounter,
incrementFifth,
incrementThird,
appendRandomCharacter,
appendMany,
} = useActions({
incrementRandomCounter,
incrementFifth: () => doUpdateMany(5),
incrementThird: () => doUpdateMany(3),
appendRandomCharacter,
appendMany: appendRandomCharToMany(4),
})
So the actions creators object is new every time, which means the callbacks are not stable. That means users would have to memoize the actions object themselves. Why not just use a deps parameter in useActions to let the user control the stability? One downside of this approach is that the linter will complain about the action creator imports not being listed in the deps (at least I think it will complain).
The code above has a subtle issue. Due to hoisting the incrementRandomCounter and appendRandomCharacter imports are not used, but instead the declared variables from the const are used in the actions object. To work around this you either have to rename the imports or the callbacks. Or you have to assign the actions to an object and then use properties on that object:
import { incrementRandomCounter } from "./counters";
import { appendRandomCharacter, appendRandomCharToMany } from "./strings";
const actions = useActions({
incrementRandomCounter,
// ...
})
// use as onClick={actions.incrementRandomCounter}
So this kind of API is a bit tricky to use. Sadly the API that only returns dispatch has the same problem if you try to memoize the action. So this must simply be made very explicit in the docs I think.
I have added a useReduxActions hook that uses deps to my POC package.
useSelect:
I fixed a bug yesterday in this. In the useIsomorphicLayoutEffect call, you need to add a line latestSelectedState.current = selectedState
General feedback:
I prefer to go for the simplest possible API, even if it does not cover 100% of the use cases. In your hooks you are allowing overriding the context to use (as connect does). I think this is a bad idea, since it opens a can of worms. Maybe some people will want to just override the store, not the whole context, so you'll need another parameter storeToUse. And then maybe somebody comes up with something else they want to override. This will lead to a long list of optional positional parameters. The alternative would of course be to use a parameter object with the overrides, but this still causes a lot of complexity (e.g. what happens if both storeToUse and contextToUse are specified?). Lastly, this causes a lot of noise in the code using these hooks. While I don't particularly like these overrides in connect either, at least there you only had to specify them once per component through the props and the component itself didn't need to be aware of them since they were handled in the connect wrapper (which is useful for the primary use case for these overrides: testing). With hooks you have to specify them for each hook call, so it bleeds into your whole component.
So, I have finally implemented all benchmarks using the hooks. Below you can find the results. 7.0.1-h are the results with hooks. Please note that I get a lot of errors being printed to the console when executing the benchmarks (with and without hooks), and I am not sure what the cause or impact of those errors is. The scripting, rendering and painting values also differ greatly between runs (sometimes, 100, other times 10000), so you should probably ignore them and focus on the FPS value. If someone was willing to execute the benchmarks on their machine again to see whether it works without errors for them, that would be great.
Here is the fork of the benchmarks repo and here is a comparison between the adjusted benchmarks and the original. I think I was able to properly re-implement the components with hooks, but please feel free to check the implementations (sometimes it was tricky to not fundamentally change the implementation and thereby corrupt the results).
Finally, here are the results. As you can see the numbers are very similar. I am not 100% sure yet where the differences in deeptree-nested, stockticker and twitter-lite are coming from.
Results for benchmark deeptree:
┌─────────┬─────────┬──────────────┬───────────┬───────────┬──────────┬──────────────────────────────────────────────┐
│ Version │ Avg FPS │ Render │ Scripting │ Rendering │ Painting │ FPS Values │
│ │ │ (Mount, Avg) │ │ │ │ │
├─────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼──────────────────────────────────────────────┤
│ 7.0.1 │ 26.42 │ 107.0, 0.7 │ 3564.00 │ 10349.00 │ 5334.00 │ 23,26,27,26,27,26,27,25,26,27,26,27,26,27,27 │
├─────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼──────────────────────────────────────────────┤
│ 7.0.1-h │ 28.09 │ 89.0, 0.6 │ 3556.00 │ 10297.00 │ 5055.00 │ 27,29,28,29,28,27,28,27,28,27,29,28,29,28,29 │
└─────────┴─────────┴──────────────┴───────────┴───────────┴──────────┴──────────────────────────────────────────────┘
Results for benchmark deeptree-nested:
┌─────────┬─────────┬──────────────┬───────────┬───────────┬──────────┬──────────────────────────────────────────────────────────────────────┐
│ Version │ Avg FPS │ Render │ Scripting │ Rendering │ Painting │ FPS Values │
│ │ │ (Mount, Avg) │ │ │ │ │
├─────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼──────────────────────────────────────────────────────────────────────┤
│ 7.0.1 │ 43.33 │ 167.0, 0.4 │ 61.00 │ 3754.00 │ 8164.00 │ 52,50,48,50,52,51,50,51,50,39,36,37,36,34,39,36,37,32,38,36,35,36,36 │
├─────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼──────────────────────────────────────────────────────────────────────┤
│ 7.0.1-h │ 58.00 │ 122.0, 0.2 │ 41.00 │ 13725.00 │ 7594.00 │ 60,58,57,59,57,59,57,58,60,57,58,56,59,60,56,59,58,57,59,58,57,58,59 │
└─────────┴─────────┴──────────────┴───────────┴───────────┴──────────┴──────────────────────────────────────────────────────────────────────┘
Results for benchmark forms:
┌─────────┬─────────┬──────────────┬───────────┬───────────┬──────────┬────────────────────────────────────────────────────────────────────────────┐
│ Version │ Avg FPS │ Render │ Scripting │ Rendering │ Painting │ FPS Values │
│ │ │ (Mount, Avg) │ │ │ │ │
├─────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼────────────────────────────────────────────────────────────────────────────┤
│ 7.0.1 │ 48.57 │ 1213.0, 0.2 │ 17145.00 │ 1097.00 │ 3316.00 │ 52,53,54,53,55,54,52,54,55,52,45,46,44,43,46,43,44,41,46,43,49,43,41,49,49 │
├─────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼────────────────────────────────────────────────────────────────────────────┤
│ 7.0.1-h │ 53.69 │ 1172.0, 0.3 │ 14884.00 │ 824.00 │ 2418.00 │ 53,54,51,54,53,52,54,53,52,56,54,57,53,54,53,54,53,54,53,54,53,56,53,57,57 │
└─────────┴─────────┴──────────────┴───────────┴───────────┴──────────┴────────────────────────────────────────────────────────────────────────────┘
Results for benchmark stockticker:
┌─────────┬─────────┬──────────────┬───────────┬───────────┬──────────┬──────────────────────────────────────────────────────────────────────────────────┐
│ Version │ Avg FPS │ Render │ Scripting │ Rendering │ Painting │ FPS Values │
│ │ │ (Mount, Avg) │ │ │ │ │
├─────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼──────────────────────────────────────────────────────────────────────────────────┤
│ 7.0.1 │ 37.77 │ 249.0, 0.3 │ 123.00 │ 8778.00 │ 5880.00 │ 39,41,42,39,38,39,40,39,38,36,37,39,36,38,37,36,37,38,39,37,36,37,34,37,36,37,37 │
├─────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼──────────────────────────────────────────────────────────────────────────────────┤
│ 7.0.1-h │ 56.57 │ 150.0, 0.4 │ 3512.00 │ 13413.00 │ 6113.00 │ 53,60,59,60,54,55,60,57,55,56,57,56,54,57,58,56,55,58,56,58,54,56,57,56,58,54,54 │
└─────────┴─────────┴──────────────┴───────────┴───────────┴──────────┴──────────────────────────────────────────────────────────────────────────────────┘
Results for benchmark tree-view:
┌─────────┬─────────┬──────────────┬───────────┬───────────┬──────────┬─────────────────────────────────────────────────────────────────────────────────────┐
│ Version │ Avg FPS │ Render │ Scripting │ Rendering │ Painting │ FPS Values │
│ │ │ (Mount, Avg) │ │ │ │ │
├─────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────────────────────────────────────────┤
│ 7.0.1 │ 40.10 │ 628.0, 0.2 │ 440.00 │ 13035.00 │ 8595.00 │ 36,37,51,34,36,33,38,39,47,42,38,40,33,44,42,29,41,44,36,43,52,42,34,43,41,44,43,43 │
├─────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────────────────────────────────────────┤
│ 7.0.1-h │ 38.02 │ 520.0, 0.8 │ 7772.00 │ 14225.00 │ 2773.00 │ 28,37,51,30,32,24,41,40,43,37,46,25,43,33,42,33,38,39,42,51,40,41,33,37,34,41,46,33 │
└─────────┴─────────┴──────────────┴───────────┴───────────┴──────────┴─────────────────────────────────────────────────────────────────────────────────────┘
Results for benchmark twitter-lite:
┌─────────┬─────────┬──────────────┬───────────┬───────────┬──────────┬──────────────────────────────────────────────────────────┐
│ Version │ Avg FPS │ Render │ Scripting │ Rendering │ Painting │ FPS Values │
│ │ │ (Mount, Avg) │ │ │ │ │
├─────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼──────────────────────────────────────────────────────────┤
│ 7.0.1 │ 38.83 │ 3.0, 0.2 │ 16354.00 │ 5077.00 │ 723.00 │ 58,57,59,56,50,42,37,38,34,33,30,29,28,26,25,24,23,22,22 │
├─────────┼─────────┼──────────────┼───────────┼───────────┼──────────┼──────────────────────────────────────────────────────────┤
│ 7.0.1-h │ 27.15 │ 2.0, 4.7 │ 2952.00 │ 5379.00 │ 1067.00 │ 60,58,53,43,36,34,29,27,26,23,22,21,20,19,17,18,17,16,15 │
└─────────┴─────────┴──────────────┴───────────┴───────────┴──────────┴──────────────────────────────────────────────────────────┘
I ran all benchmarks I have and @MrWolfZ 's benchmarks in https://github.com/dai-shi/reactive-react-redux/pull/19#issuecomment-482780101. Let me copy&paste the table (of Avg FPS) here with the hope that someone might get some insights.
| react-redux 7.0.2 | @mrwolfz/react-redux-hooks-poc | reactive-react-redux useReduxState | reactive-react-redux useReduxSelectors | redux-react-hook | |
|---|---|---|---|---|---|
| deeptree | 36.14 | 36.24 | 33.78 | 31.75 | |
| deeptree-nested | 50.99 | 58.96 | 51.13 | 42.67 | 51.30 |
| forms | 55.20 | 55.86 | 53.92 | ||
| stockticker | 51.54 | 59.10 | 23.10 | 22.91 | |
| tree-view | 50.49 | 48.87 | 46.30 | 49.55 | |
| twitter-lite | 48.22 | 33.52 | 53.64 | 56.58 |
Let's not worry about benchmarking prototypical approaches. This issue is about API design. Perf is an implementation detail.
As for the API design, I'd like to raise one discussion.
Based on https://github.com/reduxjs/react-redux/issues/1179#issuecomment-480547676:
Possible Path Forward
So here's a hypothetical suggested path towards actually building this:
- Use something like the "alternative 1" approach, and ship these hooks:
- useSelect(): subscribe-only
There are two options for useSelect().
a) pass deps
const selector = state => state.items[id];
const item = useSelect(selector, [id]);
b) useCallback
const selector = useCallback(state => state.items[id], [id]);
const item = useSelect(selector);
The context is a bit different, but I saw this comment: https://github.com/facebook/react/pull/15022#issuecomment-470643909
b) is probably better if we want the react-hooks/exhaustive-deps check.
@dai-shi two thoughts about (b):
It seems to commit you to defining selectors in the component that uses them, which could make re-use in other functions tricky.
If you have an existing app with selectors defined in the (a) style it would be nice to use them in new hook-based components and (b) would make that tricky again.
Actually on second reading I wonder if the (b) option works if you’ve already written selectors using reselect?
@ee0pdt I don't see why it wouldn't, selectors created externally using reselect are already referentially stable so there would be no need to wrap it in useCallback.
My apologies. Somehow, I didn't follow the latest proposed code.
It doesn't depend on selector identity, so my discussion point is void.
I created a live demo (written in typescript to check if typings were feasible) using @markerikson 's proposed API here https://codesandbox.io/s/32wol1yv21
console logs show which counters where rendered
Using reselect(with props) should be called an antipattern with hooks due to _"one cached result per selector"_ from one point of view, and hook _composeability_ from another. And I am not sure that _mapStateToProps factory_ would be a good way to solve problem this time, as it always was not.
In the same time, it's impossible to use useMemo for the same task due to selection cascades breaking the hooks rules.
Redux-friendly derived data generation/selection is still a problem, and not covered here enought, while it shall be a part of hooks API - something like useCreateSelector(fn) ~ useMemo, but selection cascade friendly.
@theKashey : sorry, I don't think I understand what you're saying there.
I totally get the issues with standard Reselect selectors having a cache size of 1, and that reusing them in multiple components will cause memoization to be useless if they all do selectValueById(state, ownProps.id) because the IDs are different.
I don't know what you're referring to by "selection cascades", or how that "breaks hooks rules". Can you give some examples of what you mean?
Selection cascades are the base of reselect.
createSelector(
getTodos, // also selectors or might become selectors in a future.
getUsers,
getPage,
(todos, users, page) => limit(joinTodosAndUsers(todos, users), page, PER_PAGE)
)
The problem is that every selector is memoized, and if you've changed page - getPage will update, but getTodos and getUsers would be still memoized. That lead to obvios conclusion - in any point of time any numbers of selectors could be memoized, and another any number could be not.
That leads to a problem, where you could not replace reselect by useMemo, due to rule _"Hooks are called in the same order each time a component renders."_.
So, from one side - you have to use nested selectors to properly handle state changes, but from another - you cannot use useMemo for it. And you cannot use reselect(ok, not always able). This ends up with a confused user, who had a two _common_ memoization technique just a second ago, and now left alone with _nothing_.
So, what I am saying here - this time memoization should be covered by redux. It should be a part of a new API. Not just useSelect as it was proposed before, but with built-in memoization capable to handle common cases.
Okay, now I get what you mean by "selection cascades", but I still don't get what you are saying about useMemo being an issue in regards to the rules of hooks.
You can't call useMemo() inside of useMemo(), so it's not like you'd be doing:
const paginatedUsers = useMemo(() => {
// this is illegal
const todos = useMemo(() => {
return state.todos;
})
// etc
})
Instead, you'd be doing things sequentially:
const todos = useMemo(() => {
return state.todos;
})
const users = useMemo(() => {
return state.users;
})
const paginatedUsers = useMemo(() => {
}, [todos, users])
It's also important to note that the actual memoization status of these isn't what's important - it's the fact that you are just calling useMemo (and any other hook) in the same order each time this renders.
I've double checked a few projects of mine, and in all cases, I could _flatten_ deeply nested selectors like in your example without any harm to performance. So, probably this "my" problem is solvable via documentation only - just a bit different code style and that's done.
But, as long as another approach is not helping during migration, and you could not reuse old reselect selectors - I am going to spike a hooks-friendly reselect implementation, ie useReselect.
@theKashey : just since you mention that, have you seen https://github.com/reduxjs/reselect/issues/401 and the linked post / repo?
@markerikson I have a question. (as I've read your gist.)
Is stale props issue not solved? My expectation was your useSelect (aka @MrWolfZ 's "alternative 1") and batchedUpdates solve it.
I'd like to know if I'm really wrong, because my lib uses batchedUpdates and (flat) subscriptions, and the stale props issue can't be reproduced anymore. (I think this is nothing to do with proxy, but it's just about reading state in render.)
I'm interested in edge-case examples to reproduce the stale props issue. Is there any other example than https://codesandbox.io/s/42164qln37 ?
(edit) So, what I understood afterwards is it's technically unsolved, but we hope it's not going to be a big problem because parent component is likely to re-render.
Actually, is there a minimal test case that reproduces stale props and zombie children issue? It would be really helpful to test the new implementations against it
@Torvin here are two test cases for stale props with hooks. Which one you choose depends on what you want to verify with your implementation. The tests must of course be adapted to your concrete API.
If you want to verify that stale props cause no issues, use this:
it('ignores transient errors in selector (e.g. due to stale props)', () => {
const Parent = () => {
const count = useReduxState()
return <Child parentCount={count} />
}
const Child = ({ parentCount }) => {
const result = useReduxState(count => {
if (count !== parentCount) {
throw new Error()
}
return count + parentCount;
})
return <div>{result}</div>
}
const store = createStore((count = -1) => count + 1)
const App = () => (
<Provider store={store}>
<Parent />
</Provider>
)
rtl.render(<App />)
expect(() => store.dispatch({ type: '' })).not.toThrowError()
})
If you want to verify that the state selector is never called with stale props, use this:
it('ensures consistency of state and props in selector', () => {
let selectorSawInconsistencies = false
const Parent = () => {
const count = useReduxState()
return <Child parentCount={count} />
}
const Child = ({ parentCount }) => {
const result = useReduxState(count => {
selectorSawInconsistencies = selectorSawInconsistencies || (count !== parentCount)
return count + parentCount;
})
return <div>{result}</div>
}
const store = createStore((count = -1) => count + 1)
const App = () => (
<Provider store={store}>
<Parent />
</Provider>
)
rtl.render(<App />)
store.dispatch({ type: '' })
expect(selectorSawInconsistencies).toBe(false)
})
EDIT: there was a mistake in the second test. Instead of <Child parentCount={count + 1} /> it needs to be <Child parentCount={count} /> (I already adjusted the code above). Thanks @dai-shi for pointing this out.
Thinking some more about the options for useActions.
Seems like there's basically three obvious potential signatures:
useActions(addTodo)
useActions([addTodo, toggleTodo})
useActions({addTodo, toggleTodo})
(Note that I'm really _not_ considering offering a hook that accepts the (dispatch) => dispatchProps function form that connect allows for mapDispatch, both because I don't like that form, and because if you really want to do that you can call useDispatch() yourself instead.)
Redux's bindActionCreators() already supports either a single action creator or an object, so the single function and object signatures are free. The array signature is also trivial to implement.
The main questions are:
As an alternative to a deps array, I could imagine doing a shallowEqual(actionsArg) check inside the hook, but that would fail if users opted to define functions inline, like useActions({addTodo : () => ({type : "ADD_TODO"}) }).
@markerikson sure, here's a sandbox that uses my POC and that shows the bug in action. As you can see the editor and linter are able to detect the issue, so as long as those are used the issue might be easy to detect.
Regarding memoization, I think the deps approach is best, since a) it does not force you to do it, but if you decide you need the performance gains you can easily opt in with minimal code changes, and b) it is consistent with how other hooks work.
Regarding static typing, I don't see any issue having those overloads. You can easily type them separately to get clean signatures (at least in TypeScript). (EDIT: in fact, tuples are tricky to type, so the typings become a bit ugly as you can see here)
Regarding naming of the hooks: The reason I use Redux in the name of each hook in my POC is to prevent conflicts with other libraries/hooks. I never really understood why people are still trying to get the shortest possible variable names. Usually, you wouldn't type all of it anyways, but let your editor's autocomplete take care of it. And then, when you see it in code it is much more clear where the hook comes from and what it does (and remember, we spend 90% of our time reading code, only 10% writing it (the numbers are of course completely made up, but you get my point)).
I'm not a big fan of the object form because in the most common use case you would have to either
const { addTodo: onAddTodo, toggleTodo: onToggleTodo } = useActions({addTodo, toggleTodo});
or
const { addTodo, toggleTodo } = useActions({ addTodo: actionCreators.addTodo, toggleTodo: actionCreators.toggleTodo });
which are both verbose. I like the array form and single func form.
As far as types is concerned, I think an overload could handle each of those cases fine. (At least in TypeScript)
I think it's more idiomatic Hooks if we just do the single value and array forms. That matches your other core Hook APIs more closely.
The object form allows this pattern:
import * as tilesActions from "./actions/tilesActions";
import * as toolActions from "./actions/toolActions";
const { updateTile, removeTile, setCommand } from useActions({ ...toolActions, ...tilesActions });
This eliminates shadowing (and the bug of calling unbound actions), without needing to build an array of action creators manually.
Yeah, the workaround to the "shadowing" issue is to define the action creators object ahead of time rather than inline - either by import * as actions from "./someActions", or manually creating the object. @cmrigney , note that per your example you can just do const {addTodo, toggleTodo} = useActions(actionCreators).
I agree the shadowing problem is tricky and nasty. Still, given the prevalence of the object form already, I want to keep it. We would just need to _very_ carefully document the hazards of passing an object inline.
At this point I want to ship all three forms, and I see that @MrWolfZ has already implemented them in his POC lib. I'm inclined to say that @MrWolfZ should turn that lib into a PR, and we ship it as an alpha.
My naming preference is still useSelect/useActions/useDispatch/useStore/useRedux, but I'm willing to be convinced otherwise if enough folks feel strongly enough about it.
Just put up a Twitter poll on the topic: https://twitter.com/acemarke/status/1118898809990516736
Thoughts?
I would rather replace useSelect with useStoreState. It matches the form of use<Noun> that the core Hooks API follows, but avoids conflicts with useState.
For what it's worth, "select" is an adjective or a verb, and adjectives can be "nouns" (they're called substantives).
@timdorr maybe @markerikson can confirm this, but I believe if we got rid of useSelect and only used useStore then the component would re-render every time any part of the redux state changed, vs just when the selected value changed. Though I think if there was useSelect and useStore, that would solve both.
@cmrigney : the hook would still take a selector regardless of the name, it's just a question of naming it.
@timdorr, for a noun option, maybe useSelector? Could we end up having an object of selectors as an argument? If so, then maybe useSelectors?
I wonder if there would be some regret later in using the term "state" too much. We're all used to mentally distinguish React "state" and Redux "state", but previously the term state was used in the HOC and now they're inching it towards the actual component function. Maybe it's fine. Would it also be fine for newcomers?
Naming will probably be a compromise between correctness, obviousness, and I guess distinctness from React terms. Don't you guys feel there is something ever so slightly unsettling about React having state and reducers, and dispatch too now? If I was a newcomer, would that be obvious? And would I care if it wasn't immediately obvious?
@timdorr useMemo and useEffect don't really follow this trend, Memoization and Effect aren't exactly nouns.
Anyway, Is the stale props issue a big deal?
From my tests the selector only sees the stale prop when it is called in the subscribe callback, so the problem is moot if the new selected state is not set in the subscribe callback but during a re-render, that way even if the stale props fail to trigger a re-render in the subscribe callback, the component will re-render anyway because of the new props and get the correct selected state.
Although this method means that your selector will be called twice for every re-render.
That said, there is a good chance I have no idea what I'm talking about.
@Dudeonyx Are you sure? Memo and Effect are both nouns.
Eg.
Noun
useMemouseEffectuseSelectorVerb
useMemoizeuseAffectuseSelect@charrondev You're probably correct, but on a slightly different note I think that useSelector is a much better name than useSelect.
@markerikson, are you leaning towards useActions instead of say, useAction, because of style reasons? If there are no performance differences, do you think multiple invocations is going to be distracting? My very first impression is that an useAction hook in the singular, called multiple times, might look better.
@itoed : it's more that:
import * as actions from "./actions"bindActionCreators() can handle either a single action creator or an objectSo, at this point I'm intending to ship a single useActions() that can take a single action creator, an array of action creators, or an object of action creators. If you want to call it multiple times with a single argument, that's up to you.
@markerikson, I see. Here are just some observations about usage in my personal experience, in case it helps:
Your first point was definitely true for me when I was more committed to the Presentation component vs Container component style. In my experiment converting an existing project to a Redux hook implementation where hooks may be reusable, I'm thinking this will become less common.
For your second point, my interpretation of return values that are lists is more like they are semantically tuples. In the case of useActions it seems more like a proper collection, not a tuple like [state, setState]. For comparison, I was imagining useAction to feel more like useCallback or useRef.
In any case, I can't wait for hooks so either would be wonderful!
@Dudeonyx : correct, that is the actual definition of the "stale props" issue.
I did a brain dump explaining what "stale props" and "zombie child" actually mean over in Reactiflux the other day, and I saved the chat log here:
https://gist.github.com/markerikson/faac6ae4aca7b82a058e13216a7888ec
How about a [noun, verb] tuple convention?
useMemo:
const [data, memoizeData] = useMemo(initialData);
Your first point was definitely true for me when I was more committed to the Presentation component vs Container component style. In my experiment converting an existing project to a Redux hook implementation where hooks may be reusable, I'm thinking this will become less common.
I actually have found the Container / Presentation component to be as important as ever when using Redux hooks. (I've been using @MrWolfZ's code for the last couple days, so I have a bit of an understanding of the impact on DX, here).
Ultimately, my Container components are handling retrieving data from stores, or side effects to pull that data from a remote data source. That leaves Presentation components (the direct child of a ContainerComponent) to receive the (in my case, TypeScript typed) struct representing whatever data was loaded using hooks.
Sure, one of the claims to hooks was to reduce the component hierarchy, and I think that it does still largely minimize it – when you use your Container component to retrieve multiple resources from a redux store or fetch them from a URL.
I assume this will also aid in testing, as it's not really viable to use enzyme.shallow with hooks anyway, so passing mock structs seems like a reasonable approach. Same goes for developing a Storybook: it's a bit easier to develop individual components with a known prop structure rather than loading that in the component itself.
(Sorry for the aside, hope it's helpful to someone).
As @markerikson suggested, I have created a PR with my hooks implementation.
For the naming, I dropped the Redux infix since most posters here seemed to like that better. I have however changed useSelect to useSelector as was suggested above (I have also prototyped a version useSelectors that allows passing multiple selectors, but decided against it since a) the code is much more simple like this, and b) it is trivial to just use multiple useSelector calls instead or just return an object in the selector).
I have also dropped the useRedux hook since it felt redundant and any line of code that isn't written is a good line of code, as it reduces complexity, improves maintainability, and can't cause bugs.
Very cool to see this coming into shape.
The new docs don't look quite right for the useCallback with useSelector example (though useCallback vs. useMemo isn't fully in my head yet):
const todoSelector = useCallback(() => {
return state => state.todos[props.id]
}, [props.id])
const todo = useSelector(todoSelector)
It seems like the useCallback is actually being used as if it were useMemo, when it should just be:
const todoSelector = useCallback(
state => state.todos[props.id],
[props.id]
)
const todo = useSelector(todoSelector)
The equivalent useMemo version would be as per the current example:
const todoSelector = useMemo(() => {
return state => state.todos[props.id]
}, [props.id])
const todo = useSelector(todoSelector)
Sorry if I've missed the answer to this earlier, but what is the idea behind having the dependencies array in useAction, but not in useSelector? It would be nice if useSelector had it too as that feels more hooks-idiomatic and we wouldn't then need to use useMemo or useCallback if we include props in it.
@mungojam You didn't miss anything. It was indeed an oversight on my part that I noticed myself over the weekend, as discussed in this post. I think it makes perfect sense to add the deps to useSelector, but it might make useRedux a bit more tricky.
Would it be possible that useSelector provides some way to bail out of a rerender by passing it a ref value.
My scenario is the following: in react native apps when you use react navigation for example, unlike on the web you don’t just render the screen that the user sees but also the whole stack of screens the user navigated over to get where he is and any parallel tabs. This is necessary for animations between screens etc.. Now the issue is you don’t want to rerender a bunch of hidden screens all the time but with redux all these components are subscribed to state updates and you can’t control when they update. In the past I’d just get a flag wether the component is corresponding to the currently active screen and then add a should component update HOC via recompose after the redux connect. But with hooks this doesn’t work anymore. I’d ideally like to be able to give a ref to the useSelector hook to control wether it should update a subscription. I can’t think of a way to build a custom hook that would achieve this behavior.
@MrLoh I don't think we should do this for such an edge case. However, my idea for this situation would be to just ensure the selector returns the same value as long as the active tab is not the tab the component is nested in. As long as you have the active tab in the state, this should be trivial to achieve. It should also be possible to turn this into a custom hook to select the state inside a tab.
Closing this out since the code is merged in.
@timdorr Well, it's only an alpha and I think there are still some design decisions to be made.
@MrLoh you can conditionally pass a dud selector when a component is in the background. i.e
const state = useSelector(inView ? actualSelector : ()=> null);
That way when not in view the component would never re-render due to redux state changes.
Although I can envision a few bugs potentially arising from an incorrect or stale inView boolean causing the state to be null when it shouldn't be.
@mungojam : yup, that's what happens when I write docs at midnight and haven't actually used useCallback() myself yet :)
Everyone: since this issue has gotten way too long, I've opened up #1252 for discussion of the alpha itself. Please offer feedback there.
@Dudeonyx I don't think this would work, as it would lead to a serenader as soon as the screen becomes inactive and everything goes to an empty/loading state. Nevertheless it should be possible to create a special memoized selector that saves and returns the last state when the screen is not inView
Here's a crazy idea:
Provide syntax sugar with exact same behaviour as mapStateToProps, mapDispatchToProps.
P.S. I'm not experienced enough to tell if the latter is better served by context instead.
@dimaqq Discussion has moved to #1252 but that will probably not happen, the nice thing is, it's super easy to build your own custom hooks with whatever flavor you need based on the redux hooks
Most helpful comment
the table didn't look like it had anything to do with hooks.
I'm getting pretty mentally worn out after dealing with both the v7 thread and this thread.
I'm tired of issue comments and arguments. I want to see code.
build hooks, file PRs, let's see how stuff actually works, then make decisions based on that.