jotaijs / jotai-scope

MIT License
55 stars 4 forks source link

feat(scope): A complete rewrite of ScopeProvider (#16, #17) #20

Closed yf-yang closed 7 months ago

yf-yang commented 7 months ago

fixes #16, #17 depends on https://github.com/pmndrs/jotai/pull/2356 To give it a try, use [pnpm add, yarn add, npm i] https://pkg.csb.dev/pmndrs/jotai/commit/3b6440d1/jotai

Root Cause Analysis

Take a look of this atom:

function atomWithReducer(initialValue, reducer) {
  const anAtom = atom(initialValue, (get, set, action) =>
    set(anAtom, reducer(get(anAtom), action)),
  )
  return anAtom
}
const myAtom = atomWithReducer(...);

// somewhere in a React component
const dispatch = useSetAtom(myAtom);

Note that the condition myAtom === anAtom evals to true, which means they are exactly the same atom. Then, why dispatch(action) calls reducer(action), but set(anAtom, reducedValue) directly updates the value of anAtom instead of calling reducer(reducedValue)?

The key is here: https://github.com/pmndrs/jotai/blob/b5c38081e922b56621c6da9b9c32ef3d0f46e4b3/src/vanilla/store.ts#L527, a === atom check

const writeAtomState = <Value, Args extends unknown[], Result>(
  atom: WritableAtom<Value, Args, Result>,
  ...args: Args
): Result => {
  let isSync = true
  const getter: Getter = <V>(a: Atom<V>) => returnAtomValue(readAtomState(a))
  const setter: Setter = <V, As extends unknown[], R>(
    a: WritableAtom<V, As, R>,
    ...args: As
  ) => {
    let r: R | undefined
    if ((a as AnyWritableAtom) === atom) {
      if (!hasInitialValue(a)) {
        // NOTE technically possible but restricted as it may cause bugs
        throw new Error('atom not writable')
      }
      const prevAtomState = getAtomState(a)
      const nextAtomState = setAtomValueOrPromise(a, args[0] as V)
      if (!isEqualAtomValue(prevAtomState, nextAtomState)) {
        recomputeDependents(a)
      }
    } else {
      r = writeAtomState(a as AnyWritableAtom, ...args) as R
    }
    if (!isSync) {
      const flushed = flushPending()
      if (import.meta.env?.MODE !== 'production') {
        storeListenersRev2.forEach((l) =>
          l({ type: 'async-write', flushed: flushed as Set<AnyAtom> }),
        )
      }
    }
    return r as R
  }
  const result = atom.write(getter, setter, ...args)
  isSync = false
  return result
}

The line (a === atom check) checks if an atom is setting itself, so instead of calling the setter again, it directly update the value.

TODO

codesandbox-ci[bot] commented 7 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 93f87be17468d790d9920d40b8595d910ab0fe38:

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

Hi, I think you can become a maintainer of jotai-scope. I just sent an invitation.

yf-yang commented 7 months ago

This solution is still incomplete. I write another example 06 to illustrate it. When 06 get fixed, we can guarantee both write and read are correct.

dai-shi commented 7 months ago

To give it a try, use [pnpm add, yarn add, npm i] https://pkg.csb.dev/pmndrs/jotai/commit/3185a131/jotai

I see. You can change package.json if you want.

yf-yang commented 7 months ago

@dai-shi any comments?

yf-yang commented 7 months ago

Yes, ready to release.

dai-shi commented 7 months ago

Do you want to release? I can tell you my workflow. Or, I can do it this time, no problem. Whichever you want.

yf-yang commented 7 months ago

Hmmm, then would you please tell me the workflow? I'm interested in it.

yf-yang commented 7 months ago

Seems to be:

  1. Squash and merge
  2. Bump version to 0.5.0 and add changelog
  3. npm publish --access public
dai-shi commented 7 months ago
  1. Squash and merge
  2. Update CHANGELOG manually
  3. Update package.json
  4. Tag v0.x.y
  5. Push it and CD will fire