Skip to content

Commit

Permalink
fix: no extra calculation with nested memoized selectors (#92)
Browse files Browse the repository at this point in the history
* add failing test

* minor tweak

* two test cases

* yaaaay!

* update proxy-compare

* update LICENSE
  • Loading branch information
dai-shi authored Jan 15, 2024
1 parent a3ce890 commit 551ae15
Show file tree
Hide file tree
Showing 6 changed files with 90 additions and 7 deletions.
2 changes: 2 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
# Change Log

## [Unreleased]
### Changed
- fix: no extra calculation with nested memoized selectors #92

## [2.0.4] - 2023-05-06
### Changed
Expand Down
2 changes: 1 addition & 1 deletion LICENSE
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
The MIT License (MIT)

Copyright (c) 2020-2023 Daishi Kato
Copyright (c) 2020 Daishi Kato

Permission is hereby granted, free of charge, to any person obtaining a copy
of this software and associated documentation files (the "Software"), to deal
Expand Down
57 changes: 57 additions & 0 deletions __tests__/issue_91_spec.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
import { memoize } from '../src/index';

describe('no extra calculations (#91)', () => {
it('nested memoized selectors with primitive value access', () => {
const reduxState = {
books: {
bookByName: {
bookPrice1: 50,
bookPrice2: 100,
},
},
};
let selectAllBooksCount = 0;
const selectAllBooks = memoize((state: typeof reduxState) => {
selectAllBooksCount += 1;
return Object.values(state.books.bookByName);
});
const selectSomeBooks = memoize((state: typeof reduxState) => {
const someBooks = selectAllBooks(state);
return someBooks.filter((price) => price === 100);
});
const selectBooks = memoize((state: typeof reduxState) => {
const allBooks = selectAllBooks(state);
const someBooks = selectSomeBooks(state);
return { allBooks, someBooks };
});
selectBooks(reduxState);
expect(selectAllBooksCount).toBe(1);
});

it('nested memoized selectors with object access', () => {
const reduxState = {
books: {
bookByName: {
book1: { price: 50 },
book2: { price: 100 },
},
},
};
let selectAllBooksCount = 0;
const selectAllBooks = memoize((state: typeof reduxState) => {
selectAllBooksCount += 1;
return Object.values(state.books.bookByName);
});
const selectSomeBooks = memoize((state: typeof reduxState) => {
const someBooks = selectAllBooks(state);
return someBooks.filter((book) => book.price === 100);
});
const selectBooks = memoize((state: typeof reduxState) => {
const allBooks = selectAllBooks(state);
const someBooks = selectSomeBooks(state);
return { allBooks, someBooks };
});
selectBooks(reduxState);
expect(selectAllBooksCount).toBe(1);
});
});
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@
],
"license": "MIT",
"dependencies": {
"proxy-compare": "2.5.1"
"proxy-compare": "2.6.0"
},
"devDependencies": {
"@types/jest": "^29.5.1",
Expand Down
20 changes: 16 additions & 4 deletions pnpm-lock.yaml

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

14 changes: 13 additions & 1 deletion src/memoize.ts
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,12 @@ const touchAffected = (dst: unknown, src: unknown, affected: Affected) => {
});
};

const isOriginalEqual = (x: unknown, y: unknown): boolean => {
for (let xx = x; xx; x = xx, xx = getUntracked(xx));
for (let yy = y; yy; y = yy, yy = getUntracked(yy));
return Object.is(x, y);
};

// properties
const OBJ_PROPERTY = 'o';
const RESULT_PROPERTY = 'r';
Expand Down Expand Up @@ -105,7 +111,13 @@ export function memoize<Obj extends object, Result>(
for (let i = 0; i < size; i += 1) {
const memo = memoList[(memoListHead + i) % size];
if (!memo) break;
if (!isChanged(memo[OBJ_PROPERTY], obj, memo[AFFECTED_PROPERTY], new WeakMap())) {
if (!isChanged(
memo[OBJ_PROPERTY],
obj,
memo[AFFECTED_PROPERTY],
new WeakMap(),
isOriginalEqual,
)) {
touchAffected(obj, memo[OBJ_PROPERTY], memo[AFFECTED_PROPERTY]);
resultCache?.set(obj, memo);
return memo[RESULT_PROPERTY];
Expand Down

0 comments on commit 551ae15

Please sign in to comment.