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

Parameterized selectors #1

Open
jacobrabjohns opened this issue Feb 21, 2019 · 6 comments
Open

Parameterized selectors #1

jacobrabjohns opened this issue Feb 21, 2019 · 6 comments

Comments

@jacobrabjohns
Copy link

Can you please provide me with an example of how you are using the function provided? I am struggling to infer usage, and it doesnt appear to be anywhere within your tests etc.

Thanks :)

@dtinth
Copy link
Collaborator

dtinth commented Mar 11, 2019

@jacobrabjohns

Sorry, I missed this notification.

Something like this:

const selectUserById = makeParameterizedSelector('selectUserById', userId => {
  return query => {
    return query(state => state.users[id])
  }
})

The selector can be used like this:

selectUserById(1)(state)
selectUserById(2)(state)
selectUserById(3)(state)

with the property that identity is preserved:

selectUserById(1) === selectUserById(1) // returns the same function instance

@OliverJAsh
Copy link
Contributor

Hey @dtinth, what do you think about including the definition of makeParameterizedSelector in this library?

@OliverJAsh
Copy link
Contributor

OliverJAsh commented Jan 24, 2020

makeParameterizedSelector keys selectors with:

      const key = args.join(',')

However, I don't think this is safe because not all arguments can be uniquely stringified:

[{ a: 1 }, { b: 1 }].join(',')
// => "[object Object],[object Object]"

For this reason I wonder whether it would be better for makeSelector to allow selectors to receive extra parameters, and then check those for equality in addition to the existing dependencies check. Example excerpt and usage:

     function select(state: State, ...extraParams: ExtraParams): Result {
       if (cachedResult) {
+        // Check if extra params have changed
+        let changed = false
+        for (const [index, value] of cachedResult.extraParams.entries()) {
+          if (extraParams[index] === value) {
+            changed = true
+          }
+        }
         // Check if dependencies changed
         for (const [selector, value] of cachedResult.dependencies.entries()) {
           if (selector(state, ...extraParams) !== value) {
             changed = true
             reason = selector
             break
           }
         }
       }
     }
const selectUserById = makeSelector((query, userId) => {
  return query(state => state.users[id])
})

selectUserById(state, myUserId);

This also means users could use makeSelector to create all types of selectors: parameterized and non-parameterized.

WDYT @dtinth?

@dtinth
Copy link
Collaborator

dtinth commented Jan 28, 2020

@OliverJAsh Thanks for your opinion! Sorry, I missed the notification, that’s why it took me so long to reply. Feel free to ping me on Twitter (@dtinth).

Yes I agree having makeParameterizedSelector in would make the library more convenient to use. However it also means that the API surface area that this library has to support will grow. This will increase the maintenance cost for us. That’s why I try to keep this library as bare-bones as possible, which are:

  • Dynamic dependency tracking. This is the gap this library is intended to fill.
  • Runtime introspection (see the setWrapper test, this allows us to augment rereselect with extra performance instrumentation logic without having to release new versions. At Taskworld, we need to integrate this tightly into our React/Redux stack so that we can trace which selectors are recomputed or executed as part of rerendering caused by Redux action).

So for this project, flexibility comes over convenience.

I would be very happy if someone makes a higher-level library that makes use-cases such as parameterized selectors more convenient.

In reply to your proposal,

  • We at @taskworld use TypeScript, so we typed the selectorFactory like function selectorFactory<T extends (string | number)[]>(...args: T) {. This eliminates the join problem.

  • An issue with allowing extraParams is that each set of parameter passed to the selector needs its own cache. Otherwise when a selector is called multiple times with different arguments, the cache will never be hit, which defeats the purpose of selector memoization. That’s where cache keys come in, and that’s why I had to serialize them into strings.

@OliverJAsh
Copy link
Contributor

Thanks for the reply @dtinth!

  • An issue with allowing extraParams is that each set of parameter passed to the selector needs its own cache. Otherwise when a selector is called multiple times with different arguments, the cache will never be hit, which defeats the purpose of selector memoization. That’s where cache keys come in, and that’s why I had to serialize them into strings.

Isn't the solution to this to use factories, e.g. so we can have one selector instance per component? E.g. in reselect:

const makeSelectUserById = () => createSelector(
  getUsersById,
  (state, userId) => userId,
  (users, id) => {
    return users[id]
  }
)

const selectUserById1 = makeSelectUserById(); // this has its own cache
const selectUserById2 = makeSelectUserById(); // this has its own cache

We could use the same idea with my proposal above:

const makeSelectUserById = () => makeSelector((query, userId) => {
  return query(state => state.users[id])
})

const selectUserById1 = makeSelectUserById(); // this has its own cache
const selectUserById2 = makeSelectUserById(); // this has its own cache

In that case, if a selector receives a different argument, then we deliberately want to recompute.

  • At Taskworld, we need to integrate this tightly into our React/Redux stack so that we can trace which selectors are recomputed or executed as part of rerendering caused by Redux action).

Wow, I'd love to hear more about this, because currently we have little visibility over how well our selectors are memoized, which in turn means we have little visibility over how efficient/inefficient our React re-renders are. Are you able to share any more information about the setup you have?

@dtinth
Copy link
Collaborator

dtinth commented Jan 29, 2020

@OliverJAsh Factories is one solution but the lifetime the selector-cache depends on the lifecycle of the component that uses it.

Global memoization vs per-component memoization (factories)

Consider a fictional case in a project management app where we need a selector, parameterized by project ID, that takes the following input selectors:

  • list of tasks by project
  • tags (for filtering, as needed)
  • users and groups (for filtering, as needed)
  • current user’s preferences
  • active notifications

and outputs a summary of suggestions to users what they should focus on today.

If this selectors happens to be used by 2 components, then this summary must be computed twice each time the store is updated. That’s why we create a global memoization-cache instead, which is what makeParameterizedSelector does. Therefore, it looks like this:

const selectSummaryByProjectId = _.memoize((projectId: string) => {
  return makeSelector(query => {
    // ...
  })
})

Now, multiple components that uses the same projectId for selectSummaryByProjectId(projectId) will be accessing the same selector-cache.

The drawback is that without a memoization-cache eviction strategy, this global memoization-cache will keep growing as more distinct projectId values are used, which could lead to memory leaks. To fix this makeParameterizedSelector function as shown in the README needs two extra features:

  1. Introspection, e.g. exposing a way to see how many distinct selectors are being memoized inside the memoization-cache.
  2. Memoization-cache eviction strategy. e.g. a LRU cache.

To summarize, makeParameterizedSelector:

  • Assumes a global memoization-cache.
  • Does not support component-level memoization-cache.
  • Only supports string-based parameters (which is enough for us).
  • Lacks features that allows introspection and cache eviction.
  • I’m not even sure if this would make a good API for others.

So that’s why I did not include it in the library.

React-Redux-rereselect integration

The basic flow is this:

  1. Something happens (user interaction or network event).
  2. An action is dispatched to Redux store.
  3. react-redux is notified that the store state is updated.
  4. Selectors are executed.

Here’s how we tie 4 back to 2 (and sometimes 1):

  • React has an Interaction Tracing API that allows binding re-renders to an interaction. This will be visible in React profiler as well.

  • We integrated that API into Redux. We implemented it directly, but a middleware is also available.

  • That means while the selector is being executed (as part of a re-render) we can access the current interaction using scheduler’s unstable_getCurrent function. This allows us to trace the recomputation of selector back to its originating interaction by wrapping the makeSelector API:

    const makeNamedSelector = (selectorName, selectionLogic) => {
      return makeSelector(query => {
        const currentInteractions = Tracing.unstable_getCurrent()
        PerformanceDebugger.recordSelectorReduxInteraction(
          selectorName,
          currentInteractions
        )
        return selectionLogic(query)
      })
    }
  • For example, and if we notice that the MessageUpdated Redux action caused selectOnlineUsers to be re-executed, then we did not memoize that selector properly. This gave us visibility into what we wouldn’t have seen otherwise.

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

No branches or pull requests

3 participants