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

add react hooks for accessing redux store state and dispatching redux actions #1248

Merged
merged 5 commits into from Apr 22, 2019

Conversation

6 participants
@MrWolfZ
Copy link
Contributor

commented Apr 19, 2019

As discussed in the hooks API design issue, here is an implementation that could be used as an alpha.

There are a couple of open points:

  • are tests for HMR required? (the corresponding tests in connect.spec.js are skipped)
  • I added inline documentation to the hooks, but other code doesn't have this (instead, the docu is in the types); not sure what you want to do here
  • I haven't updated the docs yet to mention hooks
  • naming of hooks could change depending on the API design issue
@netlify

This comment has been minimized.

Copy link

commented Apr 19, 2019

Deploy preview for react-redux-docs ready!

Built with commit 91116ac

https://deploy-preview-1248--react-redux-docs.netlify.com

@markerikson

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

Awesome. Initial thoughts:

  • Yeah, I'd still like to have the HMR tests updated at some point, but that's @theKashey 's area of expertise. I've pinged him a couple times on that and haven't gotten anything.
  • Totally fine with having some inline documentation for now.
  • We can hold off on actually updating the docs for the moment while this is still in alpha . I know @wgao19 set up versioned docs, but I'm not clear yet on how to handle the "current" vs "next" docs content.
  • Per the naming: I still rather like useSelector, but on the flip side, the poll at https://twitter.com/acemarke/status/1118952682025377792 says a lot more people like useReduxState(). Doesn't mean we have to do that, but it's a data point.
  • I did actually kinda want useRedux(selector, actions) in there because of the strong parallel with connect(), to make it as easy as possible for folks to migrate if they want. I know it's also basically a 2-liner, so anyone could write it if they so choose, but it seems reasonable to include it.
  • I have slightly mixed feelings about exporting useReduxContext(). We've danced on the line of whether or not that sort of thing is actually part of our public API. On the one hand, the "Accessing the Store" docs page explicitly says "this isn't part of our public API". On the other hand, we do export ReactReduxContext, and if that's exported, than obviously anyone can just do useContext(ReactReduxContext).

Also, not sure whether it's better to merge straight into master and publish the alpha from there, or set up a hooks-alpha branch instead and iterate on that. May be dependent on the versioned docs thing. @timdorr , @wgao19 , thoughts?


latestSelectedState.current = newSelectedState
} catch {
// we ignore all errors here, since when the component

This comment has been minimized.

Copy link
@markerikson

markerikson Apr 19, 2019

Contributor

My one concern here is swallowing errors when the component doesn't re-render.

Perhaps it's worth considering that "window of opportunity" approach from easy-peasy again?

The other downside is potentially losing some of the stack trace pointing back to the dispatched action.

@hedgerh

This comment has been minimized.

Copy link
Contributor

commented Apr 19, 2019

@markerikson

useRedux(selector, actions)

I like the idea of only needing to import one hook. Only trade-off I can think of is possible confusion or needless bikeshedding by giving people two very similar ways to do something. Not sure migration would be much more difficult by not including it.

useReduxState

I didn't like it at first, but I actually like that it's more explicit about what the hook is doing. This may be especially helpful for beginners. I was a fan of useSelect, myself.

@wgao19

This comment has been minimized.

Copy link
Contributor

commented Apr 20, 2019

Hi @markerikson,

  • To update docs for "next" version, make changes to the docs under project root's docs/ directory.
  • To update docs for "current" version, or any previous stable versions, make changes to website/versioned_docs/ in their respective subdirectories.
  • When this alpha version becomes stable release, run Docusaurus' version command to copy root docs/ into versioned_docs, and further changes should happen there from thereon.

I've checked Docusaurus' docs regarding versioned docs on other branches, it seems they cannot associate a version to a specific branch. The changes to docs should happen on master.

@MrWolfZ

This comment has been minimized.

Copy link
Contributor Author

commented Apr 20, 2019

Per the naming: I still rather like useSelector, but on the flip side, the poll at https://twitter.com/acemarke/status/1118952682025377792 says a lot more people like useReduxState(). Doesn't mean we have to do that, but it's a data point.

In my POC I also had useReduxState, so I am definitely in favor of that naming. Just let me know if you want me to change it.

I did actually kinda want useRedux(selector, actions) in there because of the strong parallel with connect(), to make it as easy as possible for folks to migrate if they want. I know it's also basically a 2-liner, so anyone could write it if they so choose, but it seems reasonable to include it.

I'll add a useRedux hook.

I have slightly mixed feelings about exporting useReduxContext(). We've danced on the line of whether or not that sort of thing is actually part of our public API. On the one hand, the "Accessing the Store" docs page explicitly says "this isn't part of our public API". On the other hand, we do export ReactReduxContext, and if that's exported, than obviously anyone can just do useContext(ReactReduxContext).

Yeah, I had the same thoughts. However, since this hook can easily be written by anyone themselves and in the spirit of keeping the exposed API minimal, I'll remove the export of this hook.

My one concern here is swallowing errors when the component doesn't re-render.

Perhaps it's worth considering that "window of opportunity" approach from easy-peasy again?

The window of opportunity (as implemented in easy-peasy) would also only throw the error if the component gets re-rendered. I still argue that for transient errors or for unmounted components it simply does not matter. The selector is a pure function without side-effects. Therefore, why would you care about an error that never has any observable effect (quantum scientists would argue that any component is always in an error state as long as you don't render it ;) )?

The other downside is potentially losing some of the stack trace pointing back to the dispatched action.

Yeah, I hadn't thought about that. I have set up a sandbox that shows how it would currently look like. In the example you don't get the dispatch in the stack trace, only the click event that triggered the dispatch. I have prototyped a solution for this here and you can see it in action in this sandbox. Basically, if in a render following an error in the subscription callback another error occurs, we merge the errors. This way you get both stack traces. What do you think?

MrWolfZ added some commits Apr 20, 2019

@markerikson markerikson changed the base branch from master to v7-hooks-alpha Apr 22, 2019

@markerikson

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

Well, the latest poll between just useSelector and useReduxState started off at about 70% for useReduxState, but has since narrowed down to a tossup - 51/49.

So, I'm inclined to stick with useSelector for now.

@theKashey

This comment has been minimized.

Copy link

commented Apr 22, 2019

Yeah, I'd still like to have the HMR tests updated at some point, but that's @theKashey 's area of expertise. I've pinged him a couple times on that and haven't gotten anything.

There would be some problems with HMR, and probably not only with it.

  • useActions has no dependency on actions, and could not have, as long as it's a unique object every time. So changing actions in runtime would not change them.
  • in the same time useSelector is "free" from this effect, as long as the "current" selector is stored in ref.

There is a simple way to mitigate useAction - use JSON.stringify(actions) as a key for useMemo(only in dev mode).
Another way to fix the problem - do nothing, as long as it's impossible to manually fix each and every library, so the problem should be fixed from the other side

@markerikson

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

@MrWolfZ : I think I was worried that there might still be a case where an error is caught, the component doesn't re-render, and the user never sees it, but I think that's handled with the catch(e) {} forceRender() sequence.

Yeah, lemme pull over the error handling changes you made for that POC.

markerikson added some commits Apr 22, 2019

Preserve stack trace of errors inside store subscription callback
Ported changes from react-redux-hooks-poc

Note: the "transient errors" test seems flawed atm.
Alter test descriptions to use string names
WebStorm won't recognize tests as runnable if `someFunc.name` is
used as the `describe()` argument.
@markerikson

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

Let's put this on an alpha branch for now. That way if we have to iterate on this any, we can still put out minor builds from master in the meantime.

I know any alpha docs do have to go in master, and that's okay, since it's versioned.

@markerikson

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

Well, green means go, right?

@markerikson markerikson merged commit a69b4f4 into reduxjs:v7-hooks-alpha Apr 22, 2019

5 of 7 checks passed

Header rules - react-redux-docs No header rules processed
Details
Pages changed - react-redux-docs All files already uploaded
Details
Mixed content - react-redux-docs No mixed content detected
Details
Redirect rules - react-redux-docs 5 redirect rules processed
Details
codecov/project 98.32% (+0.56%) compared to d4b54b5
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/react-redux-docs/deploy-preview Deploy preview ready!
Details
@markerikson

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

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

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

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

@mungojam

This comment has been minimized.

Copy link

commented Apr 22, 2019

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

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.

@MrWolfZ

This comment has been minimized.

Copy link
Contributor Author

commented Apr 22, 2019

I have created a new PR that adds deps to useSelector. Let's continue any discussion there.

@markerikson

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

I've opened up #1252 for discussion of the alpha itself. Please offer feedback there.

@markerikson

This comment has been minimized.

Copy link
Contributor

commented Apr 22, 2019

And just to be clear: WE'VE GOT AN ALPHA!

https://github.com/reduxjs/react-redux/releases/tag/v7.1.0-alpha.0

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.