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

[Feature request] Customizable control over when a selector triggers re-render #1416

Open
BenjaBobs opened this issue Nov 13, 2021 · 31 comments
Labels
enhancement New feature or request help wanted Extra attention is needed performance

Comments

@BenjaBobs
Copy link

Proposal

I would like to be able to specify a custom function for cache comparison on an atom/selector so I can tweak the behaviour.
Let's say we have a configurable algorithm with multiple steps, and some options that pertain to a step each.

const algorithmOptions = atom({
	key: 'algorithmOptions',
	default: {
		featureA: true,
		featureB: true,
		featureC: false,
	},
});

Now, only step 2 options are relevant for the step 2 of the algorithm, so we make a selector to fetch only that

const step2Options = selector({
	key: 'step2Options',
	get: ({get}) => {
		const allOptions = get(algorithmOptions);

		return {
			allOptions.featureB,
			allOptions.featureC,
		};
	},
	// overwrite cache comparison, so we don't trigger unnecessary rerenders by modifying algorithmOptions.featureA
	cacheCompare: (old, new) => old.featureB=== new.featureB && old.featureC === new.featureC,
})

That way, we don't trigger an unnecessary rerender when doing setOptions({ ...options, featureA: !options.featureA }).

Motivation

This can currently be done using an intermediate selector, but it's a lot of boiler plate and makes the code look clunky.
Here's a code sandbox showing the intermediate selector pattern: https://codesandbox.io/s/busy-cherry-u4rx6?file=/src/App.tsx

The real life use cases for this are when you get a state object from a backend, which you want to split out into multiple parts, or if you have a form for configuration of something, and you want to save the state of the form as is.

@drarmstr drarmstr added help wanted Extra attention is needed enhancement New feature or request performance labels Nov 13, 2021
@pc-erin
Copy link

pc-erin commented Nov 22, 2021

This is an urgently needed feature.
Without it any compound atom triggers a rerender even when no constituent value has changed.
If that atom is something that changes frequently, like mouse position, it has a big impact on application performance.

@mondaychen
Copy link
Contributor

In the example, any reason why you can't use three separate atoms, or an atomFamily as feature flags?

@pc-erin
Copy link

pc-erin commented Nov 23, 2021

Not who you were asking, but I have an example that involves manipulating Points ({ x: number, y: number }), and it wouldn't make much sense to store the x and y values in separate atoms since they will only ever be accessed together.

It also wouldn't help with the issue where selectors trigger rerenders even when there are no updates because the derived state is always a new object.

export const viewportMousePositionState = atom<Point>({
	key: 'viewportMousePosition',
	default: ZERO_POINT
})

export const gridMousePositionState = selector<Point>({
	key: 'gridMousePosition',
	get: ({ get }) => {
		const gridViewportPosition = get(gridViewportPositionState)
		const viewportMousePosition = get(viewportMousePositionState)
		const gridOffset = get(gridOffsetState)
		const zoomLevel = get(zoomLevelState)

		return {
			x: ((viewportMousePosition.x - gridViewportPosition.x) / zoomLevel) - gridOffset.x,
			y: ((viewportMousePosition.y - gridViewportPosition.y) / zoomLevel) - gridOffset.y
		}
	},
	cachePolicy_UNSTABLE: {
		eviction: 'most-recent'
	}
})

export const gridSnappedMousePositionState = selector<Point>({
	key: 'gridSnappedMousePosition',
	get: ({ get }) => {
		const mousePosition = get(gridMousePositionState)

		return {
			x: Math.round(mousePosition.x),
			y: Math.round(mousePosition.y)
		}
	},
	cachePolicy_UNSTABLE: {
		eviction: 'most-recent'
	}
})

In this example any component that consumes gridMousePositionState or gridSnappedMousePositionState rerenders on every "tick" because the get method outputs are considered to be a new object on every run and there's no way to tell the memoizer how to know if two Point objects are equal.

@ghost
Copy link

ghost commented Nov 27, 2021

I think this has been a long-standing issue with Recoil #314 and solving it will make this the go-to state management system for React.

@drarmstr
Copy link
Contributor

drarmstr commented Dec 2, 2021

As was mentioned, this can currently be addressed via intermediate selectors from #314. Though, it makes sense to improve the API for this without intermediate selectors.

API options to consider:

  • custom comparator for a selector to compare it's output value to determine if it should propagate updates downstream.
  • custom comparator for a selector to compare dependencies to determine if it should re-evaluate
  • custom comparator provided at individual callsites, such as from hooks or selector get() to determine if it should re-render or re-evaluate.

@pc-erin
Copy link

pc-erin commented Jan 5, 2022

@drarmstr If you're taking votes I'd go with option 1, but applied to both atoms and selectors.

Seems like it would scale the best. Any atom/selector that has lots of dependents or changes frequently can just have an isEqual option added to it and all dependent selectors and components automatically get the benefit of it. It also makes sense with the semantics of atoms as self-contained units of state. Who knows the significance of an atom's value better than the atom itself?

Ex:

const viewportMousePositionState = atom<Point>({
  key: 'viewportMousePosition',
  default: ZERO_POINT,
  isEqual: arePointsEqual,
})

const gridMousePositionState = selector<Point>({
  key: 'gridMousePosition',
  get: ({ get }) => { ... },
  isEqual: arePointsEqual,
})

function arePointsEqual(a: Point, b: Point): boolean {
  return a.x === b.x && a.y === b.y
}

I'm not sure how you would implement solution 2 without complicating the selector options. You'd need to pass a deps array like for hooks that specifies the depended on atom/selector, along with the comparator function every time you want to use it.

Ex:

const gridMousePositionState = selector<Point>({
  key: 'gridMousePosition',
  get: ({ get }) => {
    const gridViewportPosition = get(gridViewportPositionState)
    const viewportMousePosition = get(viewportMousePositionState)
    const gridOffset = get(gridOffsetState)
    const zoomLevel = get(zoomLevelState)
    ...
  },
  deps: [
    { state: viewportMousePositionState, isEqual: arePointsEqual },
    { state: gridViewportPositionState, isEqual: arePointsEqual },
    // etc
    // Seems like a lot more boilerplate, and I'm not sure what benefit it offers.
  ]
})

Similar thoughts for option 3: more boilerplate, not clear why you would need different comparators at different call sites.

Ex:

const gridMousePositionState = selector<Point>({
  key: 'gridMousePosition',
  get: ({ get }) => {
    const gridViewportPosition = get(gridViewportPositionState, arePointsEqual)
    const viewportMousePosition = get(viewportMousePositionState, arePointsEqual)
    const gridOffset = get(gridOffsetState, arePointsEqual)
    const zoomLevel = get(zoomLevelState)
    ...
  }
})

function ExampleComponent() {
  const gridMousePosition = useRecoilValue(gridMousePositionState, arePointsEqual)
  ...
}

@BenjaBobs
Copy link
Author

If it were me, I'd go for a mix of option 1 and 3.
Custom comparator at the selector/atom level, since I think this is going to be the most common use case.
And then I'd provide custom comparator as an optional parameter in GetRecoilValue, to override the native custom comparator of the recoil value.

@FokkeZB
Copy link

FokkeZB commented Feb 12, 2022

NOTE: We later learned we were talking about 2 different use cases.

@drarmstr I think this aligns with a need we have, so posting here but LMK if this should be a separate request.

We (Zapier) have a need to be able to use some information from other selectors/atoms in a selector's get, without trigger the selector to re-evaluate when their values change.

A concrete example is that while for an API request made in a selector we need set X, we only want to re-evaluate the selector and as part of that redo the API request when Y⊂X (a subset of X) changes, or when we manually tell it to (using useRecoilRefresher).

I'd personally lean towards your option three, as in:

const oneState = selector({
  key: 'one',
  get: async ({ get }) => {
    // This we need and want to "subscribe" to (register as dependency), so that we re-evaluate when its value changes
    const two = get(twoState);

    // This we need, but don't want to re-evaluate for
    const three = get(threeState, { subscribe: false });   

    return fetch(url, { data: { two, three } });
  }
});

