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

Other discussions around auto-detecting Redux state usage #1

Closed
markerikson opened this issue Nov 15, 2018 · 88 comments
Closed

Other discussions around auto-detecting Redux state usage #1

markerikson opened this issue Nov 15, 2018 · 88 comments

Comments

@markerikson
Copy link

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!

@dai-shi
Copy link
Owner

dai-shi commented Nov 15, 2018

@markerikson Thanks! I'll look into those discussion.

@dai-shi
Copy link
Owner

dai-shi commented Nov 17, 2018

@theKashey's proxyequal seems great. Let me try incorporating it.

@dai-shi
Copy link
Owner

dai-shi commented Nov 17, 2018

Works like a charm. Thanks tons, @theKashey.

@theKashey
Copy link

😄 😃 😀

@dai-shi
Copy link
Owner

dai-shi commented Nov 17, 2018

A deadly simple React bindings library for Redux with Hooks API
Just wrote a short introductory article.

@dai-shi
Copy link
Owner

dai-shi commented Nov 17, 2018

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.

but apparently there's a number of difficult edge cases.

Maybe, there's something I don't yet understand. There should be many edge cases, but what woud be the most critical ones?

@theKashey
Copy link

I would like to see some implementation details in the article.
Meanwhile:

  1. Why you need bailOut hack? It would be great to utilise React.memo. Actually I am not sure this is easy doable with hooks.
  2. You are not handling some edge cases, when you are reading not a POD type, an object for example, and passing it to another component. You have to “deproxify” all the props in the result (you can do it inside bailouthack).
    See https://github.com/theKashey/memoize-state/blob/master/src/call.js
  3. You should pass a third argument to proxyState to maintain cross-render proxies-object red equality.
  4. Selectors, like reselect would be able to work properly after .3, but their memoization would break your one. Like you have 3 selectors, all reading different keys of a object. 2 got memorised and assessing only object, but one is not, and assessing the inner key - only that key would be in affected. 2 would partially fix it. I have a ongoing task for this a wrapper for memoization lib, which will replay the used keys in case of memozation.

The rest is awesome.

@dai-shi
Copy link
Owner

dai-shi commented Nov 18, 2018

article

I would like to write one when I got better understanding.

bailOutHack

The original motivation of mine is to avoid memoization (which I found is a rather hard concept for beginners).
After looking at facebook/react#14110, my hope is something like this library and I started experimenting it.

I may misunderstand something. What do you mean by utilising React.memo?

I made a small example with useMemo at https://github.com/dai-shi/react-hooks-global-state/blob/master/examples/11_deep/Person.tsx#L31-L43
Is it related at all? (BTW, this is another probject of mine with the similar motivation, but since it doesn't use Redux, there's nothing to do with this project.)

deproxify

Will look into it.

the third argument & reselect

Thanks. I'll try that.

@theKashey
Copy link

Probably the best way is not use use proxyState directly, but via memoize-state, which will handle most of edge cases.
For you - just replace one line by another. See https://github.com/theKashey/beautiful-react-redux/blob/master/src/index.js#L64

@dai-shi
Copy link
Owner

dai-shi commented Nov 19, 2018

Thanks for your suggestion.

beautiful-react-redux

Wonderful. Solves the general problem I have in mind.

memoize-state

Starting reading the code.

third argument to proxyState

I got this one.
I woner if "deproxify" is needed if this ProxyMap is kept the same across rendering.

Like you have 3 selectors, all reading different keys of a object. 2 got memorised and assessing only object, but one is not, and assessing the inner key - only that key would be in affected.

Probably because I don't get this one yet. Maybe, I'd write a failing test to better understand the issue.

@theKashey
Copy link

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.

@dai-shi
Copy link
Owner

dai-shi commented Nov 20, 2018

Got that. Not a problem, but not as precise as it could be. Thanks.

@dai-shi
Copy link
Owner

dai-shi commented Dec 13, 2018

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.

@dai-shi
Copy link
Owner

dai-shi commented Dec 22, 2018

Well, I was trying twitter-lite benchmark (I don't think it's a good one for this comparison, though),
and found it just doesn't work probably because __proxyequal_scanEnd: "this is secure guard" is enumerable. I need to learn more about this.

@theKashey
Copy link

