-
Notifications
You must be signed in to change notification settings - Fork 11
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
Other discussions around auto-detecting Redux state usage #1
Comments
@markerikson Thanks! I'll look into those discussion. |
@theKashey's proxyequal seems great. Let me try incorporating it. |
Works like a charm. Thanks tons, @theKashey. |
😄 😃 😀 |
A deadly simple React bindings library for Redux with Hooks API |
I've been thinking a bit how this approach can be embeded in the official react-redux, but I'm not sure what would be appropriate.
Maybe, there's something I don't yet understand. There should be many edge cases, but what woud be the most critical ones? |
I would like to see some implementation details in the article.
The rest is awesome. |
I would like to write one when I got better understanding.
The original motivation of mine is to avoid memoization (which I found is a rather hard concept for beginners). I may misunderstand something. What do you mean by utilising I made a small example with
Will look into it.
Thanks. I'll try that. |
Probably the best way is not use use proxyState directly, but via |
Thanks for your suggestion.
Wonderful. Solves the general problem I have in mind.
Starting reading the code.
I got this one.
Probably because I don't get this one yet. Maybe, I'd write a failing test to better understand the issue. |
const state = {
a: {
value: 1,
b: {
value1: 1
}
}
....
affected - a.value, b.value.
... memoize
affected - a, as long it maintain eq-ref all good.
... change a.value(changes also a), b is the same
affected - a.value, b ... you know - it would always fallback to the "root node", value of which would be changed due to immutability principles, and never miss the update. Just would be not as precise, as could. So lets don't call it a problem, while it is not proven. |
Got that. Not a problem, but not as precise as it could be. Thanks. |
Just a note: based on facebook/react#14110 (comment), I ended up with the normal Redux subscriptions. I'd probably need to do some benchmarks how slow this proxy approach is compared to react-redux. |
Well, I was trying twitter-lite benchmark (I don't think it's a good one for this comparison, though), |
The idea behind is to detect a full object spread and inform dev about deoptimization, or not to track all keys, as long one is tracking a whole object (this never was done). For now you can disable it (globally) by calling |
@theKashey Thanks so much! It works. |
I run a benchmark using tree-view. react-redux 5.0.7
react-hooks-easy-redux 0.8.0
|
As a comparison, this is a dumb implementation without any bindings, but just use
|
:noooooooooooo: |
No, something is wrong. When I run the dumb version in dev mode, it's almost freezing. But in prod mode, it's faster. I'm not yet sure what's happening. |
I don't know why, but the dumb version in production mode is fast contradictory to the intuition. I'm curious about its optimization. So far, what I found about proxyequal is that |
Probably I know that's the problem, and I know the way to solve it. The ProblemYou could not establish proxy upon if (state.constructor.name === 'Object') {
const clone = Object.assign({}, state);
/// ^^ replace this by `const clone = {}`
Object.setPrototypeOf(clone, Object.getPrototypeOf(state));
return clone;
} The solutionSet a prototype for an empty object, and override getOwnKeys to emulate key existence. Just enable spreadGuardsBack, or activate these 3 lines in another way. Another solutionDetect @dai-shi - please monkey patch your version of proxyequal, and if it will help I'll ship a new version. |
I hope I don't misunderstand anything.
Although my laptop may not be very stable while running, I see only a slight change. Let me try a bit differently.
Not much difference? Hope these may help... |
Let me try to run some performance tests. Could you share your benchmark as is? |
Here you go: git clone https://github.com/dai-shi/react-redux-benchmarks.git
cd react-redux-benchmarks
npm install
npm run initialize
REDUX=5.0.7 BENCHMARKS=tree-view:tree-view-rher:tree-view-dumb npm start |
Initial:
The problem was not in change tracking, but in the change comparison, to be more concrete in the building a list of keys to compare - I've run some perf tests and found a bit quicker solution to build trie(FPS 14), and get data I need from it(20)
Still not awesome, but much better than before, as similar as
New version is published as 2.0.2. You may try on your machine. PS: without proxyequal, with an update on every change, it gives 12FPS on my machine (MBP mid 2015) |
Great work. Let me try on my end. with proxyequal v2.0.1:
with proxyequal v2.0.2:
Much better. (but, your perf test looks a bit better.)
How is this different from |
19 vs 20 FPS |
Hi @smashercosmo! |
That's something we talked about once. Having one tracker per state and switching it to different consumers. Probably it's not so dangerous. It's actually not a problem to trigger an error on track out of expected locations (ie async access), thus help customer detect a problem and wrap it with some method you may provide. |
It may not be dangerous if synchronous, but I’m not sure how to implement it in a hook. Haven’t yet read proxy-state-tree in detail, but I don’t get how to compare previous state and new one. |
Yeah, It's a bit complicated (and reactive) library. I am not sure it's interface would work for you. |
Meanwhile - @faceyspacey - you probably have something to say. |
@dai-shi nope, sorry, has zero experience in this field) Just thought it might be interesting for you. |
there's a lot going on in this thread...i just wanna say im super impressed and very happy someone finally brought this to fruition. great work @dai-shi !! The benchmarks are obviously of prime concern when it comes to this thing. If they can rival the old react-redux benchmarks, there's no reason this shouldn't become the de-facto way to use Redux. An important goal for me in the past was the ability to continue to do deep key access checking when using the rest operator: rest operator const { someKey, ...rest } = someObject
<div>rest.someValue</div> If you guys checked out Remixx from the Respond Framework you might have seen why this is important. Essentially the idea is that all component functions receive a 2nd argument for const MyComponent = (props, state, actions) => ...
const MyComponentDestructured = (props, { foo } , actions) => ...
Not having rest is akin to the controversy around hooks requiring linting rules for the scenarios it doesn't support--i.e. "solutions" having such failed edge cases should be avoided at all costs. @theKashey had mentioned the hurdles to this in the past. How are u guys viewing rest operator support now: a solid possibility that just needs to be pursued? or a road better not followed as the costs are too high? What are those costs? Any other edge cases that are currently not supported?? |
So - still there is no solution for the rest spread, except... |
I'm obviously not opposed to that. Are you 100% sure that's the only route? |
@faceyspacey Thanks! I started this project with the mind that Redux should be easier for beginners, but you implied that it could be used in a broader way. That sounds exciting. @theKashey already replied, but quoting his comment again, we are not yet ready for rest/spread. I don't know other unsupported edge cases at the moment, but I didn't obviously cover cases exhaustively. |
Meanwhile - const MyComponent = (props, state, actions) => ...
const MyComponentDestructured = (props, { foo } , actions) => ...
const RemixxComponent = (props) => {
const state = useReduxState();
const dispatch = useReduxDispatch();
return MyComponent(props, state, dispatch);
}; 🚀? |
it's very nice to see you remembered the strategy; it's even nicer to see yea, on our side destructuring rest isn't a problem. it's only a problem for users that would expect to do so, as destructuring is so common with arguments. both this and its "broader" use cases are exciting 🚀. now we just gotta lockdown:
...leaving modules out of the discussion for now, @theKashey what's your take on selectors? Do we need them, or is the work traditionally done in selectors now done in component functions like any other standard work? My initial idea was essentially preparing a complete and independent "UI Database" in the form of the combination of reducers + selectors. I've been away from Reactlandia for some time--what's your take? |
I use selectors, I love selectors for memoization, "scoping" and testability. And preparing a small update for reselect with WeakMaps and Hooks in mind. Selectors ARE the API. |
@theKashey , which is why I wanna make sure we pay special attention to selectors and truly get them right for the new proxy-based Redux world. So, here's an example where the selectors API in this setup fails us: const MyRemixxComponent = (props, state, actions) => {
const val = mySelector(state, props)
return <div>{val}</div>
}
const mySelector = memoize((state, props) => {
return state.temp === 'high' // low | medium | high
? state.highs // low and medium always returns mediums, therefore:
: state.mediums // POTENTIAL FOR UNNECESSARY RENDER
}) So react-hooks-easy-redux is only checking state. It's not making use of the intelligence within selectors, like react-redux does with HOCs. How can we make the above not trigger unnecessary renders when My original idea for Remixx was to make the const MyRemixxComponent = (props, state, actions) => {
const val = state.mySelector(props)
return <div>{val}</div>
}
And then somehow internally we produce a |
Plus - we have to hook into React.createElement, and track the factual usage. Including wrapping POD variables by object with defined [toPrimitive], thus track not the property access, but property read (only for numbers and strings :( ) |
that's what I was thinking. And by having In other words, we couple this to how re-rendering is currently prevented by react-hooks-easy-redux. It works to our advantage to have all our selectors up front. In fact, it's necessary (though we will to offer an API to add more via code-splitting).
I guess this begins to more closely describe the implementation.
I guess here's where things get out of control--ur saying the most optimum version of selector tracking includes tracking usage of return values, not just shallowEqual of return value, correct? Sounds like we have a roadmap?? |
Sounds like we should have a series of experiments. Another, not bad, option would be - const MyRemixxComponent = (props, state, actions) => {
const val = state.mySelector(props)
const render = useMemo(() => <div>{val}</div>, [val]);
return render();
} So - you will be able to bialout render by returning exactly the same return, and The memoization quality could be easily tested by Remixx, but calling a function twice. |
Es-lint can automatically discover all the values that go in the array like But I love this idea! I guess we must parse all functions called on It sounds like step 3 of the roadmap is maybe the easiest...perhaps this even addresses steps 1+2 (i.e. comparing return values to block additional rendering)!! ...actually, im wrong, it solves steps 1+2. But there's still a chance a value could be marked as a dep, but isn't in fact used in the jsx. So on top of this, we need to track access of selector-created objects. I wonder if that much more tracking has potential to be less performant. Either way, this is a great start! And in my opinion worthy of an initial release for the selectors feature. It's probably under 20% of the time that a dep is passed that isnt accessed. Maybe as low as 5%, what do u think?? |
Cmon - React team created ESLint plugin to help you provide all the stuff you need for What about usage rate of selectors - it depends on code style. Sometimes it's 😭. |
Lol, have only been studying hooks, havent used em yet. ..In our case, it's kind of annoying that its an es-lint plugin, as we likely need our babel plugin to do the same work. Maybe you have a way to streamline building this I'm not aware of. I think we only wanna require our users to install a babel plugin. Does it just provide warnings, or does it actually make the changes for you with Unless somehow we get this for free as part of a standard babel plugin integration, i think we either gotta copy it or implement it ourselves, eg: const MyRemixxComponent = (props, state, actions) => {
const val = state.mySelector('MyRemixComponent', props) // component name injected by babel plugin
return <div>{val}</div>
} And then the hierarchical cache compares cached values for this precise component and makes final decision of whether to trigger re-rendering. Also, since their could be multiple const MyRemixxComponent = (props, state, actions) => {
const ref = useRef(null)
const val1 = state.mySelector1(ref, props)
const val2 = state.mySelector2(ref, props)
return <div>{val1}, {val2}</div>
} Then our algo operates on a structure like: const refs = {
[ref]: [returnVal1, returnVal2], // compare these to return values in next potential rendering
[ref2]: [returnVal2_1, returnVal2_2]
} I feel like in the end it will be best to use our own internal infrastructure, i.e. the infrastructure that react-hooks-easy-redux is using. It's often better to reduce the # of dependencies on outside things. |
yep, something like this. |
const wrapSelector = selector => {
const fn = (state, ...args) => selector(proxy(state), ...args);
fn.getAffected = proxy.getAffected.....
return fn;
}
const createSelector = (...args) => wrapSelector(realCreateSelector(...args)); @dai-shi - you may proxy proxied state. With some extension to your |
(I'm getting to understand what you guys are trying to do.) |
yes, my friend, it's a chance for your work to be way bigger than something for "beginners" as you initially imagined it. And at the very least, it allows you to solve the |
We had very nice discussions here. I appreciate for all who participated. Sine it got long, let me close this issue now. Please discuss in more specific existing issues or file a new issue. |
There's been some other experimentation and discussion around auto-detecting which pieces of Redux state are used. In particular, @theKashey has done a lot of work around trying to use proxies, but apparently there's a number of difficult edge cases.
Please see respond-framework/remixx#1 and reduxjs/react-redux#1018 for related discussion.
If you've got ideas for how we can leverage this kind of functionality in a future version of React-Redux, I'd appreciate any help you can provide!
The text was updated successfully, but these errors were encountered: