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

Use provided memoize options in outer memoize call #359

Closed
wants to merge 2 commits into from
Closed

Use provided memoize options in outer memoize call #359

wants to merge 2 commits into from

Conversation

montemishkin
Copy link

There are two memoize calls in createSelectorCreator. One is to memoize the so called resultFunc. The other memoize allows us to skip out early if the resulting selector is called with the exact same arguments.

Currently, this second memoize uses the caller's provided memoize implementation, but does not pass the caller's memoizeOptions. This PR changes it to pass the memoizeOptions.

There are two memoize calls in `createSelectorCreator`.  One is to memoize the so called `resultFunc`.  The other memoize allows us to skip out early if the resulting selector is called with the exact same arguments.  

Currently, this second memoize uses the caller's provided `memoize` implementation, but does not pass the caller's `memoizeOptions`.  This PR changes it to pass the `memoizeOptions`.
assert.equal(selector({ a: 2 }), 1)
assert.equal(selector.recomputations(), 1)
// again returns `1` since `typeof state` is still "object"
assert.equal(selector({ a: 'A' }), 1)
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its worth highlighting that this is a subtle change in behavior and could break dependent codebases. So bumping a version number seems to be in order

@codecov-io
Copy link

codecov-io commented Jul 27, 2018

Codecov Report

Merging #359 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #359   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          15     15           
=====================================
  Hits           15     15
Impacted Files Coverage Δ
src/index.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fda697a...f038cb2. Read the comment docs.

@PavelDemyanenko
Copy link

Any news on this request? Looks good to me

@Zhuvikin
Copy link

Faced the same issue. Is it planned to be merged?

@ms88privat
Copy link

ms88privat commented Dec 10, 2019

I just investigated a related topic. I really think we should pass two memoize and two options to this selectorCreator, so everybody can decide what they want exactly. They should not depend on each other whatsoever. I was not happy when #297 was introduced. This forced me to do an expensive isEqual check on the broad input instead of the very lightweight outcome for the input of the result function. @ellbee What do you think about that?

@yoavniran
Copy link

facing the same issue exactly. passing memoizeOptions to the sub selectors makes a lot of sense, or at least being able to control this with a flag so it can be opt it

@oasisvali
Copy link

oasisvali commented Feb 12, 2020

For those looking for a workaround in the meantime, I've found using the following pattern to be helpful:

const uniformMemoize = (func) => _.memoize(func, hashFn);

const createUniformMemoizeSelector = createSelectorCreator(
  uniformMemoize,
);

A nice side-effect of using this is that it unlocks the ability to use different memoization strategies for the overall selector arguments vs output function arguments. e.g.

// second argument only exists when the output function is memoized
const differentMemoize = (func, isOutput) => {
  if (isOutput) { return _.memoize(func, hashFn) }
  return defaultMemoize(func);
}

// this creates selectors with overall selector argument cache = defaultMemoize (cache size 1)
// output function argument cache = _.memoize w/ hashFn (cache size unlimited)
const differentMemoizeSelector = createSelectorCreator(
  differentMemoize,
  true,
);

@markerikson markerikson modified the milestones: 4.1, 5.0 Oct 17, 2021
aryaemami59 added a commit to aryaemami59/reselect that referenced this pull request Nov 11, 2023
- Add documentation regarding new features.
- Redo `CodeSandbox` examples to align better with documentation. Solves reduxjs#598.
- Fix GitHub workflow badge URL reduxjs#628.
- Add instructions for `bun` and `pnpm`. Solves reduxjs#621.
- Fix minor type issues regarding `createStructuredSelector`. Solves reduxjs#499.
- `argsMemoize` and `argsMemoizeOptions` solve reduxjs#359.
- Remove `Redux` legacy patterns including `connect` and `switch` statements from docs.  Solves reduxjs#515.
- Replace legacy code patterns with modern syntax like `useSelector` and functional components.
- Implementation of `argsMemoize` solves reduxjs#376.
- Document order of execution in `Reselect`. Solves reduxjs#455.
- Add benchmarks to confirm the info inside the documentation.
- Add more type tests with `vitest`.
- Fix more hover preview issues with types.
@aryaemami59 aryaemami59 mentioned this pull request Nov 11, 2023
9 tasks
@markerikson
Copy link
Contributor

v5 allows separate memoize and argsMemoize customization options

@markerikson markerikson closed this Dec 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants