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

Avoid re-rendering components if selector value hasn't changed. #314

Closed
ehahn9 opened this issue Jun 12, 2020 · 40 comments
Closed

Avoid re-rendering components if selector value hasn't changed. #314

ehahn9 opened this issue Jun 12, 2020 · 40 comments
Assignees

Comments

@ehahn9
Copy link

ehahn9 commented Jun 12, 2020

tl;dr: if an atom is an object and a selector returns a single property from that object, will downstream components re-render whenever ANY part of the atom changes, or only when that selector value changes? i.e. are selector output values cached or do re-renders occur whenever any of their input dependencies change? (I suspect the latter?).

If recoil doesn't cache selector output, then I suspect my best approach is useRecoilCallback to avoid subscribing, then have something like useStateSlice() which takes a slice fn() which triggers component updates (useState/setState) when the slice value differs?

seem right?


background:

This arises from a common(?) case:

The "atom" in my case is a (firebase) document which is basically a property bag of fields. We get async updates from firebase when that data is changed (possibly external to our app - e.g. by another user, etc.).

So if my atom is the document, I (think?) I'm hosed - any update to any field of the document will cause all downstream selectors to update and therefore all components depending on ANY part of the document to re-render.

What I'd like is to think of selectors as a "slicing" the data, so I could have a lastName selector which picks the lastName field out of the Person document. Components depending on lastName would update only when that field is different, ignoring changes to other parts of the person.

@Shmew
Copy link

Shmew commented Jun 12, 2020

I just spent some time trying to see if there was some way to work around this.

I think the best solution is to find a way to make the properties the atoms rather than the document. Such that when you need to find one part, it's really only getting that one part.

If that isn't possible, I suspect the next best solution is to create an empty component that simply observes the document atom. Which you would then run checks on re-render to see if any of the atoms need to be modified. This means that not only do you need to have atoms defined for the parts of the document you want to subscribe to, but the component needs to be aware of them, and have logic to determine if the value has changed.

@drarmstr drarmstr changed the title Question: selectors which "slice" atom state? [Request] Selectors don't propagate updates if their computed value hasn't changed. Jun 12, 2020
@drarmstr drarmstr added the enhancement New feature or request label Jun 12, 2020
@drarmstr
Copy link
Contributor

Yes, it's on our roadmap to have selectors suppress downstream updates and subscriptions if their computed value has not changed. I already have a diff for suppressing updates if setting an atom to the same value.

@ehahn9
Copy link
Author

ehahn9 commented Jun 12, 2020

@drarmstr that would be very, very fantastic :-). I guess it opens up the can of "equality" worms in that selector functions would need to be careful to return the same (memoized?) instances as I assume it would be a reference equality comparison. But that's a huge advance over the current situation. Thanks for listening ;-)

@drarmstr
Copy link
Contributor

Yeah, we would likely default to reference equality but allow for custom comparators or something. That would help avoid the need for crazy memoization hijinks. We haven't started thinking about the API yet, though; so not committing to anything at this point.

@ehahn9
Copy link
Author

ehahn9 commented Jun 13, 2020

@Shmew thanks, Cody. Sounds good. Since I don't get any more granularity from firebase (the event fires for the whole doc/record, not per field), I'll make a "sync" component which subscribes to the firebase doc and then updates one atom (an atomFamily for that document) for each field in the doc if they value changes. thx for the suggestions.

@ehahn9 ehahn9 closed this as completed Jun 13, 2020
@Shmew
Copy link

Shmew commented Jun 13, 2020

@ehahn9 no problem! Can you leave the issue open so we can track the progress of this feature request? Once it's resolved you can implement it how you did originally.

@ehahn9
Copy link
Author

ehahn9 commented Jun 13, 2020

Re-opening so we can track.

@ehahn9 ehahn9 reopened this Jun 13, 2020
@jaredpalmer
Copy link
Contributor

Glad this is on the roadmap and excited about this idea. When this lands, it will remove a lot of the complexity with object writes. Currently, Working on a an app with a normalized tree view with a bunch of properties in each leaf. Most updates aren’t bad, but there are some where multiple new leanes need to be added. This is super annoying because I have to set(itemPropAtomFamily({ id }), ...) for each item property in a useRecoilCallback. The ability to set(item({ id}), ... ) and then have selectors intelligently update would be ideal. Even better would be to retain the ability to use a selectorFamily setter and have it understand the relationship to the atomFamily.

In my opinion, when this lands, Recoil will be the absolute go-to solution for complex state management. It will also simplify the performance story and allow folks to fall into a pit of success.

@AjaxSolutions
Copy link

In my opinion, when this lands, Recoil will be the absolute go-to solution for complex state management

It would be nice to see a Recoil vs MobX comparison because MobX is probably the most similar to Recoil.

@benjaminpkane
Copy link

I understand that this is still in development, but I just want to re-iterate what has been said above by saying that this package is useless from an optimization perspective until the following example App only re-renders the button that is clicked.

New to react, but I do see a lot of potential in recoil.

https://codesandbox.io/s/recoil-sandbox-20200516-4ow67

@drarmstr
Copy link
Contributor

While optimizing selectors will be nice, it is not required to optimize and avoid re-renders. Current best-practice is to use individual atoms or atom families to represent state for managing subscriptions and re-renders (#480)

@Shmew
Copy link

Shmew commented Jul 16, 2020

Handling external data where it returns a large object (which you can't control) is really not a nice story at the moment. Even just making the constSelector have this type of functionality would go a really long way, as then we can map the complex atom into multiple smaller selectors as needed without worrying about re-renders if a different part changes.

@aaronnuu
Copy link

aaronnuu commented Jul 17, 2020

While optimizing selectors will be nice, it is not required to optimize and avoid re-renders. Current best-practice is to use individual atoms or atom families to represent state for managing subscriptions and re-renders (#480)

Just a note on this best practice, for my use case having an atom for every individual piece of state increases the complexity of my components by a huge amount.

What I am trying to achieve is form state that I can manage both on the field level and at the parent component level with some helpers (think setFieldValue(name, value), setFieldError(name, error) etc.) while the fields may not even be rendered (hidden behind a tab etc). This means that what I have to do is create an atom per field, store the keys for this atom somewhere, then wherever it's used I have to get the key somehow. I was just using a util to pluck the atom out depending on the name of the field, but all of this means that I have a bunch of extra utils + data structures to manage all of this.

What I really want (and how I currently have it implemented) is just an atom at the top level that looks something like this

const formAtom = atomFamily({
  key: 'form',
  // Would really like to decouple initialValues
  // from the form atom key also
  default: (id, initialValues) => ({
    initialValues,
    values: initialValues,
    errors: {},
    touched: {},
    ...etc
  })
})

Where whenever my field renders, all I need is the name of the field and I can do something like this

const fieldValueSelector = selectorFamily({
  key: 'fieldValue',
  get: ({ name, formId }) => ({ get }) => {
    const { values } = get(formAtom(formId));
    return _.get(values, name) ?? 'default value'
  },
  set: ({ name, formId }) => ({ set }, newValue) => {
    set(formAtom(formId), (prevForm) => ({
      ...prevForm,
      values: _.set(_.cloneDeep(prevForm.values), name, newValue)
    }));
  }
})

This allows me to not even care about any kind of data structure to keep track of all the individual atoms for the field and literally just have the <Field /> component consume the entire form state and decide what to connect to depending on the name given to me (whether it's from the developer during setFieldValue or if it's a user changing a field).

I also can't just use the name as my field atom key because fields might change name during the lifetime of the component, if we're re-arranging fields within a list of fields of the same type for example.

Basically having a selector that doesn't re-render my component unless the returned value changes would save me a hundred lines of code and a lot of maintenance burden.

@drarmstr
Copy link
Contributor

Yup, I agree that we really want to do this.

@mismith
Copy link

mismith commented Jul 20, 2020

Does anyone have workaround for this in the meantime? (maybe you've come across one @drarmstr, or have an idea of how to implement one?)

I came up with a similar approach to you @aaronnuu, but I can't actually get that fully performant either. I've put together a barebones repro here: https://codesandbox.io/s/userecoilstateat-3lers

I've also tried using useRecoilCallback(), but I can't seem to get anything working dependably that doesn't have timing issues / race conditions (maybe because of how the async set()'s are queued?).

@wizzard0
Copy link

@drarmstr how to enable the feature flag? :)

@drarmstr
Copy link
Contributor

@wizzard0 - The "GK" is recoil_suppress_rerender_in_callback. @aaronabramov is looking at how best to expose these feature flags in the open-source package. In the meantime, if you'd like to experiment with it or help debug the current issue you could try adding it to the gks Map defined in Recoil_gks.js.

@wizzard0
Copy link

Well... in the meantime I gave up trying to debug memory leaks and re-implemented a tiny bit of Recoil API (just useRecoilState/useRecoilValue, no selectors or snapshots) separately %) really like the API though

@drarmstr drarmstr changed the title [Request] Selectors don't propagate updates if their computed value hasn't changed. [Request] Avoid re-rendering components if selector value hasn't changed. Mar 17, 2021
@drarmstr drarmstr changed the title [Request] Avoid re-rendering components if selector value hasn't changed. Avoid re-rendering components if selector value hasn't changed. Mar 17, 2021
@drarmstr drarmstr added the help wanted Extra attention is needed label Mar 17, 2021
@thanhlmm
Copy link

The recoil 0.2 is out but this issues still not fixed yet

@drarmstr drarmstr removed the help wanted Extra attention is needed label Apr 3, 2021
@drarmstr
Copy link
Contributor

drarmstr commented Apr 3, 2021

The 0.2 release did not enable this feature flag as an issue was discovered with React Suspense. We now have a partial fix with #952. However, looks like there's still a corner case remaining that needs to be debugged. We don't yet have a cut-down test case.

@drarmstr
Copy link
Contributor

drarmstr commented Apr 17, 2021

#952 has been updated to hopefully resolve the remaining issue. Still pending internal review and testing.

@babichss
Copy link

babichss commented May 20, 2021

Faced similar issue, but with more complex approach.

const atom$ = atom({
  key: 'atom',
  default: {}
});

const selector$ = selectorFamily({
  key: 'selector',
  get(keys: string[]) {
    return ({ get }) => {
      const map = get(atom$);
      return keys.map(key => map[key]);
    }
  }
});


setAtom({ ...atomValue, [key]: value });

I have, let's say, 10 components subscribed via selectorFamily, and result for each should be unique set of values for each component. If I change one value in given map, all 10 components will rerender.

Proposed solution with #749 check only primitive values for equality, but in my case I need to manually check resulting array. Is it possible to add some custom shouldUpdate function to selector config, which would allow user to add specified check on result equality?

Example:

const selector$ = selectorFamily({
  key: 'selector',
  get(keys: string[]) {
    return ({ get }) => {
      const map = get(atom$);
      return keys.map(key => map[key]);
    }
  },
  shouldUpdate(value, prev):boolean {
    //some black magic
  }
});

Thank you.

@BenjaBobs
Copy link

Related discussion on custom equality check function: #992

@maclockard
Copy link

@drarmstr it looks like #952 was merged, but it seems like this has not yet been fixed. Is the new behavior behind a feature flag?

@alzalabany
Copy link

any progress ? ;
only way i found acceptable so far was to call all my hooks in a container and use react.memo/useMemo but that just bring us back to age of redux's connect kinda lose declarative beauty of hooks;

how about we stop propagating update event if a certain Error was Thrown ? or a shouldnotifySubscriber of a family return false.

something like

const doc = atom({key: 'doc',default: {a:1,b:1}});
const atom = selectorFamily({
  key: 'docSelector',
  shouldnotifySubscriber: key => (prevValue, currentValue) => prev[key] !== current[key],
  get: key => ({get})=>get(doc)[key],
  set: key => ({set,get},v)=>set(doc, {...get(doc),[key]:v})
})

@drarmstr
Copy link
Contributor

drarmstr commented Aug 6, 2021

Please feel free to try out the optimization by enabling the feature flag described above. There's just a possible corner case remaining we haven't had a chance to investigate yet. Adding a custom equality check would be a next step after basic reference equality checks work. (cc @csantos42)

@maclockard
Copy link

@drarmstr how do you enable the flag?

@drarmstr
Copy link
Contributor

drarmstr commented Aug 6, 2021

Add recoil_suppress_rerender_in_callback to the flags set for gks in Recoil_gks.js. Sorry there isn't a more convenient mechanism right now.

@drarmstr drarmstr self-assigned this Aug 24, 2021
@drarmstr
Copy link
Contributor

Optimization enabled for open-source package with #1185

@ghost
Copy link

ghost commented Nov 27, 2021

Dear @drarmstr,

Do you think going about this by adding an optional extra parameter to the "useRecoilValue" hook that would take a custom comparator function which could be used to override the reference equality check used in

https://github.com/facebookexperimental/Recoil/blob/main/packages/recoil/hooks/Recoil_Hooks.js#L322

would make sense?

You once mentioned that this would be possible, #992 (reply in thread).

Thanks.

@drarmstr
Copy link
Contributor

drarmstr commented Dec 2, 2021

Current status: With 0.4.1 release selectors updates do not trigger a re-render if their value has not changed based on reference equality. Feature request with #1416 to allow custom comparators to suppress re-rendering.

@drarmstr drarmstr closed this as completed Dec 2, 2021
@AndonMitev
Copy link

Guys i have following problem. I do have global polling which fills recoil state. Then in 1 of my components i use useRecoilValue to read the state but then when i leave the component he stays rendered and get updates per X second when polling is triggered, how to unsubscribe component from state changes if is not rendered

@jacksonblankenship
Copy link

jacksonblankenship commented Dec 1, 2022

Guys i have following problem. I do have global polling which fills recoil state. Then in 1 of my components i use useRecoilValue to read the state but then when i leave the component he stays rendered and get updates per X second when polling is triggered, how to unsubscribe component from state changes if is not rendered

If it's causing expensive updates, you could use React memo. Call useRecoilValue from the parent and pass the value to the child, who's wrapped with the memo HOC. This'll still update the parent, but it could optimize the child which is helpful if the child is rendering an entire table or something.

import { memo } from 'react';

const Child = ({data}) => {
  return <div>{JSON.stringify(data, null, 2)}</div>
};

export default memo(Child);
import Child from './Child';

const Paren = () => {
  const data = useRecoilValue(someSelector);
  return <Child data={data} />;
};

export default parent;

More info here.

@daves-weblab
Copy link

Any news here? When depending on a list of keys that are used for a family and one needs to filter out some of them, selectors always retrigger even if the array content is the same, since the filter always returns a new reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment