dai-shi / proxy-memoize

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

Static props prevents selectors recall #100

Closed Bardiamist closed 2 weeks ago

Bardiamist commented 2 weeks ago

Hey, I have reproducible problem on big project but I still can't create small example.

Essence of the problem: proxy-memoize detects changes in the selector and recall as expected. But if this selector will additionally use some string like state.books[id].priceString (where priceString is '0') then proxy-memoize doesn't recall this selector.

Additional info: Selector uses another memoized selector. If don't memoize internal selector then no problems with '0' strings.

Any ideas?

dai-shi commented 2 weeks ago

Thanks for reporting. Does it only fail with '0' and not with '1'?

We would need a test case to fix the bug.

Bardiamist commented 2 weeks ago

Looks it fails with any string when this string not changed between calls.

Bardiamist commented 2 weeks ago

Not only strings. Also numbers and {} leads to this problem. I tied to create small repro many hours, copied many selectors. But it reproducing only on the project. Maybe will continue try tomorow. Any ideas would be helpful.

dai-shi commented 2 weeks ago

Selector uses another memoized selector.

We have some double selector tests, but the coverage may not be enough. For triple selector tests, test coverage would be lower.

I'm not sure if that helps.

Bardiamist commented 2 weeks ago

Reproduced

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

const state1 = {
  book1: {
    staticProp: '5',
    priceString: '10',
  },
};

const state2 = {
  book1: {
    staticProp: '5',
    priceString: '20',
  },
};

const selectAllBooks = memoize((state) => Object.values(state));

const selectPriceString = memoize((
  state,
) => state.book1.priceString);

const selectAdjustedPriceString = memoize((state) => {
  const priceString = selectPriceString(state);

  const problem = state.book1.staticProp;

  return priceString;
});

selectAllBooks(state1);
console.log(selectAdjustedPriceString(state1));
console.log(selectAdjustedPriceString(state2));

node version: 22.4.0 proxy-memoize version: 3.0.0

Second console.log should show 20 (not 10)

dai-shi commented 2 weeks ago

Thanks for the reproduction. #101 adds the test case.

dai-shi commented 2 weeks ago

101 should fix.

Please try: https://ci.codesandbox.io/status/dai-shi/proxy-memoize/pr/101 ☝️ Find "Local Install Instructions"

Bardiamist commented 2 weeks ago

I use yarn 4 so used yarn add proxy-memoize@https://pkg.csb.dev/dai-shi/proxy-memoize/commit/4aac6ae2/proxy-memoize/_pkg.tgz

This case fixed, thanks :tada:

But my initial problem still exists :cry: I'll back if will find way to create a repro.

Bardiamist commented 2 weeks ago

You can start from this

Simplified example ``` const memoize = require('proxy-memoize').memoize; const state1 = { booksState: { booksObject: { 'book1-name': { shortName: 'book1-name', }, }, }, extendedBooksState: { extendedBooksObject: { 'book1-name': { staticProp: '5', priceString: '10', book: { shortName: 'book1-name', }, }, }, }, }; const state2 = { booksState: { booksObject: { 'book1-name': { shortName: 'book1-name', }, }, }, extendedBooksState: { extendedBooksObject: { 'book1-name': { staticProp: '5', priceString: '0', book: { shortName: 'book1-name', }, }, }, }, }; const selectPriceString = memoize(( state, ) => state.extendedBooksState.extendedBooksObject['book1-name'].priceString); const selectAdjustedPriceString = memoize((state) => { const { booksState: { booksObject, }, extendedBooksState: { extendedBooksObject, }, } = state; const book = booksObject['book1-name']; book.shortName; // touch the prop const priceString = selectPriceString(state); extendedBooksObject['book1-name'].staticProp; // touch the prop return priceString; }); const selectAllExtendedBooks = memoize(( state, ) => Object.values(state.extendedBooksState.extendedBooksObject)); const selectBookThroughExtendedBooks = memoize((state) => selectAllExtendedBooks(state)[0].book); const selectMemoizedPriceString = memoize((state) => selectPriceString(state).priceString); const getPriceStringSelector = ( book // looks as touch ) => ( state, ) => selectMemoizedPriceString(state); getPriceStringSelector(selectBookThroughExtendedBooks(state1))(state1); console.log(selectAdjustedPriceString(state1)); getPriceStringSelector(selectBookThroughExtendedBooks(state2))(state2); console.log(selectAdjustedPriceString(state2)); ```

Second expected output is 0

Bardiamist commented 2 weeks ago

Here is extended example which more close to my problem

Extended example ``` const memoize = require('proxy-memoize').memoize; const state1 = { booksState: { booksObject: { 'book1-name': { shortName: 'book1-name', }, }, }, extendedBooksState: { extendedBooksObject: { 'book1-name': { staticProp: '5', priceString: '10', book: { shortName: 'book1-name', }, }, }, }, }; const state2 = { booksState: { booksObject: { 'book1-name': { shortName: 'book1-name', }, }, }, extendedBooksState: { extendedBooksObject: { 'book1-name': { staticProp: '5', priceString: '0', book: { shortName: 'book1-name', }, }, }, }, }; const state3 = { ...state1 }; // if use state1 then it will not work const selectPriceString = memoize(( state, ) => state.extendedBooksState.extendedBooksObject['book1-name'].priceString); const selectAdjustedPriceString = memoize((state) => { const { booksState: { booksObject, }, extendedBooksState: { extendedBooksObject, }, } = state; const book = booksObject['book1-name']; book.shortName; // touch the prop const priceString = selectPriceString(state); extendedBooksObject['book1-name'].staticProp; // touch the prop return priceString; }); const selectAllExtendedBooks = memoize(( state, ) => Object.values(state.extendedBooksState.extendedBooksObject)); const selectBookThroughExtendedBooks = memoize((state) => selectAllExtendedBooks(state)[0].book); const selectMemoizedPriceString = memoize((state) => { const { booksState: { booksObject, }, } = state; const book = booksObject['book1-name']; const priceString = selectPriceString(state); book.shortName // touch the prop just in case return priceString; }); const getPriceStringSelector = (book) => ( state, ) => { book.shortName; // touch the prop just in case return selectMemoizedPriceString(state); }; console.log(selectAdjustedPriceString(state1)); getPriceStringSelector(selectBookThroughExtendedBooks(state1))(state1); getPriceStringSelector(selectBookThroughExtendedBooks(state2))(state2); console.log(selectAdjustedPriceString(state2)); getPriceStringSelector(selectBookThroughExtendedBooks(state3))(state3); console.log(selectAdjustedPriceString(state3)); ```

Third expected output is 10

Strange that if use state1 instead of state3 then it has affect

dai-shi commented 2 weeks ago

Okay, let me merge #101 and tackle the new one separately.

Simplified example

Can you simplify it a bit more? A test case should be small, and a minimal reproduction helps to investigate the bug.

Bardiamist commented 2 weeks ago

I simplified as possible

dai-shi commented 2 weeks ago

Yeah, but my gut feeling is there should be a smaller repro to reveal the bug. Maybe, it has to be created from a different angle, instead of simplifying the current code.

dai-shi commented 2 weeks ago

I haven't read it very carefully yet, but one possibility is double nested memoize works, but triple nested memoize can fail.

Bardiamist commented 2 weeks ago

Yeah, but my gut feeling is there should be a smaller repro to reveal the bug.

You can try to remove something then bug will stop to reproduce

Maybe, it has to be created from a different angle, instead of simplifying the current code.

Rare case. Was difficult to find related selectors and call in right order. Simplified as possible and anonymized data.

I haven't read it very carefully

Should be easy to read on books example

one possibility is double nested memoize works, but triple nested memoize can fail.

Better if selectors will be recalled than return wrong data

dai-shi commented 2 weeks ago

You can try to remove something then bug will stop to reproduce

Done: #102

dai-shi commented 2 weeks ago

Please open a new issue if you still find a bug.

Bardiamist commented 2 weeks ago

Cool, I tested, all problems fixed, looks good, thank you! :tada:

dai-shi commented 2 weeks ago

That's great to hear.

FYI, here's the simplified repro: https://github.com/dai-shi/proxy-memoize/blob/e2aa105310e71df13f901bb7d701d6ee2d9aa27c/tests/issue_100.spec.ts#L53-L87