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

Different objects with the same arguments #30

Open
pavelzubov opened this issue Jun 13, 2019 · 9 comments
Open

Different objects with the same arguments #30

pavelzubov opened this issue Jun 13, 2019 · 9 comments

Comments

@pavelzubov
Copy link

pavelzubov commented Jun 13, 2019

I have noticed a regularity: if you try to return in the function the object which is an argument of that function, than each time a new object returns (although the argument hasn't changed regarding the library's work).

const aFunction = (state) => Math.random();
const memoFunc = memoize(aFunction);
const result1 = memoFunc({a: 1, b: 2});
const result2 = memoFunc({a: 1, b: 2});
console.log(Object.is(result1, result2)) // true
const aFunction = (state) => state;
const memoFunc = memoize(aFunction);
const result1 = memoFunc({a: 1, b: 2});
const result2 = memoFunc({a: 1, b: 2});
console.log(Object.is(result1, result2)) // false?..

Even if the object is inside another object:

const aFunction = (state) =>({n: Math.random()});
const memoFunc = memoize(aFunction);
const result1 = memoFunc({a: 1, b: 2});
const result2 = memoFunc({a: 1, b: 2});
console.log(Object.is(result1, result2) && Object.is(result1.n, result2.n)) // true
const aFunction = (state) =>({n: Math.random(), data: state});
const memoFunc = memoize(aFunction);
const result1 = memoFunc({a: 1, b: 2});
const result2 = memoFunc({a: 1, b: 2});
console.log(Object.is(result1, result2) && Object.is(result1.n, result2.n)) // false?..

Is it bug or a feature?

@theKashey
Copy link
Owner

That's a feature. You are returning a reference to an input value, declaring that you need that object, and thus it would be added to a dependencies list.
As long as dependency changes (a new value provided) - cache is dropped.

How to "fix"

const aFunction = (state) => ({...state});

return a copy of the object. Like "I dont need the object, I need only values inside it". Does not work for nested objects :)

@pavelzubov
Copy link
Author

But the main idea of memoization - storing the results of function call and return them with a same input values.
In the examples we see that the results are not storing. The function returns different values for the same inputs.
If the function has same value in input, it must return exactly the same value. It should not depend on a dependencies list and others. This is the meaning of memoization.
Reference comparison is very important in frameworks, it is used for optimise update (React.memo and onPush strategy in Angular).

@theKashey
Copy link
Owner

Not exactly

const obj = {
 a: 1,
};
const result = useMemo(() => obj, []); // eslint: Hey, missing dependency
const result = useMemo(() => obj, [obj]); // eslint: ok
const result = useMemo(() => obj, [obj.a]); // eslint: ok

memoizestate creates dependenct list in runtime

useMemo(() => obj.a, [obj.a]) === memoizestate( state => state.a);
useMemo(() => obj, [obj]) === memoizestate( state => state);

While with useMemo you can use not quite real dependency list, memoizestate would always buld the true one.

Dependency is something you are using inside the function. Or something you are returning from the function.

Once again - let's check your example

const aFunction = (state) =>({n: 42, data: state});
const memoFunc = memoize(aFunction);
// why it is working that way
const result1 = memoFunc({a: 1, b: 2}); // {n: 42, data:state}
const result2 = memoFunc({a: 1, b: 2}); // {n: 42, data:state}

// but here it would be ok?
const result1 = memoFunc(5); // {n: 42, data:5}
const result2 = memoFunc(6); // {n: 42, data:6}

state is a new object every call, thus bypasses memoization.

@pavelzubov
Copy link
Author

pavelzubov commented Jun 14, 2019

state is a new object every call

No. According to the description of your library the object should be treated as the same.
And examples confirm this:

const aFunction = (state) => Math.random();
const memoFunc = memoize(aFunction);
const firstObject = {a: 1, b: 2};
const secondObject = {a: 1, b: 2};
const result1 = memoFunc(firstObject);
const result2 = memoFunc(secondObject);
console.log(Object.is(result1, result2)) 
// if the result is stored than the firstObject 
// and the secondObject are the same object, right?

Your problem is that for one case the objects are considered the same, and for the second they are different.
And it should not depend on the implementation of the React hooks.

@theKashey
Copy link
Owner

You don't get the point - the object should be treated as the same until they are become a part of a result. That's working ok if the original state is immutable - there would be no "unexpected" new objects.

  • 👍 generate a derived data from incoming object.
  • 👎 dont derive data, but return it as is.

I could make this behaviour configurable - that's a one line fix - if you need this behavior, but I would have to ask you - what do you actually need.

To be more concrete - on data return memoizestate is going thought returned information, and removing proxies if they exists, and detecting using the part of the original object as a side effect.
I could not track that usage, or I might not remove proxies, thus not stoping "observing" that you do actually looking for inside the data.

@pavelzubov
Copy link
Author

pavelzubov commented Jun 14, 2019

Can you give examples, how I can get "unexpected" new objects if I will be receive same object in a state => state function (if the object should be treated as the same always, even if it is part of a result)?

@theKashey
Copy link
Owner

Structural sharing which is base layer for immutable structures means that if object updated - it is updated, and if you are passing a new object - that's a new object.
However this library is solving exactly this problem - when change in underlaying state mutates all the parents, and that change might be ignored.

But not always:

const map = new Map();
const key1 = {a:1};
map.set(key1, 1);

const aFunction = (state) => state;
const memoFunc = memoize(aFunction);

const memoKey1 = memoFunc(key1);
expect(map.get(memoKey1)).toBe(1);

const key2 = {a:1}; // the same shape!
map.set(key2, 2);
const memoKey2 = memoFunc(key2);
expect(map.get(memoKey2)).toBe(2); // it is expected

more if it - if you are not assessing any properly it yet again will think that you need "all of it"

const aFunction = (state) => ({ x: state.a }); // would update ONLY when .a updated
const aFunction = (state) => ({ x: state.a, state }); // when .a, AND state updated
const aFunction = (state) => ({ x: 42, state }); // only state
const aFunction = (state) => (state); // only state

So, let's start everything from the beginning - what is the case you are trying to solve? I've written 4 memoization libraries, and might be you need "not this one".

@pavelzubov
Copy link
Author

pavelzubov commented Jun 14, 2019

And I am confused by the logic of working with objects in arguments.
I pass different objects (both by content and by reference), but memoizestate assumes that they are the same.

const aFunction = (state) => Math.random()
const memoFunc = memoize(aFunction);
const result1 = memoFunc({a: 1, b: 2});
const result2 = memoFunc({});
console.log(Object.is(result1, result2)) // true?..

const aFunction = (state) => Math.random()
const memoFunc = memoize(aFunction);
const result1 = memoFunc({a: 1, b: 2});
const result2 = memoFunc(1);
console.log(Object.is(result1, result2)) // false?..

@theKashey
Copy link
Owner

I am not sure I have tests for zero-usage cases, so for me it would be not only an undefined behavior, but also a useless one.
Once again - the idea of the library is about automatical key usage tracking, or to be concrete - tracking of parts the calculation process depends on or the result was formed from.

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