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

Extra re-renders with selectable list #1175

Closed
moroshko opened this issue May 20, 2022 · 10 comments
Closed

Extra re-renders with selectable list #1175

moroshko opened this issue May 20, 2022 · 10 comments
Labels
work in progress Work is not stuck and someone is taking responsability

Comments

@moroshko
Copy link

Given a list of selectable items, when an item's selection is toggled, I'd like only that item to be re-rendered.
However, I see that ALL items are getting re-rendered.
What am I doing wrong?

Use case: Select items to perform bulk action of the selected item ids.

CodeSandbox

@dai-shi
Copy link
Member

dai-shi commented May 20, 2022

https://codesandbox.io/s/jotai-select-list-forked-p4zzwk

Nothing is wrong. React runs render function and finds that atom values are the same, and stop rendering (no commit). This behavior is basically the same as useReducer.

@TwistedMinda
Copy link
Collaborator

TwistedMinda commented May 20, 2022

@moroshko I think your algorithm could be improved if you really need to focus on performances.

Here is a schema of your pattern:

  • create map of <string, atom>
  • for each item, create a derived atom from the map, for a given id
  • everytime you update one toggle in your list
    • this will re-render all your items
    • as dai-shi mentioned it's not a big deal because there will be no commits by react (but it's still not ideal)

Here's how you could avoid the extra re-renders:

  • for each list item create a read-write atom like you did, except not depending on the map atom
  • when you update a toggle, update seperately the atom & the selectedMap atom
  • of course, you will need to fill up the initial value for each row at the first render
const selectedAtom = atom<Record<string, boolean>>({});
const updateToggle = atom(null, (get, set, payload) => {
  const { id, atom, value } = payload
  set(selectedAtom, prev => ({ ...prev, [id]: value}))
  set(atom, value)
})

const Item = (props) => {
  const { item, initialSelection } = props
  const reference = React.useRef(atom(props.initialSelection))
  const toggle = useAtomValue(reference.current)
  const updateToggle = useSetAtom(updateToggle)
  // ...
  const onPress = () => {
    updateToggle({ id: item.id, value: !toggle, atom: reference.current })
  }
  return (null)
}

const List = (props) => {
  const initialSelection = useAtomValue(selectedAtom)
  // Stop listening changes after first mount
  return React.useMemo(() => {
    const renderItem = (item) => {
      const value = initialSelection[item.id]
      return (
         <Item item={item} initialSelection={value} />
      )
    }
    return props.items.map(renderItem)
  }, [])
}

const Component = () => {
  return (
    <List items={items} />
  )
}

That way, the toggled item is the only one re-rendered.

To be honest, for such a simple example, I'd prefer my code cleaner and more maintainable, accepting more cheap re-renders. But there may be cases where this pattern could be useful I suppose.

@TwistedMinda
Copy link
Collaborator

@dai-shi I think we can close it as it's all intended behaviour. I even tried to provide a work-around if needs be to avoid extra-renders (can't remember if I tested it, tho, but looks more like a react problem than jotai problem anyway)

OP could still add a comment to re-open if need more info.

@dai-shi
Copy link
Member

dai-shi commented Jun 2, 2022

Yeah.
Would it be nice if we could describe such a behavior is expected in docs somewhere?

@TwistedMinda
Copy link
Collaborator

TwistedMinda commented Jun 2, 2022

@dai-shi Yeah. We could add a "Performance" guide. It would tackle:

  • the expected behaviour that needs to be kept in mind when having atoms depending on other ones (that they all re-calculate when their parent changes), make the user relate to how React behaves and how beautiful the synergy is
  • reminder of utils focusAtom & selectAtom; can't hurt
  • reminder of useMemo & useCallback for not stable references, but performance this time; can't hurt
  • quick comparison with flux (selectAtom) & proxies (valtio) and in which use case they theoretically perform better
  • maybe introduce my workaround-pattern:
    • load the main store once, then ignore updates (useMemo)
    • create local atoms that don't depend on the main store
    • when setting: update both local atom & main store

Otherwise, I don't know where to put something like this, it's pretty much how react behaves too: re-render the parent will re-caclulate the children (aka useReducer)

I could try to propose a PR for this, I'm pretty interested in this topic

@dai-shi
Copy link
Member

dai-shi commented Jun 2, 2022

No, I didn't mean "Performance" guide, which sounds interesting as another topic though.

What I mean is, extra re-render without commit is an expected/intentional behavior, which is the default behavior of useReducer in React 18, and nothing needs to be worried.

@TwistedMinda
Copy link
Collaborator

TwistedMinda commented Jun 2, 2022

@dai-shi Well, that fits performance concerns to me :p Why would someone care about extra-renders if not for saving some calculation for performance purposes?

Where else would you put that note? When first talking about derived atoms in "Primitves" ?

"Performance" guide, which sounds interesting as another topic though.

I'll think about it

@dai-shi
Copy link
Member

dai-shi commented Jun 3, 2022

@dai-shi Well, that fits performance concerns to me :p Why would someone care about extra-renders if not for saving some calculation for performance purposes?

I know where you come from, but I strongly want to discuss from a different angle.
This behavior is just a normal behavior, like useReducer.
Basically, render functions should be lightweight as React can call it multiple times.
If extra calculation is the concern, useMemo should help, and it's the React pattern regardless of jotai. (So, mentioning such a pattern in "Performance" guide makes sense.)

Where else would you put that note? When first talking about derived atoms in "Primitves" ?

That sound like a good idea.

btw, @sandren and I talked about an idea of combining "Primitive" section and "Core API" section.

@TwistedMinda
Copy link
Collaborator

@dai-shi

I know where you come from, but I strongly want to discuss from a different angle.

😅😅

When first talking about derived atoms in "Primitves" ?
That sound like a good idea.

Yeah sounds resaonable. Its just gonna be a "Note:" after all, and its pretty core/primitive related. I got convinced again!

This behavior is just a normal behavior, like useReducer.
Basically, render functions should be lightweight as React can call it multiple times.

Yup. And we can't omit it in our future Performance tab, right? :p
I already mentioned it at the last sentence for RN but it doesn't feel right that it's only there.

If extra calculation is the concern, useMemo should help, and it's the React pattern regardless of jotai. (So, mentioning such a pattern in "Performance" guide makes sense.)

Yup

btw, @sandren and I talked about an idea of combining "Primitive" section and "Core API" section.

Yep this was part of one of my proposition too, for re-ordering the menu. Looking forward to it!

@TwistedMinda TwistedMinda added the work in progress Work is not stuck and someone is taking responsability label Jun 3, 2022
@TwistedMinda
Copy link
Collaborator

Closing it in favor of the 2 seperate PR's.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
work in progress Work is not stuck and someone is taking responsability
Projects
None yet
Development

No branches or pull requests

3 participants