secure guards is a complex thing, and probably a mistake. And probably I should replace them by Symbols, to create a key non-enumerable by loops, but enumerable by spread.

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 spreadGuardsEnabled(false)

@dai-shi
Copy link
Owner

dai-shi commented Dec 24, 2018

@theKashey Thanks so much! It works.

@dai-shi
Copy link
Owner

dai-shi commented Dec 24, 2018

I run a benchmark using tree-view.

react-redux 5.0.7

┌─────────┬───────────┬───────────┬──────────┬─────────────────────────────────────────────────────────────────────────────────┐
│ Avg FPS │ Scripting │ Rendering │ Painting │ FPS Values                                                                      │
├─────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────────────────────────────────────┤
│ 50.73   │ 9262.02   │ 7520.57   │ 576.89   │ 1,52,51,54,52,43,49,47,48,56,53,50,47,57,49,51,50,57,49,53,54,47,52,54,52,50,42 │
└─────────┴───────────┴───────────┴──────────┴─────────────────────────────────────────────────────────────────────────────────┘

react-hooks-easy-redux 0.8.0

┌─────────┬───────────┬───────────┬──────────┬─────────────────────────────────────────────────┐
│ Avg FPS │ Scripting │ Rendering │ Painting │ FPS Values                                      │
├─────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────┤
│ 8.37    │ 28016.50  │ 693.61    │ 51.04    │ 4,5,11,7,6,12,13,6,10,11,10,4,0,7,9,11,5,8,16,8 │
└─────────┴───────────┴───────────┴──────────┴─────────────────────────────────────────────────┘

@dai-shi
Copy link
Owner

dai-shi commented Dec 24, 2018

As a comparison, this is a dumb implementation without any bindings, but just use useEffect to subscribe the store and update for any changes.

┌─────────┬───────────┬───────────┬──────────┬──────────────────────────────────────────────────────────────────────────────────┐
│ Avg FPS │ Scripting │ Rendering │ Painting │ FPS Values                                                                       │
├─────────┼───────────┼───────────┼──────────┼──────────────────────────────────────────────────────────────────────────────────┤
│ 29.62   │ 22724.41  │ 3248.64   │ 224.59   │ 22,28,27,30,26,29,31,34,27,26,25,29,23,29,30,27,38,35,33,34,24,27,36,31,22,36,33 │
└─────────┴───────────┴───────────┴──────────┴──────────────────────────────────────────────────────────────────────────────────┘

@theKashey
Copy link

:noooooooooooo:

@dai-shi
Copy link
Owner

dai-shi commented Dec 25, 2018

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.

@dai-shi
Copy link
Owner

dai-shi commented Dec 25, 2018

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 proxyState() takes several milliseconds when an object is big. This can be seen to be fair because tree-view is an extreme example. ...What's bothering is that the dumb version actually beats the proxy version.

@theKashey
Copy link

Probably I know that's the problem, and I know the way to solve it.

The Problem

You could not establish proxy upon frozen object. That's why it clones it via Object.assign(it's slow).
https://github.com/theKashey/proxyequal/blob/master/src/index.js#L43

if (state.constructor.name === 'Object') {
    const clone = Object.assign({}, state);
   /// ^^ replace this by `const clone = {}`
    Object.setPrototypeOf(clone, Object.getPrototypeOf(state));
    return clone;
}

The solution

Set 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 solution

Detect frozen object, and skip everything for plain ones.

@dai-shi - please monkey patch your version of proxyequal, and if it will help I'll ship a new version.

@dai-shi
Copy link
Owner

dai-shi commented Dec 26, 2018

I hope I don't misunderstand anything.
What I did: const clone = {}; if (true || shouldHookOwnKeys) { ... }

before (twice):
┌─────────┬───────────┬───────────┬──────────┬──────────────────────────────────────┐
│ Avg FPS │ Scripting │ Rendering │ Painting │ FPS Values                           │
├─────────┼───────────┼───────────┼──────────┼──────────────────────────────────────┤
│ 6.63    │ 28119.50  │ 606.36    │ 44.71    │ 3,5,6,8,10,12,6,7,10,6,2,0,7,6,7,9,5 │
└─────────┴───────────┴───────────┴──────────┴──────────────────────────────────────┘
┌─────────┬───────────┬───────────┬──────────┬────────────────────────────────────────────┐
│ Avg FPS │ Scripting │ Rendering │ Painting │ FPS Values                                 │
├─────────┼───────────┼───────────┼──────────┼────────────────────────────────────────────┤
│ 7.00    │ 28459.22  │ 763.89    │ 48.56    │ 3,5,6,7,13,9,8,9,8,11,1,3,0,7,8,6,8,5,6,13 │
└─────────┴───────────┴───────────┴──────────┴────────────────────────────────────────────┘

after (twice):
┌─────────┬───────────┬───────────┬──────────┬─────────────────────────────────────────────────────┐
│ Avg FPS │ Scripting │ Rendering │ Painting │ FPS Values                                          │
├─────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────────┤
│ 8.00    │ 27645.82  │ 877.30    │ 70.92    │ 1,6,9,13,11,7,9,8,3,5,9,7,4,10,8,5,10,9,10,7,5,9,12 │
└─────────┴───────────┴───────────┴──────────┴─────────────────────────────────────────────────────┘
┌─────────┬───────────┬───────────┬──────────┬───────────────────────────────────────────────┐
│ Avg FPS │ Scripting │ Rendering │ Painting │ FPS Values                                    │
├─────────┼───────────┼───────────┼──────────┼───────────────────────────────────────────────┤
│ 7.15    │ 27425.04  │ 902.62    │ 63.44    │ 1,4,9,7,8,11,4,8,6,9,10,2,3,6,5,8,6,11,8,10,8 │
└─────────┴───────────┴───────────┴──────────┴───────────────────────────────────────────────┘

Although my laptop may not be very stable while running, I see only a slight change.


Let me try a bit differently.

with spread guards:
┌─────────┬───────────┬───────────┬──────────┬────────────────────────────────────────┐
│ Avg FPS │ Scripting │ Rendering │ Painting │ FPS Values                             │
├─────────┼───────────┼───────────┼──────────┼────────────────────────────────────────┤
│ 6.47    │ 28246.59  │ 628.99    │ 47.14    │ 3,6,8,7,4,10,9,3,9,10,4,3,0,8,7,6,10,6 │
└─────────┴───────────┴───────────┴──────────┴────────────────────────────────────────┘

with spread guards + `const clone = {};`:
┌─────────┬───────────┬───────────┬──────────┬─────────────────────────────────────────────────┐
│ Avg FPS │ Scripting │ Rendering │ Painting │ FPS Values                                      │
├─────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────┤
│ 6.71    │ 27706.79  │ 905.02    │ 70.12    │ 1,4,3,8,7,4,9,10,5,9,4,10,1,3,8,7,8,6,5,10,11,9 │
└─────────┴───────────┴───────────┴──────────┴─────────────────────────────────────────────────┘

Not much difference? Hope these may help...

@theKashey
Copy link

Let me try to run some performance tests. Could you share your benchmark as is?

@dai-shi
Copy link
Owner

dai-shi commented Dec 26, 2018

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

@theKashey
Copy link

theKashey commented Jan 2, 2019

Initial:

┌─────────┬─────────┬───────────┬───────────┬──────────┬───────────────────────────────────────────┐
│ Version │ Avg FPS │ Scripting │ Rendering │ Painting │ FPS Values                                │
├─────────┼─────────┼───────────┼───────────┼──────────┼───────────────────────────────────────────┤
│ 5.0.7   │ 6.44    │ 28581.75  │ 604.55    │ 42.48    │ 4,10,8,7,13,8,5,8,7,10,1,2,0,6,7,4,5,10,5 │
└─────────┴─────────┴───────────┴───────────┴──────────┴───────────────────────────────────────────┘

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 - collectValuables, which uses search-trie underneath.

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)

┌─────────┬─────────┬───────────┬───────────┬──────────┬────────────────────────────────────────────────────────────────────────────┐
│ Version │ Avg FPS │ Scripting │ Rendering │ Painting │ FPS Values                                                                 │
├─────────┼─────────┼───────────┼───────────┼──────────┼────────────────────────────────────────────────────────────────────────────┤
│ 5.0.7   │ 19.20   │ 22173.94  │ 2957.29   │ 223.88   │ 4,8,17,22,19,15,10,14,22,24,21,18,28,22,10,21,18,22,9,17,24,25,19,24,18,33 │
└─────────┴─────────┴───────────┴───────────┴──────────┴────────────────────────────────────────────────────────────────────────────┘

