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

Nested Scope Consistency Question #32

Closed
dmaskasky opened this issue May 8, 2024 · 31 comments · Fixed by #33
Closed

Nested Scope Consistency Question #32

dmaskasky opened this issue May 8, 2024 · 31 comments · Fixed by #33

Comments

@dmaskasky
Copy link
Member

dmaskasky commented May 8, 2024

I'm wondering about the current implementation for nested scope. Is it strange that derivedAtom1 does NOT use the baseAtom scoped to layer2?

<ScopeProvider atoms={[derivedAtom1]}>
  <Counter counterClass="layer1" />
  <h2>Layer2: Base and derived2 are scoped</h2>
  <p>
    derived1 should use layer1&apos;s atom, base and derived2 are layer 2
    scoped
  </p>
  <ScopeProvider atoms={[baseAtom, derivedAtom2]}>
    <Counter counterClass="layer2" />
  </ScopeProvider>
</ScopeProvider>

For the single layer case, derivedAtom1 would use baseAtom inside Counter below, instead of the global baseAtom.

<ScopeProvider atoms={[baseAtom, derivedAtom2]}>
    <Counter counterClass="layer1" />
</ScopeProvider>

I think it would be more consistent if derivedAtom1 uses layer2 baseAtom in layer2 Counter.

Do you see any pitfalls with this proposed behavior?
Are there any advantages in keeping the current behavior?

@yf-yang
Copy link
Collaborator

yf-yang commented May 10, 2024

Go back to this case: #28 (comment)

So, when derived1 is accessed in layer2, there are two possibilities:

  1. derived1 is scoped in a parent scope.
  2. derived1's dependency is scoped in current scope.

There is no conflict between those two possibilities, so they can both happen. Therefore, we need to define the priority.

In current implementation, we check derived1 itself in parent scope first, then check its dependencies.

I'm not sure if we can offer a better rule. For example,

<ScopeProvider atoms={[derivedAtom1]}>
  <Counter counterClass="layer1" />
  <ScopeProvider atoms={[baseAtom, derivedAtom2]}>
    <Counter counterClass="layer2" />
  </ScopeProvider>
</ScopeProvider>

You think in layer 2 derived1 should access nested one? Then what about this case?

<ScopeProvider atoms={[derivedAtom1]}>
  <Counter counterClass="layer1" />
  <ScopeProvider atoms={[derivedAtom2]}>
    <Counter counterClass="layer2" />
  </ScopeProvider>
</ScopeProvider>

This case is actually part of the previous mentioned comment. I wonder if you want some rule like "if the dependency is explicitly scoped, then it has higher priority".

@dmaskasky
Copy link
Member Author

dmaskasky commented May 10, 2024

Case Study

Consider these two cases. How are they similar? How are they different?

const baseAtom = atom(0)
const derivedAtom1 = atom((get) => get(baseAtom))

function Counter() {
  const [count, setCount] = useAtom(derivedAtom1)
  ...
}


// Case 1
<Provider>
  <Counter counterClass="layer1" />
  <ScopeProvider atoms={[baseAtom]}>
    <Counter counterClass="layer2" />
  </ScopeProvider>
</Provider>

// Case 2
<ScopeProvider atoms={[derivedAtom1]}>
  <Counter counterClass="layer1" />
  <ScopeProvider atoms={[baseAtom]}>
    <Counter counterClass="layer2" />
  </ScopeProvider>
</ScopeProvider>

How are they similar?

For both Case 1 and Case 2, derivedAtom1 is scoped to the parent scope of layer2. It is not scoped to layer2.

How are they different?

In Case 1, derivedAtom1 is just an unscoped (eg global) derived atom from the store given by the top-level provider. In layer2 it gets it's value from the scoped baseAtom.

In Case 2, derivedAtom1 is in the parent scope. In layer2, current implementation has it get it's value from layer1 despite baseAtom being scoped to layer2. This is different behavior from Case 1.

Summary

With existing implementation we can make the following statement:
"
including atoms in ScopeProvider somehow makes them special because their atom dependencies' scope is fixed to the scope of the atom even if those dependencies are explicitly scoped in nested ScopeProviders.
"

My thoughts on this

We should not make that statement in the Summary above. It artificially restricts usage and goes against purpose of using ScopeProvider. And currently, there is no workaround for this specialness property of scoped derived atoms.

