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

Investigate adding a proxy-based selector API #1502

Closed
markerikson opened this issue Jan 20, 2020 · 19 comments
Closed

Investigate adding a proxy-based selector API #1502

markerikson opened this issue Jan 20, 2020 · 19 comments

Comments

@markerikson
Copy link
Contributor

markerikson commented Jan 20, 2020

Both our connect and useSelector APIs rely on the user manually specifying what pieces of state to extract to determine re-renders.

@dai-shi has done some amazing work with using ES6 Proxies to manage tracking the pieces of state that are actually needed by a component, particularly in his https://github.com/dai-shi/reactive-react-redux library.

I've said several times that I kinda see that lib as a sort of alpha test for whether an approach like that should be added to React-Redux itself. Now that our hooks API has been stable for a while, it might be time to start discussing that idea more seriously.

This may also have some relevance to the Concurrent Mode questions over in #1351 .

Ideally, anything we add would be completely tree-shakeable by default, so that it doesn't increase bundle size unless the API is actually used.

@dai-shi
Copy link
Contributor

dai-shi commented Jan 20, 2020

One of the biggest concerns was about performance because Proxies have some overhead.
During last year, I spent much effort on it and reached a reasonable point.
There's a benchmark result here: reduxjs/react-redux-benchmarks#26

@dai-shi
Copy link
Contributor

dai-shi commented Jan 20, 2020

As for CM, there's some relevance because reactive-react-redux tries it.
But technically, proxy-based API (useTrackedState in rrr) is nothing to do with CM, which means we can apply it to the current react-redux v7.

@timdorr
Copy link
Member

timdorr commented Jan 20, 2020

Can we just adopt useTrackedState with some minor changes? @dai-shi Would you be OK with that?

@dai-shi
Copy link
Contributor

dai-shi commented Jan 20, 2020

You mean like react-redux v7.2? Yes, that should be possible. I'm certainly OK with it.

(Well, I'll draft a PR anyway.)

@salvoravida
Copy link

hi! @dai-shi
first of all, great work with rrr!

i would like to get a point:

  • it is not clear from the user-point-of-view which are the main advg of using useTrackedState instead of useSelector

in summary, we are replacing
//useSelector.js

  if (equalityFn(newSelectedState, latestSelectedState.current)) {
          return
        }

with
//useTrackedState.js

if (
        latestTracked.current.state !== nextState &&
        isDeepChanged(
          latestTracked.current.state,
          nextState,
          latestTracked.current.affected,
          latestTracked.current.cache
        )
      ) {
        forceRender()
      } 

the main drawback is that user cannot use selectors, and so derivated states.

am i missing something?

@markerikson
Copy link
Contributor Author

Yeah, like I said in the PR:

We'll need documentation:

  • How does this get used?
  • Why would you want to use it?
  • What are the differences in behavior compared to useSelector?
  • What are the limitations in browser support?

In general, I see it as a mostly simpler, higher-level abstraction alternative to useSelector. Don't even need to write a selector, just say "give me the Redux state and let me read from it".

The main points of concern are the caveats listed at https://github.com/dai-shi/reactive-react-redux#caveats .

@salvoravida
Copy link

salvoravida commented Jan 23, 2020

yes i agree.
it could be named "useReduxState" that sound more clear.
and the only-once-use for component could be checked by an eslint-rule.

But the most drawback i read there is 'Proxied state shouldn't be used outside of render' because
dispatch({ type: 'FOO', value: state.foo }); it's something common.

Nowdays, people think that Redux is complicated, so the main goal is "keep it simple!" 🎉

@timdorr
Copy link
Member

timdorr commented Jan 23, 2020

FWIW, we decided against the useReduxState name early on in the planning process for hooks.

@markerikson
Copy link
Contributor Author

Hmm. Another good question is, what do the TS typings for this look like?

@salvoravida
Copy link

salvoravida commented Jan 23, 2020

useTrackedState<TState>() =>TState.?

@dai-shi
Copy link
Contributor

dai-shi commented Jan 23, 2020

useTrackedState() =>TState.?

Yeah. https://github.com/dai-shi/reactive-react-redux/blob/7c2e705930e6cca2eab33b60864d840c4e10c532/src/index.d.ts#L29

the main advg of using useTrackedState instead of useSelector

For beginners: Far easier to understand Redux and R-R without the notion of selectors

For intermediates: Never needs to worry about memoized selectors

For experts: No stale props issue

the main drawback is that user cannot use selectors, and so derivated states.

Correct. It can't provide the same behavior as this:

const isYoung = useSelector(state => state.person.age < 12);

But, that doesn't mean users cannot use selectors. They can use them, but don't get the same behavior. (extra re-renders)

The other way of saying would be selectors in R-R have two purposes: separation of concerns and render optimization.

You could create useSelector from useTrackedState.

const useTrackedSelector = (selector) => selector(useTrackedState());

Proxied state shouldn't be used outside of render

Honestly, I don't know what will happen if we leak Proxies outside render.
There might not be any big problems in most of use cases.

Technically, only the problem from the useTrackedState side is Proxying already Proxied objects.
This can be avoided by always unwrapping objects before wrapping with a new Proxy.
It's actually the approach taken by @theKashey 's proxyequal.

so the main goal is "keep it simple!"

Well, believe it or not, that's my major motivation for useTrackedState.
If you don't concern the magic behind it too much, it's just an optimized version of useStoreState that could be implemented in a couple of lines.

@theKashey
Copy link

Ideally useTrackedState should also report an "illegal" uses, when to many entities are being tracked.
Personally, for the last year, I am using tracked selectors mostly to "prove" than my manual selectors are working correctly, and will continue doing it correctly and performant(!) without tracking as well.

@eddyw
Copy link

eddyw commented Feb 11, 2020

I'm wondering if this would mean dropping IE11 support 🙈 (no Proxies on IE11) or how the fallback API may look like 🤔

@dai-shi
Copy link
Contributor

dai-shi commented Feb 11, 2020

There was a debate before, and proxy-polyfill is not included now. So, it's up to developers to add proxy-polyfill for IE11 support and RN support. There's also caveats when using the polyfill. (See dai-shi/reactive-react-redux#30 )

@timdorr
Copy link
Member

timdorr commented Feb 11, 2020

You can also use useSelector just the same as before, which doesn't rely on Proxies to work. This API is entirely additive, so it will show up in a minor release.

@user753
Copy link

user753 commented Feb 13, 2020

Does anyone know if it would be possible to achieve something similar with babel macros?

const state = useSelector(state => state)
return <div>state.foo</div>

rewrite it with babel

const state_foo = useSelector(state => state.foo)
return <div>state_foo</div>

it would have zero runtime cost.

@dai-shi
Copy link
Contributor

dai-shi commented Feb 13, 2020

I have thought about it before too. The build time approach would be interesting.

Let me point out one difference with the runtime approach.

const fooOrBar = useSelector(state => isFoo ? state.foo : state.bar); // isFoo comes from props

This simple case might be possible in the build time, but more complex case would be difficult.

@markerikson
Copy link
Contributor Author

Hiya, folks. At this point, I don't think we're likely to add a specific new proxy-based selector API to React-Redux. We do have a mention of proxy-memoize in https://redux.js.org/usage/deriving-data-selectors , and if we ever get around to adding a selectors page to the React-Redux docs I'm happy to highlight it there as well.

I'm going to go ahead and close this.

@dai-shi
Copy link
Contributor

dai-shi commented Apr 13, 2022

In case someone misses it, here's my final comment in the PR: #1503 (comment)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

7 participants