Still not awesome, but much better than before, as similar as dumb tree:

┌─────────┬─────────┬───────────┬───────────┬──────────┬─────────────────────────────────────────────────────────────────────────────────┐
│ Version │ Avg FPS │ Scripting │ Rendering │ Painting │ FPS Values                                                                      │
├─────────┼─────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────────────────────────────────────┤
│ 5.0.7   │ 20.77   │ 23343.37  │ 3144.17   │ 227.37   │ 9,23,27,24,17,22,25,30,27,28,23,21,18,21,24,17,19,23,17,14,15,21,19,18,24,10,13 │
└─────────┴─────────┴───────────┴───────────┴──────────┴─────────────────────────────────────────────────────────────────────────────────┘

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)

@dai-shi
Copy link
Owner

dai-shi commented Jan 2, 2019

Great work. Let me try on my end.

with proxyequal v2.0.1:

┌─────────┬─────────┬───────────┬───────────┬──────────┬──────────────────────────────────────┐
│ Version │ Avg FPS │ Scripting │ Rendering │ Painting │ FPS Values                           │
├─────────┼─────────┼───────────┼───────────┼──────────┼──────────────────────────────────────┤
│ 5.0.7   │ 7.69    │ 27448.93  │ 674.92    │ 47.62    │ 3,7,9,10,8,13,7,8,13,8,3,0,7,8,7,9,6 │
└─────────┴─────────┴───────────┴───────────┴──────────┴──────────────────────────────────────┘

with proxyequal v2.0.2:

┌─────────┬─────────┬───────────┬───────────┬──────────┬─────────────────────────────────────────────────────────────────────────────────┐
│ Version │ Avg FPS │ Scripting │ Rendering │ Painting │ FPS Values                                                                      │
├─────────┼─────────┼───────────┼───────────┼──────────┼─────────────────────────────────────────────────────────────────────────────────┤
│ 5.0.7   │ 15.59   │ 24285.89  │ 2009.59   │ 148.65   │ 1,10,22,25,20,23,6,15,17,13,12,18,16,12,22,23,20,7,12,14,18,17,10,15,7,15,13,19 │
└─────────┴─────────┴───────────┴───────────┴──────────┴─────────────────────────────────────────────────────────────────────────────────┘

Much better. (but, your perf test looks a bit better.)


PS: without proxyequal, with an update on every change, it gives 12FPS on my machine (MBP mid 2015)

How is this different from tree-view-dumb?

@theKashey
Copy link

How is this different from tree-view-dumb?

19 vs 20 FPS

@dai-shi
Copy link
Owner

dai-shi commented Mar 5, 2019

Hi @smashercosmo!
Looks interesting. How are you familiar with the library?

@theKashey
Copy link

That means one tree stops tracking when an other starts tracking. Since all component libraries produces their UI description synchronously, this gives a predictable behaviour.

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.

@dai-shi
Copy link
Owner

dai-shi commented Mar 6, 2019

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.

@theKashey
Copy link

Yeah, It's a bit complicated (and reactive) library. I am not sure it's interface would work for you.

@theKashey
Copy link

Meanwhile - @faceyspacey - you probably have something to say.

@smashercosmo
Copy link

@dai-shi nope, sorry, has zero experience in this field) Just thought it might be interesting for you.

@faceyspacey
Copy link

faceyspacey commented Mar 11, 2019

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 state, which inevitably will be destructured most of the time, eg:

const MyComponent = (props, state, actions) => ...
const MyComponentDestructured = (props, { foo } , actions) => ...

NOTE: in Remixx, the goal is no visible usage of hooks nor connect; store access is completely built-in and transparent; it can be accomplished via several ways: fork of React or babel plugin that wraps all standard React components.

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

@theKashey
Copy link

So - still there is no solution for the rest spread, except...
We may actually create a super simple Babel plugin, which would transform rest opartor to the something digestible by us. One line before and one line after to toggle proxies on and off.

@faceyspacey
Copy link

I'm obviously not opposed to that. Are you 100% sure that's the only route?

@dai-shi
Copy link
Owner

dai-shi commented Mar 11, 2019

@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 haven't thought much about it so far, but I will take some time to think about it as now it's clearer to me that rest/spread is important.

I don't know other unsupported edge cases at the moment, but I didn't obviously cover cases exhaustively.

@theKashey
Copy link

Meanwhile - rest was a problem for mapStateToProps realization, when you might want return/inject some slices from state as props.
With this approach, you are just "reading" from state inside component, as you wanted to do, and no need to rest.

const MyComponent = (props, state, actions) => ...
const MyComponentDestructured = (props, { foo } , actions) => ...

const RemixxComponent = (props) => {
  const state = useReduxState();
  const dispatch = useReduxDispatch();
  return MyComponent(props, state, dispatch);
};

🚀?

@faceyspacey
Copy link

faceyspacey commented Mar 11, 2019

it's very nice to see you remembered the strategy; it's even nicer to see react-hooks-easy-redux playing it's part perfectly (as in your sample implementation).

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?

@theKashey
Copy link

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.

@faceyspacey
Copy link

faceyspacey commented Mar 13, 2019

@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 state.temp changes between medium and low??

My original idea for Remixx was to make the state arg passed to your remixx components also have selectors built in, e.g:

const MyRemixxComponent = (props, state, actions) => {
  const val = state.mySelector(props)
  return <div>{val}</div>
}

on store creation, you pass reducers and selectors now, eg: createStore(reducer, selectors)

And then somehow internally we produce a state object that addresses this issue. Thoughts on how we might implement this??

@theKashey
Copy link

  • redux like mapStateToProps. A black box. But we could track what was used as inputs (property access), and what was returned. Like we know everything, and could bailout on shallow equal. But - we could return an array or an object, not just a POD type, and it may matter

  • inline state access. We might track only the first part - property access, but not to bailout if you got the same result from different values. But! Usually, you will use POD types to create elements...

  • selectors. We might track what's happening inside selector, and we might track what it will return (that should be "our" selectors). This might create something like hierarchical cache - the first level (state access) determine should MyRemixxComponent be called, while the second(selector result) may determine should MyRemixxComponent return be rejected.

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 :( )

@faceyspacey
Copy link

faceyspacey commented Mar 14, 2019

But we could track what was used as inputs (property access), and what was returned. Like we know everything, and could bailout on shallow equal.

that's what I was thinking.

And by having state.mySelectors, during createStore we have an opportunity to wire up the shallowEqual comparison of selector return values in order prevent component re-rendering.

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

This might create something like hierarchical cache - the first level (state access) determine should MyRemixxComponent be called, while the second(selector result) may determine should MyRemixxComponent return be rejected.

I guess this begins to more closely describe the implementation.

Plus - we have to hook into React.createElement, and track the factual usage

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

@theKashey
Copy link

Sounds like we should have a series of experiments. Another, not bad, option would be - useMemo

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 es-lint(not babel, not proxy!) would help properly pass down all the props you need.

The memoization quality could be easily tested by Remixx, but calling a function twice.

@faceyspacey
Copy link

faceyspacey commented Mar 14, 2019

Es-lint can automatically discover all the values that go in the array like [val]? I'm not sure what u r referring to that we get from es-lint??

But I love this idea! I guess we must parse all functions called on state, and then take the return values and place them as deps to useMemo. Is this what u have in mind?

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

@theKashey
Copy link

Es-lint can automatically discover all the values that go in the array like [val]? I'm not sure what u r referring to that we get from es-lint??

Cmon - React team created ESLint plugin to help you provide all the stuff you need for useEffect and useMemo. And also watches if you are using something you are NOT using :)

What about usage rate of selectors - it depends on code style. Sometimes it's 😭.

@faceyspacey
Copy link

faceyspacey commented Mar 14, 2019

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 eslint --fix?

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 MyRemixComponent, i guess we have to use useRef or something like this to get a handle on particular component instances. Eg:

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.

@theKashey
Copy link

yep, something like this.

@theKashey
Copy link

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 spread PR one could disable tracking on the parent proxy while selector is working to make result more pure.

@dai-shi
Copy link
Owner

dai-shi commented Mar 27, 2019

(I'm getting to understand what you guys are trying to do.)

@faceyspacey
Copy link

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 selectors issue, not just reducers. That makes your lib extremely powerful. Of course a redux fork must be used, but that's where the magic begins.

@dai-shi
Copy link
Owner

dai-shi commented Mar 31, 2019

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.

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

6 participants