The ScopeProvider lets applications mix global atoms with scoped ones, but fixing the scope of atom dependencies of scoped atoms prevents mixing scopes of atom dependencies for nested scopes.

  1. If derivedAtom1 is scoped in level1 and its dependency dep1 is scoped in level2, to use scoped dep1 in level2 derivedAtom1 must also be scoped in level2.
  2. And if derivedAtom1 has another level1 scoped dependency dep2, it must make dep2 a level2 scope as well.

For nested scopes, this makes derived atoms all-or-nothing, and the benefits of using ScopedProvider are lost.

Proposal

The priority should be: The closest scoped atom is the winner.

getAtom Algo

const derived1 = atom((get) => get(dep1), (get, set) => set(dep1, get(dep1) + 1))

To get dep1:

  1. if dep1 is explicitly added to current scope, return it
  2. if derived1 is explicitly added to current scope, copy dep1 to current scope map if DNE and return it
  3. recurse up scope ancestor path repeating (1) and (2)

To get derived1:

  1. if dep1 is explicitly added to current scope, return it
  2. recurse up scope ancestor path repeating (1)

What about the case you mentioned?

You think in layer 2 derived1 should access nested one? Then what about this case?

<ScopeProvider atoms={[derivedAtom1]}>
  <Counter counterClass="layer1" />
  <ScopeProvider atoms={[derivedAtom2]}>
    <Counter counterClass="layer2" />
  </ScopeProvider>
</ScopeProvider>

For this case, in layer2

  1. derivedAtom1 accesses baseAtom scoped to layer1
  2. derivedAtom2 accesses baseAtom scoped to layer2

Related:

@yf-yang
Copy link
Collaborator

yf-yang commented May 11, 2024

OK, so the resolution direction is

  1. Resolve dependencies
  2. Resolve to ancestor scopes.

I'm still unclear what's the boundary of 1, when we will consider 1 as finishes and start to try 2?

Let's dive deeper into details. Suppose some of atomA's dependencies are scoped, then atomA is scoped, so we finishes in 1.
If that's not the case, should we check the dependencies' ancestor scope atom first, or we do not check anything and directly check atomA in parent scope?

That's the conceptual question. Consider those two cases. You can ignore derived2. Which base would derived1 in layer3 access?

<ScopeProvider atoms={[derived1]}> // layer1
  <ScopeProvider atoms={[base]}> // layer2
    <ScopeProvider atoms={[]}> // layer3
      <Counter />
    </ScopeProvider>
  </ScopeProvider>
</ScopeProvider>
<ScopeProvider atoms={[base]}> // layer1
  <ScopeProvider atoms={[derived1]}> // layer2
    <ScopeProvider atoms={[]}> // layer3
      <Counter />
    </ScopeProvider>
  </ScopeProvider>
</ScopeProvider>

@dmaskasky
Copy link
Member Author

dmaskasky commented May 11, 2024

Language convention: nested scope is colloquially greater than parent scope (ie layer2 > layer1).

Q: should we check the dependencies' ancestor scope atom first?
A: derived1 scope > dep1 scope ? derived1 : dep1.

Example 1

<ScopeProvider atoms={[derived1]}> // layer1
  <ScopeProvider atoms={[base]}> // layer2
    <ScopeProvider atoms={[]}> // layer3
      <Counter />
    </ScopeProvider>
  </ScopeProvider>
</ScopeProvider>

Q: Which base would derived1 in layer3 access?
A: layer2. Because base layer2 > derived1 layer1.

Example 2

<ScopeProvider atoms={[base]}> // layer1
  <ScopeProvider atoms={[derived1]}> // layer2
    <ScopeProvider atoms={[]}> // layer3
      <Counter />
    </ScopeProvider>
  </ScopeProvider>
</ScopeProvider>

Q: Which base would derived1 in layer3 access?
A: layer2. Because derived1 layer2 > base layer1.

@yf-yang
Copy link
Collaborator

yf-yang commented May 11, 2024

OK, so the rule would be:
Resolve the dependency in current scope.
Then, resolve it in the ancestor scope, recursively, until some atoms of the dependencies is scoped.

Conceptually, it makes sense, but when it comes to implementation, since dependency graph is dynamically built, we need to traverse the whole graph to know if any of the dependency is scoped, then go upward to the ancestor scope.

In worst case, if a derived atom and its base atom is global, if it is deeply nested, then the computational complexity would be O(Number of nest scopes * Number of atoms in the dependency).

@yf-yang
Copy link
Collaborator

yf-yang commented May 11, 2024

base -> derived1 -> derived2
Example 1:

<ScopeProvider atoms={[derived1]}> // layer1
  <ScopeProvider atoms={[base]}> // layer2
    <ScopeProvider atoms={[]}> // layer3
      <Counter />
    </ScopeProvider>
  </ScopeProvider>
</ScopeProvider>

When calling useAtomValue(derived2):

  • layer3: traverse derived2 -> derived1 -> base, get result and None of them are scoped, so result is invalid
  • layer2: traverse derived2 -> derived1 -> base, base is scoped, so result is valid, get from layer2 scope.

Example 2:

<ScopeProvider atoms={[]}> // layer1
  <ScopeProvider atoms={[]}> // layer2
    <ScopeProvider atoms={[]}> // layer3
      <Counter />
    </ScopeProvider>
  </ScopeProvider>
</ScopeProvider>

When calling useAtomValue(derived2):

  • layer3: traverse derived2 -> derived1 -> base, get result and None of them are scoped, so result is invalid
  • layer2: traverse derived2 -> derived1 -> base, get result and None of them are scoped, so result is invalid
  • layer1: traverse derived2 -> derived1 -> base, get result and None of them are scoped, so result is invalid
  • global: get result
    Needs 3 * (3 + 1) atom.read call (base should access itself). Each useAtomValue and setAtom call will result in this number of calls. Since dependency resolution at least happens once, one more nested a component is, the additional one time more computations are needed for every atom call within.

@yf-yang
Copy link
Collaborator

yf-yang commented May 11, 2024

Horizontal line: atom.read/atom.write
Vertical line: callback handled by ScopeProvider context internally

Example1
Original Implementation (Wrong)
image
Current Implementation
image
Proposed Implementation
image

@yf-yang
Copy link
Collaborator

yf-yang commented May 11, 2024

@dai-shi Do you have any comments on different implementations?
I have three concerns:

  1. atom.read/atom.write are handled by users, they could be expensive (are they?), they could have side effects (do they?)
  2. I am still not sure if it is possible to return an additional value indicating if the dependency atom is scoped to the derived atom.
  3. Write is more difficult than read. Write should have exactly one side effect at the end. I don't know how to do it gracefully.

@yf-yang
Copy link
Collaborator

yf-yang commented May 11, 2024

@dmaskasky After checking the code, I think it is impossible to tell derived atom from base atom that base atom is scoped or not. The only stuff that can be passed to derived atom is the base atom's state value.

@dmaskasky
Copy link
Member Author

Example 1:

<ScopeProvider atoms={[derived1]}> // layer1
  <ScopeProvider atoms={[base]}> // layer2
    <ScopeProvider atoms={[]}> // layer3
      <Counter />
    </ScopeProvider>
  </ScopeProvider>
</ScopeProvider>

layer3: traverse derived2 -> derived1 -> base

this should use base (layer2) because in layer3, derived1 -> base (layer2).

Example 2:

<ScopeProvider atoms={[]}> // layer1
  <ScopeProvider atoms={[]}> // layer2
    <ScopeProvider atoms={[]}> // layer3
      <Counter />
    </ScopeProvider>
  </ScopeProvider>
</ScopeProvider>

result is invalid

what does invalid mean? since none are scoped, it should use the closest Provider store otherwise default global store.

Needs 3 * (3 + 1) atom.read call

I don't think atom.read should get called on every level. Call once, then look up the chain for the atom dependency in the weakSet.

Also, I looked at implementation and it looks like it is copying the atom at every level. I think it would be more performant to copy atom config only for atoms in the atomSet and its dependencies.

After checking the code

I think the code will need to be changed to allow for atoms of parent scopes to "lexical scope" their nested scopes. I recommend a data structure to make this easier to accomplish while staying in the current scope.

@dai-shi
Copy link
Member

dai-shi commented May 11, 2024

atom.read/atom.write are handled by users, they could be expensive (are they?), they could have side effects (do they?)

write can have side effects. by contract, read should be idempotent, but as optimization, it's not invoked unnecessarily. both can be expensive technically.

@dmaskasky
Copy link
Member Author

yes, read should be called only once.

@dmaskasky
Copy link
Member Author

I am still not sure if it is possible to return an additional value indicating if the dependency atom is scoped to the derived atom.

is this necessary? I think an atom's membership to a weakSet should be all that is needed to indicate scope. Derived atom dependencies are implicitly scoped.

Write should have exactly one side effect at the end.

Write can have any number of side-effects before and after zero to many calls to get/set, and a value may be returned from write.

@dai-shi
Copy link
Member

dai-shi commented May 11, 2024

yes, read should be called only once.

Though, it's not a guaranteed feature for the future. Our mental model should be "read" is no side effect.

@dmaskasky
Copy link
Member Author

dmaskasky commented May 11, 2024

Yes, for instance atomEffect does not assume read is called only once, but unnecessary calls should be avoided for performance. In this case, I don't think read needs to be called more than once since the mechanism for resolving atoms is built into the overrided getter and setter functions.

@yf-yang
Copy link
Collaborator

yf-yang commented May 11, 2024

image

Let me try to explain in details how this one should be implemented.

At layer 3

  1. Call derived2.read(), it is a function that calls get(derived1).
  2. Then similarly, call derived1.read(), it calls get(base)
  3. Then, base.read() calls get(base)
    Now after all of them are called, we finally figure out that "all of derived2's dependencies are not scoped", so we go to layer2.

At layer 2

  1. Call derived2.read(), it is a function that calls get(derived1).
  2. Then similarly, call derived1.read(), it calls get(base)
  3. Since the get function is actually hooked by us, we now figure out that "base is scoped", so we stop here, access layer 2's atoms.

Now, the information that "no atom is scoped in derived2's dependency chain at layer 3" should be sent to derived2, but there is no easy way to achieve that.

@yf-yang
Copy link
Collaborator

yf-yang commented May 11, 2024

Emmm... I've come up with some ugly hack.

When store.set is called,

  1. We enter a "dependency resolution stage". Init an empty Set.
  2. During the stage, we bypass original jotai mechanism. Instead, we recursively calls atom.read/atom.write, to find all the dependency atoms.
  3. Since we've collected all the dependency atoms, the first stage finishes.
  4. Then, for each dependency atoms, we find each one's deepest scope. Then, we find the deepest scope of all the atoms. That's the target scope.
  5. Then, use the atom in the target scope to actually call store.set

A bit drawback is that, dependency could vary based on different value at different scope. So, dependency resolution stage" should be called at every scope until we find the correct one. So the computational efficiency problem still exist.

@yf-yang
Copy link
Collaborator

yf-yang commented May 11, 2024

The root problem: dependency is dynamically computed. The only way to figure out the actual dependency is calling atom.read/atom.write. So, each time a horizontal arrow of the graph is traversed, an atom.read/atom.write is called.

Be aware that dependency may not even be the same in different scope. DerivedAtom could access base1 or base2 given different value of base3. Only one dependency resolution is not enough.

@dmaskasky
Copy link
Member Author

This is looking closer to what I'm imagining.

The only change I would make is to synchronously get the atom from the nearest scope first before calling its read fn. That way dynamic deps edge case is handled correctly. So for each dep recursive, always look starting at the nearest scope.

I believe only one read should suffice. 😌

@dmaskasky
Copy link
Member Author

I might be wrong but I think the requisite change should also address #24

@dmaskasky
Copy link
Member Author

I'd be happy to review a PR if you have an idea on how to fix.

@yf-yang
Copy link
Collaborator

yf-yang commented May 11, 2024

The only change I would make is to synchronously get the atom from the nearest scope first before calling its read fn.

We can't do it.

Consider this case:

DerivedAtom could access base1 or base2 given different value of base3.

We need base3's value to get the correct dependency atom of derived.
We need to know derived and base3's scope to get the correct value of base3.
We don't know base3's scope. The only way is to recursively iterate each scope, then figure out if the scope and the dependency can be simultaneously determines in the iterating scope.

@yf-yang
Copy link
Collaborator

yf-yang commented May 11, 2024

const derived = atom(get => get(base3 > 0 ? base1 : base2));

<ScopeProvider atoms={[derived]}> // layer1
  <ScopeProvider atoms={[base3]}> // layer2
    <ScopeProvider atoms={[base2]}> // layer3
      <ScopeProvider atoms={[base1]}> // layer4
        <Counter />
      </ScopeProvider>
    </ScopeProvider>
  </ScopeProvider>
</ScopeProvider>

@yf-yang
Copy link
Collaborator

yf-yang commented May 11, 2024

Emmm, maybe I can come up with another example that breaks the rule.

  • derived -> atom1
  • derived -> atom2
<ScopeProvider atoms={[derived]}> // layer1
    <ScopeProvider atoms={[atom1]}> // layer2
      <Counter />
    </ScopeProvider>
</ScopeProvider>

So from the rule, in layer2, derived should access atom1 of layer2. However, which atom2 will it access?
global atom2 seems breaks the scope.
layer1 atom2 introduces implicit scoping, I believe it would be unexpected in most cases.

@dmaskasky
Copy link
Member Author

dmaskasky commented May 11, 2024

I'm not sure I follow. In your example above, base1 vs base2 vs base3 are different atoms - not atoms of different scopes.

derived1 can only declare that it needs the value from baseAtom. Remember atoms are just definitions and scopes are tied to the store and not the atom definition. derived1 would not be able to dynamically switch to a different baseAtom scope.

Even if it were possible, the strategy I have in mind would still be able to handle this case efficiently.

I might put something together when I find the time.

@yf-yang
Copy link
Collaborator

yf-yang commented May 12, 2024

Hmmm, the same atom in different scopes are implemented as different atoms (that's aligned with the behavior that the same atom has different independent value in different scope).

Before we further discuss that, maybe we can first check this example #32 (comment). I realize this rule may be harder to understand. I need to figure out if this example makes sense before discussing implementations.

@dmaskasky
Copy link
Member Author

dmaskasky commented May 12, 2024

Ok, I understand your concern.

We need base3's value to get the correct dependency atom of derived.
We need to know derived and base3's scope to get the correct value of base3.
We don't know base3's scope. The only way is to recursively iterate each scope, then figure out if the scope and the dependency can be simultaneously determines in the iterating scope.

const derived = atom(get => get(base3 > 0 ? base1 : base2));

<ScopeProvider atoms={[derived]}> // layer1
  <ScopeProvider atoms={[base3]}> // layer2
    <ScopeProvider atoms={[base2]}> // layer3
      <ScopeProvider atoms={[base1]}> // layer4
        <Counter />
      </ScopeProvider>
    </ScopeProvider>
  </ScopeProvider>
</ScopeProvider>

There's actually two ways to handle this.

  1. recurse each scope then figure out if the atom dependency has a scope and which is the closest scope. O(n) read, O(1) insert.
  2. maintain a flattened map of all scoped atoms at each scope. O(1) read, O(n) insert. All nearest scoped atoms in the current scope must be in the weakmap.

Both read and insert must be synchronous. Since insert is a one-time cost on the average case, we should optimize for read making (2) the better option.

@yf-yang
Copy link
Collaborator

yf-yang commented May 13, 2024

We already maintain the weakmap as you said in 2. The key here is "we don't know which atoms are in the dependencies", so even we know which atoms are scoped, we don't know if the atom being used is scoped or not.

Shall we discuss #32 (comment) first?

@dmaskasky
Copy link
Member Author

dmaskasky commented May 13, 2024

Yes. Another challenge is that if a derived atom subscribes to changes to a scoped atom. If that scoped atom gets removed from the atoms prop of the ScopeProvider, then the derived atom now points to the global atom, but isn't subscribed to it. I haven't checked if this is solved by router atom or not.

@yf-yang
Copy link
Collaborator

yf-yang commented May 13, 2024

then the derived atom now points to the global atom, but isn't subscribed to it.

It is already taken into considerations.

https://github.com/jotaijs/jotai-scope/tree/main/examples/02_removeScope

To make it work, we need to dynamically recompute everything when useAtom is called.

@dmaskasky
Copy link
Member Author

I'm working on a refactor to close this issue and also #24.

Are you on Discord? It would be great if we could offline some of this conversation not relevant to this issue. I have some questions specific to the current implementation.

You can find me on the Poimandres server as dmaskasky (previously dmaskasky#3642).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants