reduxjs / react-redux

Official React bindings for Redux
https://react-redux.js.org
MIT License
23.39k stars 3.36k forks source link

Discussion: Potential hooks API design #1179

Closed markerikson closed 5 years ago

markerikson commented 5 years ago

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.

esamattis commented 5 years ago

Based on my experiments I've come up with following wishlist for the official redux hooks api:

Provide low level primitives

Maybe these higher level APIs

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

jcestibariz commented 5 years ago

There's a similar project in the Facebook incubator: https://github.com/facebookincubator/redux-react-hook

Jessidhia commented 5 years ago

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)
Jessidhia commented 5 years ago

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.

chris-pardy commented 5 years ago

Thinking about this as well and would suggest:

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

esamattis commented 5 years ago

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.

adamkleingit commented 5 years ago

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'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

chris-pardy commented 5 years ago

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.

markerikson commented 5 years ago

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 bindActionCreators helper 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 store instance. (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 select function
  • 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' select function 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 shouldComponentUpdate wherever 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?

chris-pardy commented 5 years ago

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 shouldComponentUpdate wherever 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.

ricokahler commented 5 years ago
  • 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?

adamkleingit commented 5 years ago

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();
saboya commented 5 years ago

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

chris-pardy commented 5 years ago

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

markerikson commented 5 years ago

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

ivansky commented 5 years ago

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,
    );
}
ivansky commented 5 years ago

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);
    }
}
jbrodriguez commented 5 years ago

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

ricokahler commented 5 years ago

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

flepretre commented 5 years ago

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.

ricokahler commented 5 years ago

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?

esamattis commented 5 years ago

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.

markerikson commented 5 years ago

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

t83714 commented 5 years ago

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>
    );
}

Benefits:

I 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(){
        ...
    }
}
klandell commented 5 years ago

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

markerikson commented 5 years ago

@klandell : no timeline yet. To be entirely transparent:

So, frankly, I don't see it happening very soon, and I don't even want to speculate on actual timelines.

klandell commented 5 years ago

@markerikson Refactor later it is then. Thanks again!

esamattis commented 5 years ago

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

markerikson commented 5 years ago

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

ricokahler commented 5 years ago

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?

ricokahler commented 5 years ago

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} />;
}
  • The first thing I like about this API is that it's very simple and communicates what's going in the simplest way possible. I bounced this idea off all the engineers on my team at all different levels of seniority and they all knew what was going on. I also like the aesthetic of making a hook literally called useRedux because it's very clear that it's the thing that makes pushes happen from redux.
  • The second thing I like about this API is that it leads to relying on React for optimizations and makes react-redux do a less work. Instead of relying on 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:

  • the assumption that no object is placed in the redux tree more than once (which i feel like is a fine assumption given how people use redux).
  • the optimization will only work if you select a reference type (e.g. selecting a primitive or string wouldn't work)
  • ⚠️ no longer having the ability to rely on top-down order updates (which started the comment above)

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!

saboya commented 5 years ago

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.

markerikson commented 5 years ago

@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' select function needs to be able to take their props into account
  • We should have shouldComponentUpdate wherever we can

In other words:

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 :)

ricokahler commented 5 years ago

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??

markerikson commented 5 years ago

maybe we could have an overload for connect?

OH PLEASE NO NO MORE OVERLOADS FOR CONNECT AHHHHHHHH

runs away screaming

markerikson commented 5 years ago

So asking a more serious question:

saboya commented 5 years ago

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?

markerikson commented 5 years ago

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

t83714 commented 5 years ago

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

markerikson commented 5 years ago

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

t83714 commented 5 years ago

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

Could 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
}
markerikson commented 5 years ago

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

t83714 commented 5 years ago

@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)?

esamattis commented 5 years ago

@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()

MrWolfZ commented 5 years ago

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.

MrWolfZ commented 5 years ago

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

esamattis commented 5 years ago

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

MrWolfZ commented 5 years ago

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

ricokahler commented 5 years ago

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:

  1. I believe that 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.
  2. Given that API, there is still the constraint of top-down updates and making those top-down updates work with connect. The only way I can foresee this being possible is by re-using the same tiered Subscriptions that connect uses.
  3. 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);

4. Given that users may want to `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.
5. It also makes sense to use batched updates to make multiple calls to `useRedux` run in one tick.

---

Remarks:

- Still requiring an HOC to make hooks work is a bit annoying but I think given the constraints, this is the best way to get `useRedux` _and_ `connect` to play nicely together since we're using the same tiered subscription mechanism.
- Thinking about it more the `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.
```js
// 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);

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?

markerikson commented 5 years ago

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