Why I'd prefer this is that to me any call to get() represents creating a dependency on another value, so it makes sense to right there inform Recoil on whether it should indeed be considered a "hard" dependency that we subscribe to, or a "soft" one that we just need the information from.

I realize that for the original request, this still requires an intermediate selector to select either value from algorithmOptions, but I don't think that's a downside as I think it's more clear than an often hard to read compare function that has to deal with the values of all get()'s, of which some might be called conditional - making it even more complex.

On the implementation side I wonder if this is as easy as skipping the following line when subscribe === false:

setNewDepInStore(store, state, deps, depKey, executionId);

@drarmstr
Copy link
Contributor

@FokkeZB If you just want to read a value of an atom/selector without subscribing to it then you can use useRecoilCallback().

@FokkeZB
Copy link

FokkeZB commented Feb 12, 2022

NOTE: We later learned we were talking about 2 different use cases.

@drarmstr I'm aware of that, but notice how in my use case I need to read values inside of a selector's get, which means I cannot use hooks.

Is your suggestion to do something like this perhaps?

const oneState = selector({
  key: 'one',
  get: async ({ get, getCallback }) => {
    // This we need and want to "subscribe" to (register as dependency), so that we re-evaluate when its value changes
    const two = get(twoState);

    // This we need, but don't want to re-evaluate for
    const unsubscribedGet = getCallback<Array<RecoilValue<any>>, any>(
      ({ snapshot }) => async (recoilVal) => snapshot.getPromise(recoilVal)
    ) as <T>(recoilVal: RecoilValue<T>) => Promise<T>;

    const three = await unsubscribedGet(threeState);

    return fetch(url, { data: { two, three } });
  }
});

That feels hacky to me, and also fails with:

Error: Callbacks from getCallback() should only be called asynchronously after the selector is evalutated.  It can be used for selectors to return objects with callbacks that can work with Recoil state without a subscription.

@BenjaBobs
Copy link
Author

As I mentioned, the mix of option 1 and 3 would grant enough flexibility for everyone to be able to solve for their use cases, as well as being being a simple solution that doesn't get in your way, what with it being optional. I haven't read the code for triggering the subscribtion notifications, so I don't know if it's particularly easy to implement though, but I think it's the best solution.

@FokkeZB
Copy link

FokkeZB commented Feb 13, 2022

NOTE: We later learned we were talking about 2 different use cases.

What I don't like about option 1 is that the puts the comparator at the producer, not the consumers (!) of values.

I might have multiple consumers of the same value, and some of them might want to re-evaluate when the consumed value changes, while others might not.

@BenjaBobs
Copy link
Author

Yes, but by putting both options in the hands of the developer, with option 3 taking precendence over option 1, you can save a lot of code for the cases where you want the condition to be the same everywhere. You could absolutely achieve the same results of option 1 by using option 3 in all consumers, option 1 just saves you some work, and the worey that if a new dev is attached to the project, they'll forget to set the cache comparing function in the new place where they need to consume the state.

I have no evidence for this, but my gut feeling is that the most common use case is the one that option 1 solves, and the one that option 3 solves is more rare.

@FokkeZB
Copy link

FokkeZB commented Feb 13, 2022

NOTE: We later learned we were talking about 2 different use cases.

About:

the worey that if a new dev is attached to the project, they'll forget to set the cache comparing function in the new place where they need to consume the state.

In contrast, I worry that when consuming a value I might be puzzled when I notice that my selector doesn't always re-evaluate when I know the consumed value has changed. Then only by looking at the implementation of the consumed value will I find out why.

Also technically I think option 3 is much easier to implement, and I don't think there's any use case that option 1 can solve, but option 3 can't. And since AFAIK the goal is to keep the API to a minimum, I think option 3 makes more sense.

@BenjaBobs
Copy link
Author

BenjaBobs commented Feb 13, 2022

In contrast, I worry that when consuming a value I might be puzzled when I notice that my selector doesn't always re-evaluate when I know the consumed value has changed.

