Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Discussion: Potential hooks API design #1179

Closed
markerikson opened this issue Feb 7, 2019 · 194 comments
Closed

Discussion: Potential hooks API design #1179

markerikson opened this issue Feb 7, 2019 · 194 comments

Comments

@markerikson
Copy link
Contributor

@markerikson markerikson commented Feb 7, 2019

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
Copy link
Contributor

@esamattis esamattis commented Feb 8, 2019

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 bailout
    • I think think this is essential for empowering userland wrappers
  • useDispatch() - just return the dispatch function
  • useStore() - Too powerful?

Maybe these higher level APIs

  • useActionCreators() takes an actions creators object and binds it to dispatch (memoized)
  • Variation of useMapState() with shallow equal check
  • useSelector() - reselect like helper
    • This might mitigate the need for the shallow equal check in the useMapState primitive
    • I have an example implementation of this in my redux-hooks lib

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
Copy link

@jcestibariz jcestibariz commented Feb 8, 2019

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

@Jessidhia
Copy link

@Jessidhia Jessidhia commented Feb 8, 2019

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
Copy link

@Jessidhia Jessidhia commented Feb 8, 2019

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
Copy link

@chris-pardy chris-pardy commented Feb 8, 2019

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

@esamattis
Copy link
Contributor

@esamattis esamattis commented Feb 8, 2019

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
Copy link

@adamkleingit adamkleingit commented Feb 9, 2019

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
Copy link

@chris-pardy chris-pardy commented Feb 12, 2019

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
Copy link
Contributor Author

@markerikson markerikson commented Feb 12, 2019

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
Copy link

@chris-pardy chris-pardy commented Feb 12, 2019

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
Copy link
Contributor

@ricokahler ricokahler commented Feb 12, 2019

  • 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
Copy link

@adamkleingit adamkleingit commented Feb 12, 2019

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
Copy link

@saboya saboya commented Feb 14, 2019

@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
Copy link

@chris-pardy chris-pardy commented Feb 15, 2019

@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
Copy link
Contributor Author

@markerikson markerikson commented Feb 15, 2019

@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
Copy link

@ivansky ivansky commented Feb 16, 2019

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
Copy link

@ivansky ivansky commented Feb 16, 2019

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
Copy link

@jbrodriguez jbrodriguez commented Feb 20, 2019

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
Copy link
Contributor

@ricokahler ricokahler commented Feb 20, 2019

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
Copy link

@flepretre flepretre commented Feb 27, 2019

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
Copy link
Contributor

@ricokahler ricokahler commented Mar 17, 2019

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
Copy link
Contributor

@esamattis esamattis commented Mar 17, 2019

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
Copy link
Contributor Author

@markerikson markerikson commented Mar 18, 2019

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

@dfee
Copy link

@dfee dfee commented Apr 18, 2019

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

@MrWolfZ
Copy link
Contributor

@MrWolfZ MrWolfZ commented Apr 19, 2019

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.

@mungojam
Copy link

@mungojam mungojam commented Apr 22, 2019

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)
@mungojam
Copy link

@mungojam mungojam commented Apr 22, 2019

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.

@MrWolfZ
Copy link
Contributor

@MrWolfZ MrWolfZ commented Apr 22, 2019

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

@MrLoh
Copy link

@MrLoh MrLoh commented Apr 22, 2019

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.

@MrWolfZ
Copy link
Contributor

@MrWolfZ MrWolfZ commented Apr 22, 2019

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

@timdorr
Copy link
Member

@timdorr timdorr commented Apr 22, 2019

Closing this out since the code is merged in.

@timdorr timdorr closed this Apr 22, 2019
@timdorr timdorr unpinned this issue Apr 22, 2019
@MrWolfZ
Copy link
Contributor

@MrWolfZ MrWolfZ commented Apr 22, 2019

@timdorr Well, it's only an alpha and I think there are still some design decisions to be made.

@Dudeonyx
Copy link

@Dudeonyx Dudeonyx commented Apr 22, 2019

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

@markerikson
Copy link
Contributor Author

@markerikson markerikson commented Apr 22, 2019

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

@MrLoh
Copy link

@MrLoh MrLoh commented Apr 22, 2019

@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

@dimaqq
Copy link

@dimaqq dimaqq commented Apr 24, 2019

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.

@MrLoh
Copy link

@MrLoh MrLoh commented Apr 24, 2019

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet