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

feat(vanilla): Add a hack for jotai-scope #2353

Closed
wants to merge 3 commits into from

Conversation

yf-yang
Copy link
Contributor

@yf-yang yf-yang commented Jan 22, 2024

Reverts #2186
Add a property scopedOriginal and add a intercepted function to ensure copy of scoped atoms can directly set itself without calling its write function again. Now all the write function in jotai-scope get executed exactly once.
Related to jotaijs/jotai-scope#20

Copy link

vercel bot commented Jan 22, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
jotai ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jan 23, 2024 3:09am

Copy link

codesandbox-ci bot commented Jan 22, 2024

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 b938def:

Sandbox Source
React Configuration
React TypeScript Configuration
React Browserify Configuration
React Snowpack Configuration
Next.js Configuration
Next.js with custom Babel config Configuration
React with custom Babel config Configuration

@yf-yang yf-yang changed the title revert(vanilla): reverts #2186 feat(vanilla): Add a hack for jotai-scope Jan 23, 2024
@yf-yang yf-yang marked this pull request as ready for review January 23, 2024 02:55
@dai-shi
Copy link
Member

dai-shi commented Jan 23, 2024

First of all, thanks for working on this. Here's some notes.

  • My motivation behind jotai-scope is to completely avoid modifying Jotai core. However, what we found is it's extremely difficult and inefficient, right?
  • We don't need to revert feat(vanilla): prefer using this for atom config #2186 as it has its own capability (meaning it might be useful not only jotai-scope).
  • I'm not 100% sure if scopedOriginal is a good abstraction.
  • So, please allow me to draft a new PR.

@yf-yang
Copy link
Contributor Author

yf-yang commented Jan 23, 2024

completely avoid modifying Jotai core

As I state in jotaijs/jotai-scope#20, the shallow equality comparison is insufficient. Yesterday I thought about hours and figured out the key is to call atom.write EXACTLY once, which means this modification is unavoidable.

We don't need to revert #2186

I am not so sure, at least part of the change should be reverted. Let me illustrate the key difference:
Internally, if read function/write function is not specified, in #2186, it would be converted to something like: read = get => get(this). However, in a scoped copy atom copy of atom anAtom (we call if anAtomCopy), when it internally calls get(anAtom), with #2186, it would be transformed to get(anAtomCopy) instead of get(anAtom). That's the difference with this.

@dai-shi
Copy link
Member

dai-shi commented Jan 23, 2024

#2356 is something I would like to go with.
Would you please try it and see if it works for jotai-scope?

@dai-shi
Copy link
Member

dai-shi commented Jan 23, 2024

That's the difference with this.

And, it seems intuitive, no?

const copyAtom = (a) => ({ ...a });

const countAtom = atom(0);
const copiedAtom = copyAtom(countAtom);

  set(copiedAtom, 1);
  get(countAtom) // ---> should be `0`

@yf-yang
Copy link
Contributor Author

yf-yang commented Jan 23, 2024

Nope, the difference is here

jotai/src/vanilla/atom.ts

Lines 100 to 102 in b5c3808

config.read = function (get) {
return get(this)
}

and

jotai/src/vanilla/atom.ts

Lines 103 to 115 in b5c3808

config.write = function (
this: PrimitiveAtom<Value>,
get: Getter,
set: Setter,
arg: SetStateAction<Value>,
) {
return set(
this,
typeof arg === 'function'
? (arg as (prev: Value) => Value)(get(this))
: arg,
)
} as unknown as Write<Args, Result>

Originally I want to make sure the target argument of getAtom should not be the scoped copy, with them, it cannot be assured.

However, I think it can be solved by adding more conditions in jotai-scope.

@yf-yang
Copy link
Contributor Author

yf-yang commented Jan 23, 2024

The difference is not in store access, but in the parameter passed to setter and getter. Very hard to explain this, maybe after I write a documentation to explain how the scope works now.

@yf-yang
Copy link
Contributor Author

yf-yang commented Jan 23, 2024

anAtomCopy = {...anAtom};
store.set(anAtomCopy, 0);
// =>
anAtomCopy.write(getter, setter, 0);
// => 
anAtomCopy.write(getter, (a, 0) => set(a, 0), 0);
// with `this`, a here is `anAtomCopy`
// without `this`, a here is `anAtom`

@dai-shi
Copy link
Member

dai-shi commented Jan 23, 2024

I still want to keep #2186 as it feels right and gives more capability. So, I wish you can find a solution on the jotai-scope end.

@dai-shi
Copy link
Member

dai-shi commented Jan 23, 2024

by the way, you can override .write in jotai-scope, if you want. Maybe we should use the same function reference.

@@ -524,7 +532,7 @@ export const createStore = () => {
...args: As
) => {
let r: R | undefined
if ((a as AnyWritableAtom) === atom) {
if (getScopedOriginal(a) === getScopedOriginal(atom)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The same thing can be done with #2356, no?

anAtom.is = function(target) {
  return getScopedOriginal(this) === getScopedOriginal(target)
}

@yf-yang yf-yang closed this Jan 23, 2024
@yf-yang yf-yang deleted the revert-2186 branch January 23, 2024 06:22
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 this pull request may close these issues.

2 participants