dai-shi / proxy-memoize

Intuitive magical memoization library with Proxy and WeakMap
http://proxy-memoize.js.org/
MIT License
719 stars 16 forks source link

Extra calculations #91

Closed Bardiamist closed 10 months ago

Bardiamist commented 11 months ago

Hello, in my app I do big calculations and some parts of code memoized. I noticed that these memoized calculations runs many times during one redux state change. After long time I catched problem and created minimal repro:

const memoize = require('proxy-memoize').memoize;

const reduxState = {
  "books": {
    "bookByName": {
      "book1": {
        "price": "50",
      },
      "book2": {
        "price": "100",
      }
    }
  }
};

const selectAllBooks = memoize((state) => {
  console.log('selectAllBooks');

  return Object.values(state.books.bookByName);
});

const selectSomeBooks = memoize((state) => {
  let someBooks = selectAllBooks(state);

  return someBooks.filter((book) => book.price === 100);
});

const selectBooks = memoize((state) => {
  const allBooks = selectAllBooks(state);
  const someBooks = selectSomeBooks(state);

  return {
    allBooks,
    someBooks,
  };
});

selectBooks(reduxState);

I expect that selectAllBooks should be called once but it called two times in this example. Can you fix it?

Bardiamist commented 11 months ago

Checked all proxy-memoize versions. Problem appeared in 2.0.1.

dai-shi commented 11 months ago

Thanks for reporting.

Problem appeared in 2.0.1.

So, the change is #60.

I expect that selectAllBooks should be called once but it called two times in this example.

How does it behave if you remove memoize from selectBooks?



const selectBooks =(state) => {
  const allBooks = selectAllBooks(state);
  const someBooks = selectSomeBooks(state);

  return {
    allBooks,
    someBooks,
  };
};
``
Bardiamist commented 11 months ago

If don't memoize selectBooks then selectAllBooks runs 2 times. But if keep selectBooks memoized and don't memoize selectSomeBooks then selectAllBooks runs 1 time.

dai-shi commented 11 months ago

Thanks. I think I understand the issue.

dai-shi commented 11 months ago

Spent a few hours investigating this. It seems this requires quite a bit of work.

Bardiamist commented 11 months ago

I reverted to 1.0.0 for now

Because: 1.1.0 and 2.0.0 has performance problems https://github.com/dai-shi/proxy-memoize/issues/59 2.0.1 doesn't trigger selectors when required https://github.com/dai-shi/proxy-memoize/issues/63#issuecomment-1404744324 And now we see that 2.0.2, 2.0.3 and 2.0.4 triggers selectors when not required

Hope everything will be fixed and tests will be added

dai-shi commented 11 months ago

I think #91 is technically fixable, but maybe not for others. v1.0.0 doesn't behave correctly for some cases. Feel free to give a try.

Bardiamist commented 10 months ago

v1.0.0 doesn't behave correctly for some cases.

What wrong with 1.0.0? We have some problem and I suspect that sometimes selectors not trigges then they should be.

Hope in the next version all problems will be resolved.

dai-shi commented 10 months ago

Did you try #92? Are all problems resolved?

Bardiamist commented 10 months ago

Tried

"proxy-memoize": "https://pkg.csb.dev/dai-shi/proxy-memoize/commit/5a2f830c/proxy-memoize",

from https://ci.codesandbox.io/status/dai-shi/proxy-memoize/pr/92

looks good :tada:

dai-shi commented 10 months ago

https://www.npmjs.com/package/proxy-memoize/v/2.0.5