jotaijs / jotai-scope

MIT License
55 stars 4 forks source link

Refactor ScopeProvider #33

Closed dmaskasky closed 3 months ago

dmaskasky commented 4 months ago

Overview of Behavior

atoms: a, b, c, d, A(c + d), B(a + b + A)

S1[c]: a0, b0, c1, d0, A0(c1 + d0), B0(a0 + b0 + A0(c1 + d0))
S2[B]: a0, b0, c1, d0, A0(c1 + d0), B2(a2 + b2 + A2(c2 + d2))
S3[a]: a3, b0, c1, d0, A0(c1 + d0), B2(a3 + b2 + A2(c2 + d2))
S4[A]: a3, b0, c1, d0, A4(c4 + d4), B2(a3 + b2 + A4(c4 + d4))
S5[d]: a3, b0, c1, d5, A4(c4 + d5), B2(a3 + b2 + A4(c4 + d5))
-------------------------------------------------------------
S1:
  explicit: [c1]
  implicit: []
  inherited: [0 => [a0, b0, d0, B0, A0]]
S2:
  explicit: [B2]
  implicit: [a2, b2, c2, d2, A2],
  inherited: [0 => [a0, b0, d0, A0], 1 => [c1]]
S3:
  explicit: [a3]
  implicit: []
  inherited: [0 => [b0, d0, A0], 1 => [c1], 2 => [b2, c2, d2, A2, B2]]
S4:
  explicit: [A4]
  implicit: [c4, d4]
  inherited: [0 => [b0, d0], 1 => [c1], 2 => [b2, c2, d2, B2], 3 => [a3]]
S5:
  explicit: [d5]
  implicit: []
  inherited: [0 => [b0], 1 => [c1], 2 => [b2, B2], 3 => [a3],  4 => [c4, A4]]

Let us assume we have base atoms a, b, c, d and derived atoms A and B; { where A depends on c and d, and B depends on a, b, and A }

Scope S0 is the global scope under the nearest jotai Provider or defaultStore. Scopes S1-Sn are nested where S1 is the first level and S5 is a descendant of S1 at the fifth level.

In scope S1, primitive atom c is explicitly scoped. This means that c1 corresponding to c in S1 holds an independent value. All derived atoms in this scope will use c1. All descendant scopes will use c1 unless c is explicitly defined in a nearer ancestor.

In scope S2, derived atom B is explicitly scoped. This means that all of its dependents are implicitly scoped. Explicitly and implicitly scoped atoms are copied and do not inherit their value from the parent scope. This is why all dependents of B2 are denoted with a 2 suffix.

In scope S3, primitive atom a is explicitly scoped. All other atoms are inherited from S2 where B is explicitly scoped. Even though B2 is inherited, it still will use a3 in S3. This is a similar behavior to B0 using c1 in S1 with the only difference being B0 is unscoped vs B2 is inherited. To me, there is no difference between inherited and unscoped.

And so on... All subsequent scopes feature similar behavior as described above.

Summary

  1. fixes https://github.com/jotaijs/jotai-scope/issues/24

  2. fixes https://github.com/jotaijs/jotai-scope/issues/32

  3. fixes https://github.com/pmndrs/jotai/issues/2458#issuecomment-2025784135

  4. fixes: inherited atoms that can hold a value (Primitive, Writable), are reused in the nested scopes

  5. fixes: ScopeProvider shouldn't read parent scope past a Jotai Provider.

Implementation

Scope

createScope handles everything dealing with scope and is moved to a separate file.

WeakMaps

  1. explicit - atoms added to ScopeProvider
  2. implicit - dependencies of explicit and implicit atoms are scoped
  3. unscoped / inherited
    • scoped atoms of the parent scope (explicit, implicit, inherited)
    • derived readable and derived writeable atoms, needed for enabling access to scoped atoms

getAtom

from the originalAtom, gets the explicit, implicit, inherited, unscoped derived, and unscoped primitive atoms. As necessary, creates scoped copies (one-time) and saves them in their appropriate weakmap.

Inheriting Atoms

Atoms are inherited from the ancestor scopes. The top ancestor is the global scope where the original atom is used.

Inherited Derived Atoms

To inherit atoms that have a custom read, they are copied so that the custom read and write functions can use that scope's getAtom function.

Inheriting Primitive Atoms

To inherit primitive atoms, those atoms are stored directly and not copied. Since the atom itself is the key to its value, we need to store the original in the weakmap.

Inherited Write Only Atoms

const valuedWritableAtom = atom(0, (get, set) => {
  set(valuedWritableAtom, get(someAtom))
})

Where someAtom could be scoped.

⚠️ Dirty Hack For write-only atoms, since these atoms also hold a value, we also want to preserve the originals. However the write function may access atoms in the current scope. To work around this, we modify the original write method synchronous before baseStore.set and restore the original write method synchronous after in a finally block.

What I'd love is a utility to clone an atom but have the clone also resolve the same value as the original. That way I can copy atoms like below and wrap their write method to resolve atoms in the current scope.

Tests

adds tests for:

  1. scoped derived uses nested scope dependency
  2. derived dependency scope is preserved in self reference
  3. ScopeProvider provides isolation for scoped primitive atoms
  4. unscoped derived can read and write to scoped primitive atoms
  5. unscoped derived can read both scoped and unscoped atoms
  6. dependencies of scoped derived are implicitly scoped
  7. scoped derived atoms can share implicitly scoped dependencies
  8. nested scopes provide isolation for primitive atoms at every level
  9. unscoped derived atoms in nested scoped can read and write to scoped primitive atoms at every level
  10. inherited scoped derived atoms can read and write to scoped primitive atoms at every nested level
  11. implicit parent does not affect unscoped
  12. scoped writable atoms can read scoped atoms and themselves
codesandbox-ci[bot] commented 4 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.

dmaskasky commented 3 months ago

All tests passing!

yf-yang commented 3 months ago

Thanks for the PR, the code is now well organized. Needs some time to think about what the theoretical model of current implementation is.

dai-shi commented 3 months ago

⚠️ Dirty Hack For write-only atoms, since these atoms also hold a value, we also want to preserve the originals.

What's the difference between a normal derived atom (with read&write functions) and a write-only atom? We don't have any specific logic with write-only atoms in core. Write-only atoms are simply a usage pattern.

dmaskasky commented 3 months ago

⚠️ Dirty Hack For write-only atoms, since these atoms also hold a value, we also want to preserve the originals.

What's the difference between a normal derived atom (with read&write functions) and a write-only atom? We don't have any specific logic with write-only atoms in core. Write-only atoms are simply a usage pattern.

// A: primitive
// { read: defaultRead, write: defaultWrite }
atom(0)

// B: custom write
// { read: defaultRead, write: customWrite }
atom(0, function customWrite(get, set) {})

// C: custom read
// { read: customRead }
atom(function customRead(get) {})

// D: custom read and write
// { read: customRead, write: customWrite }
atom(
  function customRead(get) {},
  function customWrite(get, set) {}
)

atoms type A & B can hold a value. Type B can also reference atoms in the current scope. When a scope inherits a valued atom (A or B), the inherited value is used.

Why is this important?

Scope works by intercepting the read and write methods of the atom config to access the scoped variant of the original atom.

For Type B, we need to:

  1. use the original atom config to keep using the same value
  2. override the write function

I'm generally not a fan of mutating configs but I don't see any other way, unless Jotai natively supports creating a cloned copy of an atom whose config references the original atom's value in the store.

dai-shi commented 3 months ago

What's the difference between a normal derived atom (with read&write functions) and a write-only atom?

Oooops, that's my bad. A normal derived atom and a write-only atom are different. I wanted to say "a primitive atom" and a write-only atom. But, yeah, it's different for this library.

dai-shi commented 3 months ago

To work around this, we modify the original write method synchronous before baseStore.set and restore the original write method synchronous after in a finally block.

Sounds like a cool hack. But, I agree it's a last resort. I wonder if or if not unstable_is helps here.

dmaskasky commented 3 months ago

Sounds like a cool hack. But, I agree it's a last resort. I wonder if or if not unstable_is helps here.

unstable_is didn't work for me but it seems like it should.

Could it be this? https://github.com/pmndrs/jotai/blob/main/src/vanilla/store.ts#L418

I don't think this assumption is still valid https://github.com/pmndrs/jotai/pull/2371#issuecomment-1919200882

dai-shi commented 3 months ago

Let's wait for @yf-yang 's thought.

yf-yang commented 3 months ago

Review todo:

yf-yang commented 3 months ago

@dmaskasky Check this out https://codesandbox.io/p/sandbox/elated-hellman-kkpx95?file=%2Fsrc%2FApp.tsx%3A18%2C1

Try increase base2 at layer2, it is unexpectedly scoped. We expect it to be globally shared.

If you change the order of useAtom(derivedAtom) and useAtom(baseAtom2), you would see that its behavior will change. That's the problem I mentioned before: order matters in current implementation (because using derived atom will cause its dependencies being added to implicit)

yf-yang commented 3 months ago

@dai-shi BTW, do you know how to install a csb bot built pkg (with its commit id) in the csb? I didn't find the correct approach so I have to copy all the codes.

dmaskasky commented 3 months ago

@dmaskasky Check this out https://codesandbox.io/p/sandbox/elated-hellman-kkpx95?file=%2Fsrc%2FApp.tsx%3A18%2C1

Try increase base2 at layer2, it is unexpectedly scoped. We expect it to be globally shared.

If you change the order of useAtom(derivedAtom) and useAtom(baseAtom2), you would see that its behavior will change. That's the problem I mentioned before: order matters in current implementation (because using derived atom will cause its dependencies being added to implicit)

I'll look into it. I explicitly wrote the code in a way to protect against this possibility. I believe there is a bug in my code somewhere. I'll update the code and add a test case for this.

I tried all four combinations of layer 1, layer 2 ordering of base2 and derived hooks and got this result. For all four cases I incremented baseAtom2 at layer 2.

 L1(D, 2), L2(D, 2): 2 is scoped to L2
 L1(D, 2), L2(2, D): 2 is NOT scoped
 L1(2, D), L2(2, D): 2 is NOT scoped
 L1(2, D), L2(D, 2): 2 is scoped to L2
dai-shi commented 3 months ago

@dai-shi BTW, do you know how to install a csb bot built pkg (with its commit id) in the csb? I didn't find the correct approach so I have to copy all the codes.

Do you mean csb bot to build sandbox links? I think it's no longer supported. It still builds packages that we can install: https://ci.codesandbox.io/status/jotaijs/jotai-scope/pr/33/

yf-yang commented 3 months ago

It may not be a bug. I'm still trying to understand how the new implementation works.

Generally speaking when resolving an atom there are only two directions. Vertically (to its parent scope copy) and horizontally (to its current scope dependency). The case here is because implicit is set both vertically and horizontally, so if it is vertically resolved first, it would be resolved to the global one. If it is horizontally resolved first, it would be a scoped one.

That's the fundamental problem. Although the code is refactored a lot, the resolution patterns are still combinations of those two patterns.

yf-yang commented 3 months ago

Do you mean csb bot to build sandbox links

I mean, in this PR the csb bot generates a pkg using commit abc123, how can I install that specific version when I create an online demo?

dai-shi commented 3 months ago

You can install it with npm i https://pkg.csb.dev/jotaijs/jotai-scope/commit/f6cf9907/jotai-scope. I think it works with StackBlitz (webcontainer) too.

image
dmaskasky commented 3 months ago

Generally speaking when resolving an atom there are only two directions. Vertically (to its parent scope copy) and horizontally (to its current scope dependency). The case here is because implicit is set both vertically and horizontally, so if it is vertically resolved first, it would be resolved to the global one. If it is horizontally resolved first, it would be a scoped one.

I found the solution. inherited atoms needs to be stored by their implicit scope. so

const inherited = new Map<AnyAtom, [AnyAtom, Scope?]>()

becomes

const inherited = new Map<Scope, new Map<AnyAtom, [AnyAtom, Scope?]>>()
dmaskasky commented 3 months ago

Generally speaking when resolving an atom there are only two directions. Vertically (to its parent scope copy) and horizontally (to its current scope dependency). The case here is because implicit is set both vertically and horizontally, so if it is vertically resolved first, it would be resolved to the global one. If it is horizontally resolved first, it would be a scoped one.

@yf-yang, @dai-shi I added a test case for this that tries all 4 combinations of hook ordering. It was failing as expected and in the manner expected.

The latest commit includes this test and the fix. We should be good to go, unless we can find other edge cases.

I've been playing around with your handy tool. I haven't been able to find any other issues. I believe this is now a comprehensive solution. Please let me know what you think.

yf-yang commented 3 months ago

Sure, will come back to you later

dmaskasky commented 3 months ago

@yf-yang, @dai-shi I would be happy to answer any questions you may have on the change, theory or implementation. Its complicated, I know, and its also as simple as I can make it. Maybe you could be refine this, make it cleaner. In any case, I think its at least a great starting point and worthy of your review.

dmaskasky commented 3 months ago

@yf-yang Check this out https://codesandbox.io/p/sandbox/elated-hellman-forked-z3dkvl?file=%2Fsrc%2FApp.tsx%3A25%2C39

Try now increase base2 at layer2, as expected it is globally shared. 😊

yf-yang commented 3 months ago

So coming back to the example:

d(a1 + a2)

<ScopeProvider atoms={[derived]}> // S1
    <ScopeProvider atoms={[atom1]}> // S2
      <Counter />
    </ScopeProvider>
</ScopeProvider>

in S2, d will access S2 a1 and no a2, how do we explain this phenomenon?

yf-yang commented 3 months ago

https://codesandbox.io/p/sandbox/festive-dust-5hnqmx?file=%2Fsrc%2Findex.tsx

Layer 1, write is broken

yf-yang commented 3 months ago

Anyway, that's a write problem.

I am still trying to understand why/how read works. Seems scopeKey is only used to distinguish direct store.get and hooked atom.read.

dmaskasky commented 3 months ago

Anyway, that's a write problem.

I am still trying to understand why/how read works. Seems scopeKey is only used to distinguish direct store.get and hooked atom.read.

GlobalScopeKey represents the scope key for unscoped atoms. Explicitly scoped atoms are stored with their level's scope.

Custom Read and write fns of explicitly scoped atoms pass the explicit atom's scope as the implicit scope to getAtom. This is how implicit scope works for their atom dependencies.

dmaskasky commented 3 months ago

https://codesandbox.io/p/sandbox/festive-dust-5hnqmx?file=%2Fsrc%2Findex.tsx

Layer 1, write is broken

I think you forgot to copy ScopeProvider code over to this example. The scope.ts code looks current, but ScopeProvider.tsx code looks stale.

After updating the ScopeProvider code, it was working correctly. I took your example and added extra debug info to make it a little easier what's going on. The example is behaving correctly.

https://codesandbox.io/p/sandbox/festive-dust-forked-8t5k5x

yf-yang commented 3 months ago

I now understand how it works, nice catch!

Generally speaking, we are following this pattern. image

One drawback of this pattern is, it cannot deal with scoped derived atom. If an atom is scoped, then when traversing its dependency, we don't preserve any information about the dependent's scope, so we will get the global atom.

So, when hitting the derived scoped atom, its scope is recorded (the green line). When visiting its dependency, if its dependency hits the green line, the dependency would be resolved to the scope atom in the green line. Alternatively, if the dependency hits a deeper scoped atom before the green line, it would still use the deeper scoped atom.

image

yf-yang commented 3 months ago

The core mechanism can be simplified, but let's stick to this pr for now.

yf-yang commented 3 months ago

Have you checked #24? I think it could be solved now?

dmaskasky commented 3 months ago

One drawback of this pattern is, it cannot deal with scoped derived atom. If an atom is scoped, then when traversing its dependency, we don't preserve any information about the dependent's scope, so we will get the global atom.

This behavior was intentional actually. Say we had the following,

const base = atom(0)
const derived = atom((get => get(base))
const App = () => {
  return (
    <ScopeProvider atoms={[derived]} debugName="S1">
      <ScopeProvider atom={[base]} debugName="S2">
        <Counter />
      </ScopeProvider>
    </ScopeProvider>
  )
}

It makes sense that derived in S2 would read base scoped to S2. This is consistent with:

const base = atom(0)
const derived = atom((get => get(base))
const App = () => {
  return (
    <ScopeProvider atom={[base]} debugName="S1">
      <Counter />
    </ScopeProvider>
  )
}

Where now derived is not scoped and base is scoped to S1. Existing behavior is that derived would use base from S1 not base from global scope.

So to make these two cases consistent, we must follow the rule that nested scopes override ancestor scopes. This is the original motivation of this PR and addresses https://github.com/jotaijs/jotai-scope/issues/32.

dmaskasky commented 3 months ago

Yes https://github.com/jotaijs/jotai-scope/issues/24 is fixed. https://codesandbox.io/p/sandbox/jotai-forked-fc7kns

dmaskasky commented 3 months ago

May I please have write access?

dai-shi commented 3 months ago

May I please have write access?

I can do that if @yf-yang is okay.

yf-yang commented 3 months ago

May I please have write access?

I'm OK with that. Do I have the team manage access? Didn't find it.

dai-shi commented 3 months ago

May I please have write access?

Done!

dmaskasky commented 3 months ago

I added a test for scoped writable and replaced the patched store symbol with instanceof.

dmaskasky commented 3 months ago

Need one more approval please 🙏