I don't think you (usually) should be think about where a selector gets its data from, but instead just what data it promises to deliver.

If someone made a selector with a custom caching logic, it was probably for a good reason, and hopefully it's self apparent from the domain (e.g. a selector for getting the mouse X coordinate) or documented by the creator of the selector.

Also technically I think option 3 is much easier to implement, and I don't think there's any use case that option 1 can solve, but option 3 can't.

I think that's probably correct.

And since AFAIK the goal is to keep the API to a minimum, I think option 3 makes more sense.

This is a good point, and I'll admit that my main motivation for also wanting option 1 (with option 3 overriding) is a combination of laziness on my part, and worry that consumers of my state will forget to use the custom caching logic I wrote, that makes my selector behave as I intented.

I'm aware that I can synthetically create option 1 by creating a selector (A) which I don't export, and then create another selector (B) which I do export, which returns the data from A but with a custom caching logic.
But the whole reason I opened this issue is that I wanted a simpler and easier to understand/maintain way to manage custom caching logic.

EDIT:
Also, on the documentation issue, I know that it's naive of me to assume that people will document their state properly, and I admit that is a potential footgun.
I think the trade off is worth.
Thoughts?

And @drarmstr has your team had a chance to mull over the feedback here since you're initial post about the three options?

@FokkeZB
Copy link

FokkeZB commented Feb 13, 2022

NOTE: We later learned we were talking about 2 different use cases.

To make sure I understand your original question correctly; what does old and new refer to in your cacheCompare?

As I'm reading it this is @drarmstr's option 2, where cacheCompare is called every time any of step2Options's dependencies their values change, with old holding their old values and new their new values. But what if step2Options does more get() calls, some of which perhaps conditionally? I don't see how we could keep cacheCompare readable.

I already explained why option 1 doesn't appeal to me either because in this case algorithmOptions would have a function to determine that a change of featureA would not cause any dependees to re-evaluate. What if I add a new selector that needs algorithmOptions and does want to be re-evaluate when featureA changes?

IMO option 3 is the only reasonable option, because here we determine whether a change of value from a dependency should trigger us to re-evaluate right where we create the dependency by calling get().

I also think this should take the form of a complex comparator function, but simply a boolean. If only part of the value we get() should trigger us to re-evaluate, we can use intermediate selectors to get us both parts and set the flag on just the one that we don't want to trigger a re-evaluation.

Yes, this might feel boilerplaty, but IMO it keeps us closer to the data-flow graph idea. Think of option 3's boolean as controlling whether a line in the graph should be solid (triggering re-evaluation) or dashed (not triggering re-evaluation).

@BenjaBobs
Copy link
Author

BenjaBobs commented Feb 13, 2022

In my original post, my example was basically @drarmstr's option 1, where old was the previously calculated return value, and new is a newly calculated one.

I agree with your critique of option 2, I think it'll be too complex and difficult to control.

I already explained why option 1 doesn't appeal to me either because in this case algorithmOptions would have a function to determine that a change of featureA would not cause any dependees to re-evaluate. What if I add a new selector that needs algorithmOptions and does want to be re-evaluate when featureA changes?

Then I think you should have your selector/component depend on algorithmOptions instead of step2Options.
I firmly believe it is step2Options's responsibility to declare when it's result is a new value, or not a new value.
Option 1, as I see it, best encapsulates this responsibility.
The reason I would like like option 3, and for that to be able to override option 1, is that I think it's always good to provide as much control as possible, but to have some simple defaults.

EDIT:
To exemplify what I mean about the responsibility, here's an example piggybacking of @pc-erin's example:

// state which supplies a curser position
export const curserPosition = atom({
	key: 'curserPosition',
    default: [0, 0],
	// imagine that this effect sets the value when the mouse is moved
    // so very often, and in small increments
    effects: [updateCurserPositionEffect],
});

Let's assume we want to export similar state that snaps to a grid of size const grideSize = 5;
Using option 1, we can do this:

export const curserPositionSnapped = selector({
	key: 'curserPositionSnapped',
	get: ({ get }) => {
		// get the mouse position
		const position = get(curserPosition);

		// calculate snapping points
		return [
			Math.round(position[0] / gridSize) * gridSize,
			Math.round(position[1] / gridSize) * gridSize,
		]
	},
	// I made up the name 'cacheOptions' because we there are already other options for the cache
	cacheOptions: {
		// super verbose name which I hope explains my intentions for it
		defaultDownstreamTriggerComparer: (old, new) => old[0] !== new[0] || old[1] !== new[1],
	}
});

If any consumer of curserPositionSnapped were to decide that they wanted to receive updates every time there was it calculated its result, we would get a bunch of updates like this:

[0, 0],
[0, 0],
[0, 0],
[0, 0],
[0, 0],
[0, 0],
[0, 0],
[5, 0],
[5, 0],
[5, 0],
[5, 0],
[5, 5],

instead of

[0, 0],
[5, 0],
[5, 5],

Causing a lot more rerenders than necessary.
I believe this goes against the intention of the curserPositionSnapped state.
Much like service locator pattern vs dependency injection pattern.

@FokkeZB
Copy link

FokkeZB commented Feb 14, 2022

Ah.... I thought #314 had (also) address this example use case, but I gave it a try at https://codesandbox.io/s/recoil-playground-jkvq7?file=/pages/invalidation-option1.tsx:988-1059 and you're right that it does not. Although the hidden cachePolicy_UNSTABLE.quality = 'value' option does, as the CodeSandbox demonstrates.

So, I think we have 2 feature requests here:

  1. Yours, which I suggest we rename to Customizable control over value equality. This is for the use case where a selector's get returns a literal, which Recoil needs help with to tell if it should be considered a new value or not. I suggest we implement this by a variation option 1:
    1. Unhide cachePolicy_UNSTABLE.quality
    2. Allow the value to be a function that receives the old and new value and is expected to return true when they should be considered equal.
  2. Mine, for which I've created Customizable control over node to node subscriptions. This is for the use case where a selector's get depends on the values of (likely multiple) other selectors or atoms, but only wants to re-evaluate for changes of some of them. I suggest we implement this by a variation of option 3 (see issue).

@rpggio
Copy link

rpggio commented Feb 18, 2022

I am experimenting with a couple of extensions that might serve as a workaround. Interested to know if there are any obvious problems.
Demo

/**
 * Use a writable selector to prevent excess renders.
 * If the setting value is equal to the current value, don't change anything.
 */
export function equalAtom<T>(options: EqualAtomOptions<T>): RecoilState<T> {
  const { key, equals, ...innerOptions } = options;
  const inner = atom({
    key: `${key}_inner`,
    ...innerOptions
  });

  return selector({
    key,
    get: ({ get }) => get(inner),
    set: ({ get, set }, newValue) => {
      const current = get(inner);
      if (!equals(newValue as T, current)) {
        set(inner, newValue);
      }
    }
  });
}
/**
 * Use a wrapper selector to prevent excess renders.
 * If the latest selection is value-equal to prior ref, return the prior ref.
 */
export function equalSelector<T>(
  options: EqualSelectorOptions<T>
): RecoilValueReadOnly<T> {
  const inner = selector({
    key: `${options.key}_inner`,
    get: options.get
  });

  let prior: T | undefined;

  return selector({
    key: options.key,
    get: ({ get }) => {
      const latest = get(inner);
      if (prior != null && options.equals(latest, prior)) {
        return prior;
      }
      prior = latest;
      return latest as T;
    }
  });
}

@rmill777
Copy link

rmill777 commented Mar 13, 2022

@FokkeZB For the moment,

cachePolicy_UNSTABLE: { equality: 'value' }

is the best solution for me and seems to work how I think selector re-renders should work. Would like to see this feature made stable.

@drarmstr
Copy link
Contributor

Note that you can also use a selectorFamily() to split up the selector to limit re-evaluations. Family parameters are serialized for value-equality checks.

@BenjaBobs
Copy link
Author

That would have a negative effect on the memory footprint though, right?

@drarmstr
Copy link
Contributor

