jotaijs / jotai-scope

MIT License
55 stars 4 forks source link

feat(scope): Avoid extra re-renders with memoization #10

Closed yf-yang closed 10 months ago

yf-yang commented 10 months ago

There are three stuff in this PR:

  1. Wrap ScopeProvider's main body with useMemo
  2. Add a set scopedAtoms that stores all the scoped atoms inherited from all of one ScopeProvider's ancestors. Patch getAtom so that if scopedAtoms does not contain an atom, do not search the atom in parent scope. scopedAtoms acts as a cache here.
  3. Update example. I am not sure if this one should be put in the same PR, so I split it into another commit.
dai-shi commented 10 months ago

1: I didn't use useAtom intentionally, as using memo outside should do if really necessary. Basically, we should lift the component up to avoid extra re-renders. I don't know the actual usage, and if people would memoize atoms prop. 2: Does it work with derived atoms? (btw, as I noticed previously, it should be a linked list, instead of concatenating.) 3: Please create a new example 03_something.

yf-yang commented 10 months ago
  1. ScopeProvider may not be near the root of the tree, it could be near to the root of a reused component. If the component re-renders, won't it re-render the ScopeProvider?
  2. I think so

it should be a linked list, instead of concatenating.

What do you mean by that?

dai-shi commented 10 months ago

I would like to wait adding useMemo until we see some use cases. It's not near the root, but I think it's possible to avoid re-renders for most cases. I might be wrong.

By linked list, I mean something like [atoms, parentAtoms] instead of [...atoms, ...parentAtoms] because we want to keep the scope provider tree structure (for devtools in the far future.)

dai-shi commented 10 months ago
const baseAtom = atom(0)
const derived1Atom = atom((get) => get(baseAtom))
const derived2Atom = atom((get) => get(derived1Atom))

<ScopeProvider atoms={[baseAtom]}>
  // Does derived2Atom get scoped?
yf-yang commented 10 months ago
const baseAtom = atom(0)
const derived1Atom = atom((get) => get(baseAtom))
const derived2Atom = atom((get) => get(derived1Atom))

<ScopeProvider atoms={[baseAtom]}>
  // Does derived2Atom get scoped?

Emm, seems not. I can get your point, let me think about other solutions.

yf-yang commented 10 months ago

Coming back for 2, I realize we have to make copies of all the atoms and their dependent atoms, because when calling useAtom and establish dependency connections between an atom and its parents, the dependency graph of its parents is still unknown until current scope is fully rendered. It is a tradeoff to make potential redundant copies.

One minor optimization opportunity is detecting typeof atom.read === 'function', and bypass unnecessary creation of non-scoped primitive atoms. This solution is not so graceful, so I would like to know if you like it.

dai-shi commented 10 months ago

I prefer not; a) we should basically avoid assuming how atoms are constructed, and b) it won't improve anyway because read is always a function.

yf-yang commented 10 months ago

read is always a function

https://github.com/pmndrs/jotai/blob/2857b4272427f495f26b9a662ce13a6d59800208/src/vanilla/atom.ts#L94

Sorry, I mean this line, config.init, but you are right, we still cannot distinguish them because write can have dependencies.

dai-shi commented 10 months ago

Yeah, and even if an atom have .init, that doesn't mean the atom is a primitive atom. .read is still customizable and can depend on other atoms.

yf-yang commented 10 months ago

For 1, please take a look of this example: https://codesandbox.io/s/dawn-grass-kzqp3j?file=/src/SortableItem.js, createScope.js is the useMemo version, createScope1 is the plain version. I add a log to indicate the re-render.

I believe you can avoid that stuff in a pure atom world, but according to my experiences, there are frameworks that using React.Context (and sometimes they are the top level context), so actually we are using a mixture of atoms and original React.Context. For those cases, we can add a useMemo, or instruct developers to use React.memo themselves. I personally prefer the first case, because the learning curve to understand when you need that memo is steep.

codesandbox-ci[bot] commented 10 months ago

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 5ce0a063e335cf6a571fbffb90024b2a37e19504:

Sandbox Source
React Configuration
React TypeScript Configuration
dai-shi commented 10 months ago

Thanks for the example. I'll check it out later. useMemo (incl. useCallback) has actually been one of my favorite hooks. I've used it in many of my libraries. But, I believe in the future it will be discouraged, so I'd like to look for alternatives. I would probably prefer the second approach (= memo in userland).

yf-yang commented 10 months ago

Updated https://codesandbox.io/s/dawn-grass-kzqp3j?file=/src/SortableItem.js, memo is also fine.

yf-yang commented 10 months ago

Updated https://codesandbox.io/s/dawn-grass-kzqp3j?file=/src/SortableItem.js, memo is also fine.

This one is problematic when I try to update children, I need further investigation.

dai-shi commented 10 months ago

Your memo usage looks good (I think you can omit the second arg of memo). If you want to update the children, it should be passed from outside.

Your realistic example helps me understanding the use case. While I still think memo is fine and useMemo will be discouraged in a longer term, I'm fine with adding useMemo for now. But, the deps must be [..., atoms] instead of [..., ...atoms]. I wonder if it's really useful in this case, because you have to memoize atoms anyway.

yf-yang commented 10 months ago

I have a question. If the children changes, then only useMemo can prevent those lines from executing again, memo can do nothing with it, is it right?

Today I encounter a case that children changes. Although this is not jotai style and I may avoid that by lifting ScopeProvider up, it adds some difficulty.

dai-shi commented 10 months ago

Yeah, basically we want to lift up children, but otherwise, useMemo may help.

yf-yang commented 10 months ago

Thoughts:

yf-yang commented 10 months ago

Another related example. https://codesandbox.io/s/restless-browser-m4jmqq?file=/src/App.js For such case, memo/useMemo is necessary

yf-yang commented 10 months ago

I am now working with Lexical, it is developer by someone from React team, so generally it could be considered as equivalent to a useState in the parent component, and child components are components with atoms. So this one is a special case that children changes (due to the parent component's state keeps changing). I took a day to think how to make jotai work with it and found a way to memo the jotai part, ignore all the parent states, and only add setState callback to the atoms. So the states are updated, but the children only use the initial state to hydrate, and then children uses atoms, only sync the latest state with parent (just like the Persistence part of jotai's doc).

I describe this scenario to show that, although it is viable to totally avoid re-render, I must say it is hard to find a way to refactor the code. If useMemo is internally implemented, although it is not the best solution to avoid re-render, it offers a little bit tolerance.

dai-shi commented 10 months ago

I'm actually fine with useMemo to cover various use cases. On the other hand, I'm a bit skeptical about how the warning is useful in practice.

dai-shi commented 10 months ago

I'm actually fine with useMemo to cover various use cases. On the other hand, I'm a bit skeptical about how the warning is useful in practice.

A new idea just came to me. Let me draft a PR.

dai-shi commented 10 months ago

Let me draft a PR.

Well, let me directly modify this PR, as it's your original take.