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

Fight the Spread #9

Open
theKashey opened this issue Apr 19, 2018 · 2 comments
Open

Fight the Spread #9

theKashey opened this issue Apr 19, 2018 · 2 comments

Comments

@theKashey
Copy link
Owner

theKashey commented Apr 19, 2018

As continue of #5

Full object spread is a big problem - from point of view it is a performance killer, as long all object fields are set to be trackable, from another - this is why we emit a warning asking a user to reconsider selector, and that is not nice.

First version of "spread detection" just disables tracking during massive spread operation. Now - only record a fact, as long we have to emit a new "result" if any of fields got updated.

But how memoize-state tracks the keys?

  • "By use". there is a proxy get hook, to trap any access to a property. This is how we know about something got accessed.
  • "By result". it analyse the function result in search of values just passed through, then memoize-state have to deproxify result, replacing Proxies by original values. This is but sorting array input does nothing - it will be replaced by the original object, as long it represents it.

Also

  • there is "magic" field, "scan(Start|End)Guard", added to each object to detect spread.

Missing

The idea:

What if add a special flag, to deproxify result outside of function call? Just split the calculation and the data you are going to pass throught.

Also we have to keep in mind - some spreads, are 100% "legal", as long "things just works that way". For example <Element {...props} /> - all the props are passed down to nested component.

Approach number 1

So "by result" will track the keys from "result" to be replaced by real values from "input". We still could have function memoized, but keeping "spreaded" paths updated.

const fn = state => memoize({...state, value: state.a + state.b});
// "returns" deproxifyed result {...state, value}
// should react only to .a or .b changes. But react on all props, recalculating everything.

const fn = state => memoize({...state, value: state.a + state.b}, {lazy: true});
// "returns", "raw" result {...Proxy(state), value)
// Will react only to .a or .b changes. "unwapping" proxies to the "current" values on each call.

As long object Objects and Arrays got proxies - not work for simple states. Meanwhile - could handle almost all the cases.

Approach number 2

Create a special function like Object.assign, to perform this operation without extra magic. "scanGuards" is the magic, I'll better keep of, but without them, we could not distinguish when values were "used", and when just "returned"

cons fn = state => ({...state, a: state.a, b:state.a+1});
fn({a:1, b:2});

spreaded, returned(used), used. If value was spreaded or returned - it still could be used.
Only b should be omitted.

Approach number 3

const fn = state => memoizedFlow(state, state => ({value: state.a + state.b});
// will produce "expected" behavior, as long it has `state = ({...state, fn(state)})` internally, outside of memozation tracking.

But, as long the goal of this library is to be "magic" - we might things about a better way.
@alex-pex
Copy link

alex-pex commented Aug 24, 2018

The spread detection is triggered in a case I don't understand

I have an app state with the following part :

{
  referentials: {
    clubs: {
      isLoading: false,
      ids: [
        {
          id: '/resamania/clubs/1',
          schema: 'Club'
        },
        {
          id: '/resamania/clubs/2',
          schema: 'Club'
        }
      ]
    },
    activities: {
      isLoading: true,
      ids: []
    },
    studios: {
      isLoading: true,
      ids: []
    },
    coaches: {
      isLoading: true,
      ids: []
    }
  }
}

And a selector like this

/**
 * Returns the current state of referential loading
 *
 * @param {object} state
 * @returns {boolean}
 */
export const getIsLoadingSomething = memoize(state =>
  Boolean(Object.values(getReferentials(state)).find(value => value.isLoading))
);

I'm getting

memoize-state: object spread detected in state => Boolean(Object.values(getReferentials(state)).find(value => value.isLoading)) . Keys affected: [".referentials"] . This is no-op.


What I understand is that I'm iterating over all referential state and tho I'm killing optimization, but I don't know how I can do things differently. I don't want to hardcode the list of referentials. This is killing the magic of memoize-state a bit!

I read this #5 (comment) but still don't understand

@theKashey
Copy link
Owner Author

To say the truth - the current behaviour of spread detection is wrong - Full spreads or full enumerations are absolutely legal operations.

I am looking forward to disabling it.

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

2 participants