jotaijs / jotai-scope

MIT License
60 stars 4 forks source link

fix unexpected isolation #18

Closed ImSingee closed 10 months ago

ImSingee commented 11 months ago

This fixes #16

dai-shi commented 11 months ago

Hi, thanks for working on this! Can you quickly explain what was the bug? It seems that the PR diff includes both a fix and a refactor.

ImSingee commented 11 months ago

@dai-shi In fact, I can't figure out the real bug. I have read every code (both jotai, jotai-utils, jotai-scope), but all seem correct.

Finally, I guess that's because of the redundant createScopedAtom on the non-isolated atom, so I try to delete them (do a small refactor, mainly removing the un-need arguments and logic), and it works...

dai-shi commented 11 months ago

Okay, I will review the code again later.

codesandbox-ci[bot] commented 11 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 4903c1ee6d39c2a809a8eff9b5decef149bd8f44:

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

Hi, I updated examples/02. There, I want to have doubledAnotherCount be in sync with anotherCount.

image

It breaks with this PR change. However, I'm not sure if we can cover both cases.

ImSingee commented 11 months ago

@dai-shi In my opinion, it's the developer's duty to keep two associated atoms both isolated or unisolated.

One is isolated and another is unisolated can be treated as undefined behavior, and we should only say "do not do this thing" in the document, not try to solve the problem.

I think not creating unexpected isolation is far more than handling this edge case.

dai-shi commented 11 months ago

This is actually requested by users, and I think it's a requirement, not an undefined behavior. We strictly want to support, use cases like:

const countAtom = atom(0);
const anotherAtom = atom(1); // only this will be scoped
const mixedAtom = atom((get) => get(countAtom) + get(anotherAtom));

That's why the current implementation is very complicated. Maybe, it's impossible to cover all the edge cases.

If we want to isolate atoms totally, it would be much easier to implement with Provider and a React Context. We can explore such capability as a new util, but it's different from the goal of current ScopeProvider.

ImSingee commented 11 months ago

@dai-shi Got it!

For others, if you want to use the jotai-scope with reducer, you can use npm @singee/jotai-scope 0.4.3 ("jotai-scope": "npm:@singee/jotai-scope@0.4.3"). It's published based on this PR and can be used with reducer, but not with the above case.

I hope we can find a solution to both cases.

yf-yang commented 10 months ago

Fixed with #20