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

Update createSelector and defaultMemoize to accept options (maxSize, equalityCheck, resultEqualityCheck) #513

Merged
merged 12 commits into from
Oct 20, 2021

Conversation

markerikson
Copy link
Contributor

@markerikson markerikson commented Oct 19, 2021

This PR:

  • Updates the createSelector types to accept an optional options object as the last argument
  • Defines the options object as having a memoizerOptions field that exactly matches the type of all of the provided memoize function's arguments after the initial function-to-be-memoized, with exact type inference of all arguments
  • Further modifies the memoizerOptions field to optionally be just the first of the memoize function's additional arguments, to support the most common cases of configuring a memoize function (such as passing a custom equality comparison function, or whatever options object the memoization lib supports)
  • Extracts defaultMemoize to its own file
  • Updates defaultMemoize to accept either of an equalityCheck function or an options object as its first argument
  • Rewrites defaultMemoize to add support for a customizably-sized cache with LRU behavior, based on https://github.com/erikras/lru-memoize
  • Adds an optional resultCheckEquality comparison function to allow reusing the most recent cached value that matches a recalculated value (the "todos.map(todo => todo.id) is still shallow equal after toggling a todo" use case)

This makes the following usages possible:

    const defaultMemoizeAcceptsArgsAsArray = createSelector(
      (state: StateAB) => state.a,
      (state: StateAB) => state.b,
      (a, b) => a + b,
      {
        memoizerOptions: [
          // equalityCheck, first arg
          (a, b) => a === b
        ]
      }
    )


    const defaultMemoizeAcceptsFirstArgDirectly = createSelector(
      (state: StateAB) => state.a,
      (state: StateAB) => state.b,
      (a, b) => a + b,
      {
        // equalityCheck, first arg, no wrapping array!
        memoizerOptions: (a, b) => a === b
      }
    )

    const defaultMemoizeAcceptsFirstArgDirectly = createSelector(
      (state: StateAB) => state.a,
      (state: StateAB) => state.b,
      (a, b) => a + b,
      {
        memoizerOptions: {
          equalityCheck: (a, b) => a === b,
          maxSize: 3,
          resultEqualityCheck: shallowEqual
        }
      }
    )

These changes should solve a majority of the complaints users have had with Reselect:

  • You no longer need to call createSelectorCreator just to customize the equality comparison
  • It now supports cache sizes > 1, configurable per-instance
  • A a previous result value that is considered equal to a freshly-generated result can be reused.

And all of this should be entirely backwards-compatible with existing code :)

Example of the latter case from the defaultMemoize tests:

    for (let maxSize of [1, 3]) {
      let funcCalls = 0

      const memoizer = defaultMemoize(
        (state: Todo[]) => {
          funcCalls++
          return state.map(todo => todo.id)
        },
        {
          maxSize,
          resultEqualityCheck: shallowEqual
        }
      )

      // Initial calculation
      const ids1 = memoizer(todos1)
      expect(funcCalls).toBe(1)

      // Same input, cache hit, reuse last result
      const ids2 = memoizer(todos1)
      expect(funcCalls).toBe(1)
      expect(ids2).toBe(ids1)

      // Different input, but value is shallow equal to the last result
      const ids3 = memoizer(todos2)
      expect(funcCalls).toBe(2)
      expect(ids3).toBe(ids1)
    }

@codecov
Copy link

codecov bot commented Oct 19, 2021

Codecov Report

Merging #513 (ee1500d) into master (6bc2b7a) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master      #513   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         2    +1     
  Lines           53        94   +41     
  Branches         7        21   +14     
=========================================
+ Hits            53        94   +41     
Impacted Files Coverage Δ
src/defaultMemoize.ts 100.00% <100.00%> (ø)
src/index.ts 100.00% <100.00%> (ø)

@markerikson markerikson force-pushed the feature/createselector-options branch from 4c07b61 to 89ff857 Compare October 19, 2021 05:30
src/defaultMemoize.ts Outdated Show resolved Hide resolved
(...args: SelectorResultArray<Selectors>) => Result
]
...items:
| [...Selectors, (...args: SelectorResultArray<Selectors>) => Result]
Copy link
Contributor Author

Choose a reason for hiding this comment

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

💬 this is so stupid and ugly, but it works (when an optional object at the end of the tuple didn't)

Copy link

Choose a reason for hiding this comment

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

Whoaaa. Don't recall ever seeing : | [ in that order before.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hah, yeah. My first attempt was just throwing an optional object into the tuple, but that totally broke everything.

I just sort of randomly thought "what if I make it a union of two exact tuples instead", and to my shock and amazement that actually worked. Immediately.

Copy link

@phoenixeliot phoenixeliot Dec 20, 2022

Choose a reason for hiding this comment

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

I really don't need to bother leaving this comment, but for anyone curious, :\n| ...\n| ... is how Typescript does multi-line type-or expressions, just prefix each line with a |. It's like trailing commas in a list.

@markerikson
Copy link
Contributor Author

Also just remembered that extracting another file will break the Babel changes I made in the last couple days. Have to compile that file too.

Copy link

@erikras erikras left a comment

Choose a reason for hiding this comment

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

It kills me to see all those anys in there. But if you're using a single cache structure for multiple Entrys, I guess you just can't know.

Glad to see some of this old code (that still has 44k downloads/month) getting a new lease on life. 👏

return false
}

// Do this in a for loop (and not a `forEach` or an `every`) so we can determine equality as fast as possible.
Copy link

Choose a reason for hiding this comment

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

Noice!

src/defaultMemoize.ts Outdated Show resolved Hide resolved
(...args: SelectorResultArray<Selectors>) => Result
]
...items:
| [...Selectors, (...args: SelectorResultArray<Selectors>) => Result]
Copy link

Choose a reason for hiding this comment

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

Whoaaa. Don't recall ever seeing : | [ in that order before.

@markerikson
Copy link
Contributor Author

Yeah, upon further thought I think I can switch most or all of the any in the cache logic to unknown.

The amusing thing is I just copy-pasted the code, and all I had to do was whip up a pair of interfaces for Entry and Cache... and it just worked despite you having written the original code in Flow! :)

(helps that the types were really simple in the first place of course, but still neat)

createSelector now accepts an options object as its last argument.
For now, the only field in that object is `memoizeOptions`.  This
field represents whatever the additional parameters are for the
supplied memoization function.

If this options object is supplied, `createSelector`will try to
use the `memoizeOptions` field as the params to `memoize()`.  If
the object exists but no `memoizeOptions` field, it will fall back
to whatever memoizer options were passed as args to the original
`createSelectorCreator` call.

Further complicating things: normally, `memoizeOptions` is an array
containing all the individual args. For example, `defaultMemoize`
currently takes one options arg: the `equalityCheck` function used
for comparisons.  So, to pass that directly here, you'd do:

createSelector(
  [input1, input2],
  output,
  { memoizeOptions: [ equalityCheck ] }
)

But, it's very common to want to _only_ use the _first_ options
arg, and many libs have _only_ one arg anyway (like defaultMemoize
does right now).  So, to simplify usage, we also allow passing
that first arg directly in `memoizeOptions` without an array
around it:

createSelector(
  [input1, input2],
  output,
  { memoizeOptions: equalityCheck }
)

Internally, this is done by checking to see if `memoizeOptions` is
an array or not. If it's an array, we use it as-is. If not, we
assume it must be "the first options arg" and wrap it in an array
so it can be spread via `apply()`.

This does mean that a lib that takes an array as its first options
arg will break things here, but I can't immediately see any libs
that do that - most either take `equalityCheck` or an object.

Besides, the goal of this effort is that `defaultMemoize` will
handle enough use cases that's all you'd typically use anyway.
@markerikson markerikson force-pushed the feature/createselector-options branch from 89ff857 to ee1500d Compare October 20, 2021 00:33
@markerikson
Copy link
Contributor Author

markerikson commented Oct 20, 2021

Replaced a bunch of any with unknown, and fixed up the build config.

Also renamed memoizerOptions to memoizeOptions (no 'r') for consistency.

@@ -110,5 +110,8 @@
"ts-jest": "26.5.6",
"typescript": "^4.4.0"
},
"dependencies": {}
"dependencies": {
"memoize-one": "^6.0.0-beta.1",

Choose a reason for hiding this comment

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

6.0.0 has just been released 🎉

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

Successfully merging this pull request may close these issues.

4 participants