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(scope): A complete rewrite of ScopeProvider (#16, #17) #20

Merged
merged 16 commits into from
Jan 26, 2024

Conversation

yf-yang
Copy link
Collaborator

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

fixes #16, #17
depends on pmndrs/jotai#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

  • test with examples
  • refactor the code structure
  • change some obscure parameter names
  • add in-code docs

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 93f87be:

Sandbox Source
React Configuration
React TypeScript Configuration

@dai-shi
Copy link
Member

dai-shi commented Jan 22, 2024

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

@yf-yang
Copy link
Collaborator Author

yf-yang commented Jan 22, 2024

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.

@yf-yang yf-yang changed the title refactor(scope): change the logic when writing atoms feat(scope): A complete rewrite of ScopeProvider Jan 23, 2024
@yf-yang yf-yang marked this pull request as ready for review January 23, 2024 02:57
@yf-yang yf-yang requested a review from dai-shi January 23, 2024 03:03
@dai-shi
Copy link
Member

dai-shi commented Jan 23, 2024

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.

type Store = ReturnType<typeof getDefaultStore>;
export const ScopeContext = createContext<readonly [GetStoreKey, Store]>([
(a) => a,
getDefaultStore(),
Copy link
Member

Choose a reason for hiding this comment

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

Just a nit, but getDefaultStore is designed with lazy initialization in mind, but this forces to initialize it at the module load.

Copy link
Collaborator Author

@yf-yang yf-yang Jan 23, 2024

Choose a reason for hiding this comment

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

@@ -59,14 +62,15 @@
"eslint": "^8.52.0",
"eslint-config-airbnb": "^19.0.4",
"eslint-config-prettier": "^9.0.0",
"eslint-import-resolver-typescript": "^3.6.1",
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added this resolver for eslint-plugin-import

@yf-yang yf-yang requested a review from dai-shi January 24, 2024 13:24
@yf-yang
Copy link
Collaborator Author

yf-yang commented Jan 25, 2024

@dai-shi any comments?

Copy link
Member

@dai-shi dai-shi left a comment

Choose a reason for hiding this comment

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

Thanks a lot for working on this.
It looks pretty clean.

Is it ready to release?

.eslintrc.json Outdated Show resolved Hide resolved
src/ScopeProvider.tsx Outdated Show resolved Hide resolved
...args,
);
},
}),
// eslint-disable-next-line camelcase
unstable_is: (a: AnyAtom) => isSelfAtom(a, originalAtom),
Copy link
Member

Choose a reason for hiding this comment

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

Interesting to see that this doesn't use this. Yeah, because it already know originalAtom.

@yf-yang
Copy link
Collaborator Author

yf-yang commented Jan 26, 2024

Yes, ready to release.

@dai-shi
Copy link
Member

dai-shi commented Jan 26, 2024

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
Copy link
Collaborator Author

yf-yang commented Jan 26, 2024

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

@yf-yang
Copy link
Collaborator Author

yf-yang commented Jan 26, 2024

Seems to be:

  1. Squash and merge
  2. Bump version to 0.5.0 and add changelog
  3. npm publish --access public

@yf-yang yf-yang changed the title feat(scope): A complete rewrite of ScopeProvider feat(scope): A complete rewrite of ScopeProvider (#16, #17) Jan 26, 2024
@yf-yang yf-yang merged commit 432678d into jotaijs:main Jan 26, 2024
2 checks passed
@yf-yang yf-yang deleted the reducer_fix branch January 26, 2024 08:46
@yf-yang yf-yang mentioned this pull request Jan 26, 2024
@dai-shi
Copy link
Member

dai-shi commented Jan 26, 2024

  1. Squash and merge
  2. Update CHANGELOG manually
  3. Update package.json
  4. Tag v0.x.y
  5. Push it and CD will fire

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.

Can't use with reducer
2 participants