It will be cached either way. You can adjust the cache policy. Currently selectors have independent cache policies; it might be interesting to enable selectors to share an LRU cache...

@FokkeZB
Copy link

FokkeZB commented May 20, 2022

@rmill777 @drarmstr be aware that the combination of cachePolicy_UNSTABLE: { equality: 'value' } and an async selector dependency that might throw or return a function is at risk of causing #1804

@drarmstr two questions after running into this bug:

  1. Any update on improved control over customizing equality?
  2. Any reason getLoadableFromCacheAndUpdateDeps can't use allowFunctions: true?

@drarmstr
Copy link
Contributor

@FokkeZB Functions can't really be used for value equality because they can contain closure state which isn't easily captured for serialization and comparison.

@FokkeZB
Copy link

FokkeZB commented Jun 28, 2022

Here's a family version of the equalSelector that @RPisRyan shared:

import {
  ReadOnlySelectorFamilyOptions,
  RecoilValueReadOnly,
  selectorFamily,
  SerializableParam
} from "recoil";

interface EqualSelectorOptions<T, P extends SerializableParam>
  extends Pick<ReadOnlySelectorFamilyOptions<T, P>, "key" | "get"> {
  equals: (a: T, b: T) => boolean;
}

export function equalSelectorFamily<T, P extends SerializableParam>(
  options: EqualSelectorOptions<T, P>
): (param: P) => RecoilValueReadOnly<T> {
  const inner = selectorFamily<T, P>({
    key: `${options.key}_inner`,
    get: options.get
  });

  const priors = new Map<string, T>();

  return selectorFamily<T, P>({
    key: options.key,
    get: (param) => ({ get }) => {
      const innerValue = inner(param);
      const latest = get(innerValue);
      const priorKey = innerValue.key;
      if (priors.has(priorKey)) {
        const prior = priors.get(priorKey) as T;
        if (options.equals(latest, prior)) {
          return prior;
        }
      }
      priors.set(priorKey, latest);
      return latest;
    }
  });
}

Kudos to @bryanhelmig for the idea to use a Map. Ideally we'd use an atom of course, but since you can't set() an atom from within a selector's get, that's not an option.

Demo (fork)

@IDisposable
Copy link

IDisposable commented Nov 11, 2022

...
    get: ({ get }) => {
      const latest = get(inner);
      if (prior != null && options.equals(latest, prior)) {
        return prior;
      }
      prior = latest;
      return latest as T;
    }
...

Would be awesome to short-circuit when the reference is known to be the same and not bother calling the options.equals so that boilerplate doesn't need to be in any of implementations. I'm suggesting:

const latest = get(inner);
if (prior != null && (lastest === prior || options.equals(latest, prior))) {
    return prior;
}

@IDisposable
Copy link

IDisposable commented Nov 11, 2022

...
  const priors = new Map<string, T>();
... 
      if (priors.has(priorKey)) {
        const prior = priors.get(priorKey) as T;
        if (latest === prior || options.equals(latest, prior)) { // do we need to worry about null / undefined in the priors Map?
          return prior;
        }
      }
      priors.set(priorKey, latest);

Added the reference equality check here, as it's free.

As a newbie to this entire library, this may be stupid-obvious, but my long use of OWL/MFC/COM/WinForms/etc. makes me immediately concerned for the bloat in this Map. What happens if the keys go stale and how do we manage to purge them?

@FokkeZB
Copy link

FokkeZB commented Nov 12, 2022

@IDisposable good idea to do a simple equality check first, although that is also what Lodash' equals does, which our internally used version of the above defaults to.

To avoid Map from becoming too big, you could use a LRU cache: https://www.npmjs.com/package/lru-cache

@IDisposable
Copy link

Since the quality of the options.equals implementation is unknown, and the reference check is trivially cheap, I would strongly suggest that be in the base code.

@nichita-pasecinic
Copy link

I'd love to see this feature available, thought that's very prioritar for a react state management solution 😄

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request help wanted Extra attention is needed performance
Projects
None yet
Development

No branches or pull requests

9 participants