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

Feedback: React-Redux v7 hooks alpha #1252

Open
markerikson opened this issue Apr 22, 2019 · 191 comments

Comments

@markerikson
Copy link
Contributor

commented Apr 22, 2019

Please use this thread to give feedback on the new React-Redux hooks APIs available in 7.1-alpha.

Prior references and discussion:

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

Copying over comments from the other couple threads for reference:

#1179 (comment) (@mungojam ):

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

#1179 (comment) (@mungojam ):

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.

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

#1179 (comment) (@MrWolfZ ):

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

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

#1248 (comment) (@markerikson ):

A couple quick observations as I work on docs:

  • useRedux() doesn't allow you to pass a deps array for actions

  • We probably oughta tweak the error message for useReduxContext() a bit

  • Do we need that same "didUnsubscribe" check in useSelector() that we have in connect()?

  • Do we want to make an empty deps array the default for useActions()? How often would you actually want to recreate those every time?

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

#1248 (comment) (@MrWolfZ ):

@markerikson I worked a bit with the hooks on the weekend, and also made an observation. Currently, the selector in useSelector is re-created every time. That means if you use a memoizing selector you need to memoize the selector itself yourself. This could be fixed by also passing a deps array into useSelector.

Now my comments to your observations:

useRedux() doesn't allow you to pass a deps array for actions

Indeed, I completely missed this. It would be easy enough to add this parameter, but if we decide to add deps to useSelector as mentioned above, then things get messy. Do we only use a single deps for useRedux? Or pass in two of them? I think this is an argument for removing this hook after all.

Do we need that same "didUnsubscribe" check in useSelector() that we have in connect()?

I intentionally removed this, since it was only required to ensure the selector never sees inconsistent state and props. Basically, with my implementation it does not matter whether the subscription callback is called after the component was unmounted. The only thing that happens is potentially a forceRender call that does nothing. Also, based on this issue the didUnsubscribe guard doesn't seem to be enough to prevent the callback to be called after unmounting anyways. On the flip side it doesn't hurt to have that check in there and it could give minimal performance improvements by not calling the selector when we know we don't need to. I prefer having a simpler implementation (and not having to write a test for it ;) ), but feel free to add it back in.

Do we want to make an empty deps array the default for useActions()? How often would you actually want to recreate those every time?

I strongly argue to not make an empty deps array the default. Firstly, to be consistent with how useEffect, useMemo, and useCallback work. Secondly, if we were to do this, it might lead to very subtle bugs with stale props inside the actions that are incredibly hard to debug. By leaving the default undefined the worst thing that can happen is slightly worse performance and that can easily be fixed by just adding the deps yourself.

@vadzim

This comment has been minimized.

Copy link

commented Apr 22, 2019

As I understand the main reason to have the object form of useActions is to use all the actions from a module like

import * as actions from 'myactions'
...
const { action1, action2 } = useActions(actions)
...

I see a possible disadvantage here that statically analyzing tools will not be able to understand that some of actions are not used in a project anymore. E.g. WebStorm notifies about unused exports in a project.

@mungojam

This comment has been minimized.

Copy link

commented Apr 22, 2019

Moving my comment #1248 (comment)

const boundAC = useActions(actionCreator : Function, deps : any[])

const boundACsObject = useActions(actionCreators : Object<string, Function>, deps : any[])

const boundACsArray = useActions(actionCreators : Function[], deps : any[])

Unless there is a compelling reason for both, I think it would be good to drop either the 2nd or 3rd version of useActions. I'd rather have just one way to create multiple actionCreators.

I probably prefer the object one as it is more auto-documenting within the hook. The hooks that use arrays like useState etc. are different because it is for a single thing and the two array elements usually have a single name i.e. [count, setCount]. It also lets users name them easily rather than {state: count, setState: setCount}. You don't have that problem with useActions.

@mungojam

This comment has been minimized.

Copy link

commented Apr 22, 2019

Moving my comment #1248 (comment)

Do we only use a single deps for useRedux? Or pass in two of them? I think this is an argument for removing this hook after all.

Personally I think useRedux isn't very hooksy. Either people will write their action creator and selector logic within the useRedux call and then it's quite verbose, or they will write them outside and then what is actually being usefully added by useRedux? Maybe there are some more advanced scenarios it could cater for if added later but I don't think it should be added if it's just for familiarity with connect as it's just an added layer over the other hooks.

@threehams

This comment has been minimized.

Copy link

commented Apr 22, 2019

@MrWolfZ Are you planning to maintain a PR at https://github.com/DefinitelyTyped/DefinitelyTyped? If not, I can add one later tonight and maintain it with alpha changes.

@Satyam

This comment has been minimized.

Copy link

commented Apr 22, 2019

In useSelector, in line 55, where it says:

const memoizedSelector = useMemo(() => selector, deps)

Wouldn't it make sense to have:

const memoizedSelector = useMemo(() => () => selector(deps) , deps)

so you could do:

const todo = useSelector(([id]) => state => state.todos[id], [props.id])

I would also love to have the option of using a path for a selector, much like lodash _.get() but using placeholders for dependencies, thus:

const selTodoItem = ([id]) => state => state.todos[id];

Would turn into:

const selTodoItem = 'todos.%0';

@markerikson markerikson pinned this issue Apr 22, 2019

@MrWolfZ

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

@threehams thanks for reminding me. I have created a PR with the hooks types. Any feedback is highly welcome, since it is the first time I am contributing to DefinitelyTyped.

@Satyam what is the benefit of getting the props into the selector as an argument instead of just by closure? One downside would definitely be more typing. Regarding the path, I feel that is a very special case that you can easily create a custom hook for. One of the points of hooks was to make it easy to compose them and to make it easy to create your own. I am not the maintainer of this repo, so I don't have the last say on this, but I definitely feel we should leave the hooks as simple as possible and not overload them too much.

@Satyam

This comment has been minimized.

Copy link

commented Apr 22, 2019

@MrWolfZ The reason for having the selectors get the props, or whatever other dependency, as an argument is that I don't usually mix details of the redux store along the components. I have the selectors defined somewhere along the store so that if the store changes in any way, I don't have to go searching for all the affected selectors embedded within the components. Likewise, if the same selector is used in several places, I don't have to copy and paste the same selector code into the several components that use it. And it is not the component props that I mean to use as arguments for the selector but any value taken from wherever it might be. The arguments used by the selector and its dependencies are one and the same, I can hardly imagine a selector that has more dependencies than arguments or vice versa.

Also, I don't see why those arguments are to be passed as an array of dependencies, it might be better to have it as an object so the arguments can be named. The fact that they are also dependencies in the memoized selector should be an implementation detail solved internally, for example:

const memoizedSelector = useMemo(() => () => selector(deps) , Object.values(deps))

So, the selector could be called:

const todo = const todo = useSelector(selTodoItem, { id: props.id});

where selTodoItem would be defined elsewhere as :

const selTodoItem = ({id}) => state => state.todos[id];

which clearly is not such an improvement over my previous example with just one argument passed as an array, but it would make things clearer if there were more than one argument.
So, basically, the principle of separation of concerns is what makes it funny to have the action creators and the reducers all close to the store, but the selectors along the components, just to leverage the closure.

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@Satyam : I'm going to have to disagree with several of your opinions here:

  • I don't understand your proposal for const todo = useSelector(([id]) => state => state.todos[id], [props.id]) at all
  • We're definitely not going to add some kind of a string interpolation overload. If you want to generate selectors along that line, it should be easy enough to do yourself and pass them to useSelector
  • Dependency arrays are the natural form here because that's how useMemo and useCallback work. There's no reason to ask users to define them as an object, only to promptly convert the values back into an array.
  • If you want to pre-memoize the selector function yourself, you can do that. But, given the common case of selecting values based on props, it's reasonable for us to offer an API that just handles that automatically if the deps are supplied.
@MrWolfZ

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@Satyam the Object.values approach won't work, since it uses the same order as for...in and that order is arbitrary and not necessarily stable, as you can see here.

Regarding passing args to the selector, I assume the most common use case will be inline selectors that can just use closure. However, even for your case you can just do this:

const todo = useSelector(selTodoItem({ id: props.id}), [props.id]);

