-
Notifications
You must be signed in to change notification settings - Fork 4
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
Refactor ScopeProvider #33
Refactor ScopeProvider #33
Conversation
0ca2cb8
to
579d615
Compare
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. |
af92d3b
to
4ee9719
Compare
- scoped derived uses nested scope dep - derived dep scope is preserved in self reference
5e51c00
to
3033f74
Compare
src/ScopeProvider/scope.ts
Outdated
const explicit = new WeakMap<AnyAtom, AnyAtom>(); | ||
const implicit = new WeakMap<AnyAtom, AnyAtom>(); | ||
const inherited = new WeakMap<AnyAtom, AnyAtom>(); | ||
const unscoped = new WeakMap<AnyAtom, AnyAtom>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In order of priority, descending:
explicit
- map of <originalAtom, scopedCopy>. atoms passed to ScopeProvider are explicit.implicit
- map of <originalAtom, scopedCopy>. Dependencies of explicit scoped atoms are implicitly scoped.inherited/unscoped
- map of <originalAtom, scopedCopy>. Scoped atoms in parent scopes are copied to this scope so they can access scoped atoms in this scope.
- map of <originalAtom, scopedCopy>. Unscoped derived atoms are copied so they can access scoped atoms in this scope.
src/ScopeProvider/scope.ts
Outdated
|
||
if ('read' in scopedAtom) { | ||
// inherited atoms should preserve their value | ||
if (scopedAtom.read === defaultRead) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaultRead
and defaultWrite
are methods on primitive atoms
3033f74
to
0169dee
Compare
ef8930c
to
65b47e4
Compare
@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. 😊 |
fb17e03
to
12a5636
Compare
src/ScopeProvider/index.ts
Outdated
@@ -0,0 +1 @@ | |||
export { ScopeContext, ScopeProvider } from './ScopeProvider'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think we need this file. Just import it directly from the library index. Though, it's just a preference.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done.
I also added exporting INTERNAL_ScopeContext from src/ScopeProvider/ScopeProvider.tsx
, but am intentionally excluding this export from the package. I hope this will be a good compromise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks.
As the current config in this repo bundles dist
, exporting only in the source file doesn't work.
Anyway, let's drop it: #33 (comment)
49f8d39
to
a6eae83
Compare
So coming back to the example:
<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? |
https://codesandbox.io/p/sandbox/festive-dust-5hnqmx?file=%2Fsrc%2Findex.tsx Layer 1, write is broken |
Anyway, that's a write problem. I am still trying to understand why/how read works. Seems |
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. |
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. |
a6eae83
to
1d767d1
Compare
The core mechanism can be simplified, but let's stick to this pr for now. |
Have you checked #24? I think it could be solved now? |
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 #32. |
Yes #24 is fixed. |
May I please have write access? |
I can do that if @yf-yang is okay. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, I don't understand the entire logic at all but it's fine. I don't want to block this pr (or this repo) anyways.
@yf-yang Feel free to merge and publish a new version.
My interest is how to support jotai-scope
with jotai
in an abstract way. (and our current idea is unstable_is
.)
Left some minor comments.
return 'write' in anAtom; | ||
} | ||
|
||
const { read: defaultRead, write: defaultWrite } = atom<unknown>(null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a nice hack, but ideally it would be nice if we could avoid this with unstable_is
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
defaultRead and defaultWrite are used to detect when an atom has a custom read or a custom write.
- This is important when determining whether to copy the atom or use the original.
unstable_is
will handle this case in the future. - It is also a performance enhancement since defaultRead's
this
is faster than using getAtom to resolve the atom.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we can avoid checking it later, but in the implementation of this PR, there are some special branches using that comparison, like inheritAtom
/** | ||
* @debug | ||
*/ | ||
toString?: () => string; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just curious how useful is this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for debugging, it is useful
I'm OK with that. Do I have the team manage access? Didn't find it. |
Done! |
I added a test for scoped writable and replaced the patched store symbol with instanceof. |
- add test for scoped writable
585006c
to
3562d75
Compare
Need one more approval please 🙏 |
Overview of Behavior
Summary
fixes atom derived from a scoped atom can't read it's own value #24
fixes Nested Scope Consistency Question #32
fixes Idea: lazy initialization for primitive atoms pmndrs/jotai#2458 (comment)
fixes: inherited atoms that can hold a value (Primitive, Writable), are reused in the nested scopes
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
explicit
andimplicit
atoms are scopedgetAtom
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
Where someAtom could be scoped.
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: