jotaijs / jotai-scope

MIT License
55 stars 4 forks source link

Patch more functions #8

Closed yf-yang closed 10 months ago

yf-yang commented 10 months ago

6 is not the only api that needs patches. At least two categories of apis need patches:

Core Package

All the four files in https://github.com/pmndrs/jotai/tree/main/src/react/utils needs patch.

Integrations

jotai-immer's useImmerAtom https://github.com/jotaijs/jotai-immer/blob/main/src/useImmerAtom.ts

I am not sure if any other packages are influenced. Generally speaking, useXxxAtom should be patched, and atomWithXxx are fine.

dai-shi commented 10 months ago

It's an endless game. I originally thought to let devs to patch useHydratedAtoms in userland. But, as its implementation and typing is tricky, it's included. For useReducerAtom and useResetAtom, you can use useScopedAtom and you can patch on your own if necessary. useAtomCallback doesn't take an atom, so it should just work. I don't think we should cover integrations. Basically you can use useScopedAtom and/or you can patch it.

So, I don't think we should patch more. Or, we just expose useScopedAtom and let people patch things, which is clearer (I'm thinking about the performance aspect.)

yf-yang commented 10 months ago

OK, then I still suggest this package be directly merged into the core package (i.e., replace useAtomValue and useSetAtom). Let me give some detailed explanations:

Pros

  1. Eliminate API name confusion
  2. No need to patch anything
  3. Jotai should not be a solely global state management library, code reuse (atom dependency reuse) and scope are tightly related to state management. Without jotai-scope, the way I can come up with that resolves similar issues are:
    • Call useMemo and initiate an atom inside. This is a really really bad way, since the atom variable is scoped, you cannot access it anywhere in the component tree.
    • Use atoms in atom trick. This is better but still not good enough, if you want to define four atoms, they have some internal dependencies, and you want to copy multiple of those four atoms, it would be painful using atoms in atom. I think scope is quite an important feature if you want to use jotai to build reusable components, but it seems not so many people are concerned. I don't know why, that involves the scenarios of jotai.

Cons

  1. Performance? All right, I did assume patched useAtomValue and useSetAtom involves some additional performance pitfall, but when writing the benchmark, I suddenly realize that the performance impact is very slight.

First, I need to clarify, what should be compared. We should compare the performance of jotai's useAtom and jotai-scope's useAtom in jotai's use cases, not jotai-scope's.

When I need jotai-scope, the only viable API I can use is jotai-scope's, so I have no choice but accept the performance impact.

Then, if I only need jotai, but I use jotai-scope's useAtom to replace jotai, what will happen, that is the performance impact we really care about.

What does it mean by if I only need jotai? It could be divided into two scenarios:

First scenario: we are not using any ScopeProvider, then look at those lines: https://github.com/jotaijs/jotai-scope/blob/9737d4c9c606d0c084ce18fb39891edc95163925/src/createScope.tsx#L84-L87 Our performance overhead is one useContext call (that finds nothing and goes to the default context) and one getScopedAtom call. The getScopedAtom is merely an identity function: https://github.com/jotaijs/jotai-scope/blob/9737d4c9c606d0c084ce18fb39891edc95163925/src/createScope.tsx#L14 For first scenario, by the analysis, I think the impact is slight. By comparing jotai and jotai-scope's download numbers, this scenario represents most cases.

Then, second scenario: we are using ScopeProvider, but we are calling useAtom with a non-scoped atom. I believe that is the performance pitfall you are worrying about. https://github.com/jotaijs/jotai-scope/blob/9737d4c9c606d0c084ce18fb39891edc95163925/src/createScope.tsx#L84-L87 Now, the useContext call does find a context, so the getScopedAtom is no longer the default identity function, it becomes much heavier.

If we know exactly the atom we are using is not scoped, then by replacing useAtom from jotai-scope to jotai's, there could be some potential gain, in exchange of memorizing which atoms are scoped, which are not.

For that scenario, I would say I want to choose the more radical way, to trade development efficiency and clarity with the possible performance loss (the more layers of ScopeProvider, the more Map.get search, the more performance loss), by directly replace all of them.

Or we can offer a milder solution: directly replace jotai's useAtom with jotai-scope's, then rename the original useAtom, and instruct developers to use this advanced technique to improve possible performance loss.

Again, if no ScopeProvider is used, the only performance loss is a useContext call and an identity function call. At least we call useContext twice when calling useAtom (useAtomValue and useSetAtom calls useStore respectively), now it becomes 3 times. That is the impact over most current jotai users.

dai-shi commented 10 months ago

Hm, I don't think it should be merged because this is still a new idea and implementation. But, you raise an important point. Patching is a pain. Let me try something without modifying Jotai core.