If you really want to have such a hook (and don't care about stability of props order), it is easy to implement it:

const useMySelector = (selector, deps) => useSelector(selector(deps), Object.values(deps))
@Satyam

This comment has been minimized.

Copy link

commented Apr 23, 2019

@markerikson

@Satyam : I'm going to have to disagree with several of your opinions here:

  • I don't understand your proposal for const todo = useSelector(([id]) => state => state.todos[id], [props.id]) at all

The reason for const todo = useSelector(([id]) => state => state.todos[id], [props.id]) is that the selector can be defined elsewhere, more likely along the store itself, like shown later: const selTodoItem = ([id]) => state => state.todos[id];. As the argument id comes as an argument, it doesn't rely on closure which requires theselector to be defined within the component.

  • We're definitely not going to add some kind of a string interpolation overload. If you want to generate selectors along that line, it should be easy enough to do yourself and pass them to useSelector

The string interpolation was just a wish list item of mine, I shouldn't have mentioned it along the main issue, sorry.

  • Dependency arrays are the natural form here because that's how useMemo and useCallback work. There's no reason to ask users to define them as an object, only to promptly convert the values back into an array.

It feels like the external API is being defined by the internal implementation of the API and not by the way a developer might use it. The developer should not be concerned if the API uses useMemo or whatever else internally. It should not be defined because useMemo expects dependencies, it should be defined because the selector needs arguments which happen to be its dependencies.

  • If you want to pre-memoize the selector function yourself, you can do that. But, given the common case of selecting values based on props, it's reasonable for us to offer an API that just handles that automatically if the deps are supplied.

Actually, when I wrote that part, the documented API didn't have the deps argument yet so it no longer applies.

@MrWolfZ That is true, redundant as it is. If the deps are the arguments to the selector, why not giving them to it? I don't know the reason why useMemo doesn't provide the memoized function with the arguments but it clearly is something a developer would expect. The way the issue is highlighted in the documentation clearly shows that it is contrary to normal expectation. I would assume that there is some technical reason for that in useMemo, I don't see any reason not to have it in useSelector and avoid the unnecessary repetition or tying up the selector to the component by closure.

@Satyam

This comment has been minimized.

Copy link

commented Apr 23, 2019

@MrWolfZ Besides, if you do it the way you suggest:

const useMySelector = (selector, deps) => useSelector(selector(deps), Object.values(deps))

What is the point of memoizing if the part that does depend on the arguments, that is selector(deps) has already been evaluated?
Anyway, my point in passing arguments as an object was to show that there are many ways to define the API for useSelector that are better oriented to the developer using it and are not so much focused on the way useMemo works.

@ricokahler

This comment has been minimized.

Copy link

commented Apr 23, 2019

@MrWolfZ these look amazing! Nice job!

Here are my two cents:

I worked a bit with the hooks on the weekend, and also made an observation. Currently, the selector in useSelector is re-created every time. That means if you use a memoizing selector you need to memoize the selector itself yourself. This could be fixed by also passing a deps array into useSelector.

I think the deps array should be a required argument, it makes things faster and isn't much of a mental lift for the user to add. The current typings for useMemo also require the deps array so I think it wouldn't be a bad idea to make that argument required and invariant if it's not there.


if (shallowEqual(newSelectedState, latestSelectedState.current)) {

I wonder if this would be better as an Object.is or === check here. I've messed with some redux hooks and I've always used multiple useSelector calls vs returning an object e.g.

function Foo() {
  // returning an object where shallowEqual helps
  const { a, b } = useSelector(state => ({ state.a, state.b }), []);

  // using multiple calls to `useSelector`
  const a = useSelector(state => state.a, []);
  const b = useSelector(state => state.b, []);
  // i like doing this 👆better
}

I like this better because it feels simpler. If you're returning part of the store without any calculations or creation of objects/arrays, you don't need to shallowEqual. It also feels more hook-esque to call useSelector more than once.

In cases where I would want to create an object within the selector, I like to pass in a memoized function e.g.

import { createSelector } from 'reselect';

const selectA = state => state.a;
const selectB = state => state.b;
const selectAB = createSelector(selectA, selectB, (a, b) => ({ a, b }));

function Foo() {
  const ab = useSelector(selectAB, []);
}

and then memoized function would return the same object reference not needing the shallow equal check either.

Maybe that's more complicated? but it fits in with the current Redux ecosystem so I'm okay with it.


In regards to Action Object Hoisting, I've actually found it to be relatively ergonomic to just useDispatch and wrap all action calls in dispatch.

function Foo(personId) {
  const dispatch = useDispatch();
  
  useEffect(() => {
    dispatch(fetchPerson(personId));
  }, [personId]);

  // ...
}

The only time where I found it to be more ergonomic to use something like useActions is when passing abound version of an action creator to a lower components. I've done it like this:

function Container() {
  return <PresentationalComponent addTodo={useAction(addTodo)} />
}

However, there is another idea I've had inspired by @material-ui/styles:

In material-ui/styles, they have a factory makeStyles that returns a hook useStyles

import React from 'react';
import { makeStyles } from '@material-ui/styles';

const useStyles = makeStyles({
  root: {
    backgroundColor: 'red',
  },
});

export default function MyComponent() {
  const classes = useStyles();
  return <div className={classes.root} />;
}

Similarly, we could create a factory makeAction that will wrap an action creator and return a hook:

import React from 'react';
import { makeAction } from 'react-redux';
//                               👇 could also pass in an existing action creator
const useAddTodo = makeAction(todoName => ({ type: 'ADD_TODO', payload: todoName }));

function Foo() {
  const addTodo = useAddTodo();

  // ...
}

Maybe that's more verbose but I like the idea and conventions of it. Let me know what you think.

@josepot

This comment has been minimized.

Copy link

commented Apr 23, 2019

Hi and thanks a lot for this @MrWolfZ and @markerikson !

I like it, a lot!

Although, I agree with @ricokahler in that I would prefer for useSelector to perform an Object.is or a === instead of a shallow-compare. I understand that the first thing that the shallowCompare does is to check for referential equality, and that the only difference that it would make for me would be the few unnecessary comparisons that will take place when the result of the selector has actually changed... But still, I would much rather if useSelect didn't perform a shallow-compare by default... Perhaps that could be an option? or another hook useObjectSelector?

Finally, about the "controversy" on whether useSelector should accept props or not. I like the fact that it doesn't. However, if Reselect v5 proposal got accepted, perhaps it would make sense to include a different hook named useInstanceSelector (or something like that) that could accept props and "usable selectors". Although, I understand that could be seen as coupling this API to the API of an optional dependency, so I can see how that could be controversial... Although, I still think that it would be an idea worth considering.

Thanks again for this!

@MrWolfZ

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@Satyam

@MrWolfZ Besides, if you do it the way you suggest:

const useMySelector = (selector, deps) => useSelector(selector(deps), Object.values(deps))

What is the point of memoizing if the part that does depend on the arguments, that is selector(deps) has already been evaluated?

I'm not sure I understand. Yes, a new result for selector(deps) is always created, but the callback that is memoized and then used to select from the state is the first one that was created for each deps. So if selector memoizes on either the deps, the state or both it will work.

Anyway, my point in passing arguments as an object was to show that there are many ways to define the API for useSelector that are better oriented to the developer using it and are not so much focused on the way useMemo works.

I'm just gonna say that your suggestion is oriented towards how you are using the API, but I am sure there are loads of other ways to use it we can't even think of. The idea behind making it work the same as useMemo and useCallback is to a) make it immediately familiar to all react hooks users, and b) to build upon the foundations that the react team built that they surely have put some thought into. Also, with your suggestion, the semantics of the first parameter would change based on whether the second parameter was passed (i.e. we would probably need different handling for no deps, empty deps, and filled deps). This makes the API very complex and also makes it difficult to add the deps parameter later on. I completely understand where you are coming from with this proposal, but in the end, there is no right answer to API design and this mostly comes down to personal preference, and I prefer to stick with the API I implemented.

@ricokahler

I think the deps array should be a required argument, it makes things faster and isn't much of a mental lift for the user to add. The current typings for useMemo also require the deps array so I think it wouldn't be a bad idea to make that argument required and invariant if it's not there.

I don't mind making it mandatory, but I also don't strongly feel that it needs to be. The only downside of not providing it would be potentially worse performance and the fix to that is easy. I also think this case is slightly different to useMemo in that useMemo would be completely pointless without the deps but useSelector works just fine without it.

I wonder if this would be better as an Object.is or === check here.

Yeah, I thought about this as well. First of all, even with the current implementation nothing keeps you from calling useSelector multiple times. Secondly, the downside of not doing shallowEquals is that useSelector(state => ({ a: state.a, b: state.b })) is now always causing a render and the fix to that would be not very obvious. I think that usage pattern will be common enough to justify doing shallowEquals. Lastly, I have actually benchmarked the hooks both with reference equality and shallowEquals and the results were almost identical, even though the benchmarks exactly trigger the sub-optimal behaviour by selecting large arrays from the state where each value is compared with shallowEquals (sadly I didn't keep the results around but you can easily reproduce this if you want).

Similarly, we could create a factory makeAction that will wrap an action creator and return a hook:

Yup, I have thought about this as well and I am actually doing something similar in a (toy) project I work on. That said, the current API does in fact already allow you to do this easily. Just do this:

const useAddTodo = () => useActions(todoName => ({ type: 'ADD_TODO', payload: todoName }))

If you find that unwieldy you can always easily create makeAction yourself:

const makeAction = ac => () => useActions(ac)

@josepot

Finally, about the "controversy" on whether useSelector should accept props or not. I like the fact that it doesn't. However, if Reselect v5 proposal got accepted, perhaps it would make sense to include a different hook named useInstanceSelector (or something like that) that could accept props and "usable selectors". Although, I understand that could be seen as coupling this API to the API of an optional dependency, so I can see how that could be controversial... Although, I still think that it would be an idea worth considering.

I have to admit I am not very familiar with reselect and the likes. However, all the examples I see at reselect and also in redux-views have selectors that depend on how mapStateToProps works, i.e. taking state and props as arguments. With hooks you can actually achieve the same by just using closure (basically, by doing what I suggested above already):

const getUsers = state => state.users

const getUser = id => createSelector(
  getUsers,
  users => users[id]
)

const user = useSelector(getUser(props.id), [props.id])

In that example, useSelector takes care of memoizing on the props, while reselect takes care of memoizing on the users. However, as I said I am not so familiar with this approach, so please let me know if I made a mistake in my way of thinking.

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

FWIW, it's always possible for us to add more hooks down the road, so if anything I would prefer to keep the initial list relatively minimal and primitive. On the flip side, I also don't want to have to go changing the hooks we ship (no more major versions!), so we want to get these initial APIs correct before they go final.

@janhesters

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

Don't know if this is the right place for mistakes in the docs and whether this is indeed an error, but in the next docs it says:

const increaseCounter = ({ amount }) => ({
  type: 'increase-counter',
  amount
})

export const CounterComponent = ({ value }) => {
  // supports passing an object of action creators
  const { increaseCounterByOne, increaseCounterByTwo } = useActions(
    {
      increaseCounterByOne: () => increaseCounter(1),
      increaseCounterByTwo: () => increaseCounter(2)
    },
    []
  )

increaseCounter(1) would throw, because 1 has no property called amount

I think this should be:

increaseCounterByOne: () => increaseCounter({ amount: 1 }),
increaseCounterByTwo: () => increaseCounter({ amount: 2 })

or

const increaseCounter = amount => ({
  type: 'increase-counter',
  amount
})

I might be wrong here though.

@G710

This comment has been minimized.

Copy link

commented Apr 23, 2019

Hi,

first of all, thank you for your hard work and thoughts that went into this library (and this new API in particular 👍 )

I've been experimenting with the lib today and so far I really like it! The provided hooks fit perfectly into the new eco-system and feel natural to use. I've never really warmed up to the mapXToProps functions but the hooks just make sense to me.

I don't really get the fuzz about the useSelector deps. I mean, you can pass props to the function in it's current state, right? (seems to be working in my project) You just have to be careful that your selector can't run into an invalid state.

As far as I understand most suggestions so far can be build upon the existing functions and are easy enough to a) just implement in the project or b) write up a small lib.

Let's see if I run into any issues in the next few days but so far the transition seems to be seamless. Thanks a lot! 👍

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

@janhesters : yep, good catch. If you've got time, please submit a PR to fix those examples in the docs on master and in the source comments in v7-hooks-alpha (which is where I copied them from).

@G710 : yes, you can totally use props in selectors, it's just that there's certain cases you have to watch out for. Thanks for the feedback!

@janhesters

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

@markerikson Sure. Which of the two fixes do you want me to create a PR for?

A:

increaseCounterByOne: () => increaseCounter({ amount: 1 }),
increaseCounterByTwo: () => increaseCounter({ amount: 2 })

or B:

const increaseCounter = amount => ({
  type: 'increase-counter',
  amount
})
@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

I'd say change amount from a destructured object to a simple number, to match the other usages (increaseCounter(2)).

@janhesters

This comment has been minimized.

Copy link
Contributor

commented Apr 23, 2019

PR for master.

But I can't find hooks.md in the v7-hooks-alpha branch 🤔

Do you mean the hooks-docs branch?

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented Apr 23, 2019

Correct - due to the docs setup, the docs themselves are on master, even though we've got the code over in that v7-hooks-alpha branch.

@timdorr

This comment has been minimized.

Copy link
Member

commented May 8, 2019

So, I'm pretty split on the whole memoization issue. It would be really nice if there was a useProps, because then we could mirror the mSTP behavior of checking for a second arg on the function. Oh well...

I think I'm in favor of removing it. The whole point of adding Hooks isn't to make users recreate connect inside of their components; it's to allow a little bit of Redux usage anywhere without having to break out a heavyweight tool. Very often you will be using one Hook at a time. If you're using useSelector and useDispatch, you're basically a step away from needing connect anyways, so just use that.

I think we need to be pretty clear: This is not a replacement for connect. Let's keep that in mind as we discuss this stuff too.

@mdcone

This comment has been minimized.

Copy link

commented May 9, 2019

Writing tests in enzyme has been easy for components wrapped in a connect because we can pass in the store as a prop and then do a shallow render.

When using the hooks API, however, we are forced to wrap the code in a Provider in order to supply the store to the context. I won't get into the details here about how we are using enzyme, but this makes testing much harder to do.

Is there a way that the store can be set for the hooks API in a testing scenario? Is that something that should be considered at all?

@timdorr

This comment has been minimized.

Copy link
Member

commented May 9, 2019

Nope, not being considered yet. You should stick with connect for those kinds of things.

One possible thing would be to let you pass in a store to the hooks via props:

function MyComp({ store }) {
  const val = useSelector(mySelector, store)

And we just check for either arguments.length or store === undefined.

@MichaelDeBoey

This comment has been minimized.

Copy link

commented May 10, 2019

@mdcone If you need to change your tests because you're changing to hooks, you're probably testing implementation details.

A way to be sure you're not testing implementation details is to try and use the great react-testing-library @kentcdodds provided to the community. 🙂

@retyui

This comment has been minimized.

Copy link

commented May 10, 2019

How about use declarative style for useSelector:

const getUserName = (store, id) => {/* ... */};

const name = useSelector(getUserName, [props.userId]);
@mdcone

This comment has been minimized.

Copy link

commented May 10, 2019

@timdorr, that dependency injection approach would work. Feels weird supplying the store as a prop, but maybe that's the tradeoff that would need to be made.

@MichaelDeBoey, thanks for the reference, I'll check it out.

@josepot The root of the matter is that when we render out a component using enzyme, and it is wrapped in a Provider, and then we dive into that component to verify some of its children, react-redux loses the context the Provider had set and throws an exception.

This could just be an issue with how enzyme works or how we are using it, but something I noticed yesterday when using these new hooks and writing a test for the component using them.

Thanks for the feedback and this library!

@josepot

This comment has been minimized.

Copy link

commented May 10, 2019

One possible thing would be to let you pass in a store to the hooks via props

I really dislike this idea.

Adding this to the API just to provide a means for writing those kinds of tests seems wrong. Also, the only way to take advantage of that would be to change the API of the component so that it can receive the store through props just for testing purposes...

This problem can be easily fixed from the tests, without the need of having to write all your components in a super-weird way just for testing purposes. I mean, if you have to change the API of all your components just so that you can test them... Perhaps you should reconsider the way that you are testing them.

Instead of changing the API of all your components, why not use a HOC like this in your tests?

const withStore = store => Component => props => (
  <Provider store={store}>
    <Component {...props} />
  </Provider>
);

Also, as @timdorr already said before:

This is not a replacement for connect. Let's keep that in mind as we discuss this stuff too.

@josepot

This comment has been minimized.

Copy link

commented May 10, 2019

Hi @mdcone

@josepot The root of the matter is that when we render out a component using enzyme, and it is wrapped in a Provider, and then we dive into that component to verify some of its children, react-redux loses the context the Provider had set and throws an exception.

This could just be an issue with how enzyme works or how we are using it, but something I noticed yesterday when using these new hooks and writing a test for the component using them.

Is this code public? Or could you please show the relevant parts of this somewhere? I would like to have a look at it because I really think that this is an XY problem. Also, I agree with @MichaelDeBoey check this out.

@mdcone

This comment has been minimized.

Copy link

commented May 10, 2019

@josepot It isn't public, but here's an example:

test('The component outputs a div element', () => {
        const store = generateStoreWithData({
            message: 'Howdy'
        });

        function MessageComponent() {
            const message = useSelector(state => state.message);

            return <div>{message}</div>;
        }

        const renderedWithProvider = shallow(
            <Provider store={store}>
                <MessageComponent />
            </Provider>
        );

        expect(renderedWithProvider.find('MessageComponent').length).toBe(1);

        const messageComponent = renderedWithProvider
            .find('MessageComponent')
            .dive();

        expect(messageComponent.find('div').length).toBe(1);
    });

The error produced is "Invariant Violation: could not find react-redux context value; please ensure the component is wrapped in a " at the point where the dive is called.

@josepot

This comment has been minimized.

Copy link

commented May 10, 2019

@mdcone

I'm not an enzyme expert, but I think that for those tests you should use mount instead of shallow.

According to the enzyme docs:

  • shallow:

Shallow rendering is useful to constrain yourself to testing a component as a unit, and to ensure that your tests aren't indirectly asserting on behavior of child components.

  • mount:

A method that re-mounts the component, if it is not currently mounted. This can be used to simulate a component going through an unmount/mount lifecycle.

No equivalent for ShallowWrappers.

TBH I'm surprised that shallow works with connected components 🤷‍♂

Although, please notice that even the React docs recommend using react-testing-library. IMO it's a much better testing approach than using enzyme.\

EDIT

A few things that I've learned from this:

  1. It's dangerous to give advice when you don't fully know other ppl situation. I didn't know that you were using React Native. It is true that this library exists, but I suspect that most RN developers are still using enzyme... If I had known that, then I would have thought it twice before "pushing" you out of enzyme. My bad for being so judgemental.

  2. It turns out that I got lucky with my "mount" suggestion because those tests do work using mount.

  3. Despite the fact that the tests are working, I can see this message in the console:

Warning: unmountComponentAtNode(): The node you're attempting to unmount was rendered by another copy of React.

This makes me feel suspicious that enzyme must be using its own version of React, and as we all know hooks don't work properly when there are duplicate React versions. So, I checked if enzyme's latest version fully supports hooks and it turns out that it doesn't yet.

  1. It's quite likely that once enzyme fully supports hooks you won't have this issue. So, if that is the case, then this is, in fact, an XY problem. In any event, this is a tooling issue, you can setup enzyme to work with mount. It does look painful, but it can be done 🙂.
@mdcone

This comment has been minimized.

Copy link

commented May 10, 2019

@josepot Thanks for the advice. We're running in React Native, so I'm not sure that mount works without hacking a solution together because it wants a DOM to attach to, but I do appreciate you taking the time to help out.

Sounds like using a tool other than enzyme will be worth evaluating going forward!

@AlicanC

This comment has been minimized.

Copy link

commented May 12, 2019

I'm having a problem with useSelector().

Example:
https://codesandbox.io/s/925no65j4w

Explainer:
I have a map (new Map()) in my state and I have an action that sets values in it by cloning the map, setting the value and putting the clone in the next state.

When I read the map using myMap = useSelector(state => state.myMap) my component doesn't get updated so I have to write { myMap } = useSelector(state => state) instead.

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

@AlicanC : I think I know why that's happening, and it makes sense to me that you're seeing that.

You're creating a new Map instance each time. However, as a class instance, all of its methods are actually on the prototype. Also, all the data is stored internal to the map. So, two different Map instances are actually shallow-equal to each other, and useSelector() is not triggering an update. Putting the Map instance into an object as a field causes the returned object to not be shallow equal to the last result.

As a side note, we would generally recommend not putting Maps or other non-serializable values into your Redux store. The code may run, but you're more likely to encounter problems like this.

@josepot

This comment has been minimized.

Copy link

commented May 12, 2019

@markerikson @AlicanC

As a side note, we would generally recommend not putting Maps or other non-serializable values into your Redux store. The code may run, but you're more likely to encounter problems like this.

IMO a Map is pretty easy to serialize / deserialize. Just because it's not a plain javascript object does not mean that it's not serializable. Some people even put ImmutableJs instances in their redux-store. Although, I do agree that for that case using an Object makes more sense.

To me, what's clearly problematic is having useSelector perform a shallow-equal, instead of performing an Object.is or a strictly equal. 🤷‍♂

IMO this issue highlights what I already said before: which is that performing a shallow-equal comparison on useSelector creates a leaky abstraction, because it forces developers to understand a pretty arbitrary implementation detail.

@artem-malko

This comment has been minimized.

Copy link

commented May 13, 2019

Great feature!
I have just one question: how can I apply factory mapStateToProps flow to useSelector? I mean https://react-redux.js.org/api/connect#returns (text from note).

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

@artem-malko : you would want to generate your own selectors that capture props and memoize that with useMemo() or useCallback():

function MyComponent(props) => {
    const selector = useCallback(
        (state) => state.todos[props.id],
        [props.id]
    );

    const todo = useSelector(selector);
}

On which note, it looks like we don't have an example like that in the hooks alpha docs page. Either we had one and it got removed, or we never put one in. We should show that.

@timdorr

This comment has been minimized.

Copy link
Member

commented May 13, 2019

That should show up in #1267 now that I've merged into the deps removal stuff.

@MrWolfZ

This comment has been minimized.

Copy link
Contributor

commented May 13, 2019

@Lenic

This comment has been minimized.

Copy link

commented May 15, 2019

I think this is the right way:

// mycode.js
function MyComponent(props) => {
  const todo = useSelector(state => state.todos[props.id], state => [props.id, state.todos]);
  // other code
}

And the useSelector cache selector parameter like this:

// useSelector.js
export function useSelector(selector, depsFunc) {
  const { store, subscription: contextSub } = useReduxContext();

  // compute dependency by current state
  const deps = depsFunc(store.getState());

  const latestDeps = useRef(null);
  const latestSelectedState = useRef(null);

  if(latestSelectedState.current !== null && latestSelectedState.current !== null && equalDeps(deps, latestDeps.current)) {
    return latestSelectedState.current;
  }

  latestDeps.current = deps;
  const currentSelector = useCallback(selector, deps);
  // other code
@josepot

This comment has been minimized.

Copy link

commented May 15, 2019

@Lenic

#1272 was recently merged, which removes the deps argument of useSelector. In that PR there is the following comment from @gaearon that I think summarizes the rationale for this change very well:

FWIW our conclusion internally has been that custom Hooks should avoid their own dependency argument, and people should just memoize if it's necessary. You have to learn that pattern anyway.

Also, I don't understand what you are suggesting. I think that the only valid reason for changing the selector provided to useSelector is when the selector depends on props. See this comment from @timdorr and the answer that follows. The depsFunc that you are proposing is just another selector... Which means that logic could be encapsulated in just one function that composes those 2 selectors. IMO that makes depsFunc a superfluous argument.

@Lenic

This comment has been minimized.

Copy link

commented May 16, 2019

@josepot

I understand what @gaearon means, the useSelector api is the same as the connect function, and the extra caching is provided by the third-party library.

My idea is to provide an api similar to React Hooks, with dependencies to decide whether to recalculate the results.

// useCacheSelector.js
export default function useCacheSelector(selector, depFunc) {
  const store = useStore();
  const deps = depFunc(store.getState());

  return useMemo(()=> useSelector(selector), deps);
}

This way, I can get the cached value if the dependency has not changed. I am doing the job like a reselect library.

// mycode.js
function MyComponent(props) {
  const todo = useCacheSelector(state => state.todos[props.id], state => [props.id, state.todos]);
  // other code
}
@istarkov

This comment has been minimized.

Copy link
Contributor

commented May 16, 2019

It seems for me that useLayoutEffect here

useIsomorphicLayoutEffect(() => {

is not needed and can be changed to useEffect.

As even you lost event in between render and useEffect then checkForUpdates will do all the dirty work

checkForUpdates()

In case of useEffect you will need to check didCancel in event handler see this

As for now the only test which will fail is

it('subscribes to the store synchronously', () => {
let rootSubscription
const Parent = () => {
const { subscription } = useReduxContext()
rootSubscription = subscription
const count = useSelector(s => s.count)
return count === 1 ? <Child /> : null
}
const Child = () => {
const count = useSelector(s => s.count)
return <div>{count}</div>
}
rtl.render(
<ProviderMock store={store}>
<Parent />
</ProviderMock>
)
expect(rootSubscription.listeners.get().length).toBe(1)
store.dispatch({ type: '' })
expect(rootSubscription.listeners.get().length).toBe(2)
})
and it's check some internal implementation details without any reason I know.

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@istarkov : I believe that's there because we had to modify connect() to use layout effects for its subscriptions to solve some timing bugs, so we're doing the same thing here for consistency.

@MrWolfZ

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

@istarkov your observations are indeed correct, as is @markerikson's explanation as to the why.

While there is no technical need to use a layout effect, I thought it makes sense for consistency. Since there isn't really an observable difference, the test checks that implementation detail. However, we could easily change it if necessary. Do you know of a good reason we should prefer useEffect over useLayoutEffect?

Also, maybe we should add a comment to that effect that explains this so that the question doesn't come up again in the future.

@istarkov

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

Thank you for the answers!

Do you know of a good reason we should prefer useEffect over useLayoutEffect`?

In case of current code and React version - no. There are no heavy computations inside, having sync nature seems like this

checkForUpdates()
can be deleted also useLayoutEffect highly simplifies code and thinking about, also its somehow "system" level library so all is fine.

For me the most issue is incorrect comment

// useLayoutEffect in the browser. We need useLayoutEffect to ensure the store
// subscription callback always has the selector from the latest render commit
// available, otherwise a store update may happen between render and the effect,
// which may cause missed updates; we also must ensure the store subscription
// is created synchronously, otherwise a store update may occur before the
// subscription is created and an inconsistent state may be observed
as here useLayoutEffect is used just to hold callbacks and data references as an optimisation hack.
As for now useLayoutEffect looks like the best way to avoid additional use{XXX}Effect dependencies for optimisation purposes.

As like many others before writing my code I'm using github to find similar code for ideas, good practices, possible edge cases etc - so this comment made me think that there is something exists I don't understand ;-) and the best way to learn is to ask here, thank you again!

@timdorr

This comment has been minimized.

Copy link
Member

commented May 17, 2019

From @iamawebgook here #1286 (comment)

With connect you can pass manual equality checking function, instead of shallowEqual, but there is no way to do it with useSelector, there is no way to fix these kind of cases and turns out I need to rewrite lots of my code, because I have different data structures used on my components. Also transforming to other data types may be overhead. I suggest to add some way to support manual comparison of values for useSelector.

I do think this is something we should add. Maybe as an options arg?

@MrWolfZ

This comment has been minimized.

Copy link
Contributor

commented May 17, 2019

@timdorr yeah, in regards to the comparison I was thinking as well about making this an option. At the same time, I want the API to be as simple as possible, and if a custom comparison is required, one can always just use connect.

@markerikson

This comment has been minimized.

Copy link
Contributor Author

commented May 20, 2019

After a bunch of discussion on equality behavior over in #1288 , I've just published v7.1.0-alpha.5, which:

  • makes reference equality the default
  • adds an optional comparison function as the second arg to useSelector()
  • exports our shallowEqual function for use with useSelector() if desired
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.