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.

Sign up for GitHub

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): customizable atom.unstable_is #2356

Merged
merged 9 commits into from
Jan 24, 2024
Merged

feat(vanilla): customizable atom.unstable_is #2356

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
3339108
feat(vanilla): customizable atom.is
dai-shi Jan 23, 2024
85a8770
minor change
dai-shi Jan 23, 2024
3185a13
refactor
dai-shi Jan 23, 2024
b7b66bf
do not depend on this for atom.is
dai-shi Jan 23, 2024
de438fd
make default atom functions stable
dai-shi Jan 23, 2024
226ce3a
revert to this
dai-shi Jan 23, 2024
787b018
mark unstable for now
dai-shi Jan 23, 2024
3b6440d
Merge branch 'main' into feat/vanilla/customizable-atom-is
dai-shi Jan 23, 2024
2b2e3e7
Merge branch 'main' into feat/vanilla/customizable-atom-is
dai-shi Jan 24, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/vanilla/atom.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,7 @@ type OnMount<Args extends unknown[], Result> = <
export interface Atom<Value> {
toString: () => string
read: Read<Value>
is?: (a: Atom<unknown>) => boolean
Copy link
Member Author

Choose a reason for hiding this comment

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

I thought about requiring this property rather than making it optional, but as we want to release this as a patch, let's do this.
Eventually, if it makes sense (like moving to class syntax), we can consider changing it a required property.

Copy link
Contributor

@yf-yang yf-yang Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

A scenario needs to be considered: A global atom anAtom has two copies copy1 and copy2, it is possible that copy1 internally calls set(copy2, value) (so copy1.is(copy2) should evals to true). Therefore, even with this change, we still needs to add another property in jotai-scope's intercepted copy to mark two copies as having the same original atom.

Copy link
Member Author

@dai-shi dai-shi Jan 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So, does it mean it requires to be is?: (self: Atom<unknown>, target: Atom<unknown>) => boolean and doesn't use this?

Copy link
Contributor

Choose a reason for hiding this comment

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

Well, for the jotai-scope PR, there is no need to do so.
Personally, I think the term should not be called is, as is suggests equality and equality is symmetric, while in this case it is not.
https://doc.rust-lang.org/std/cmp/trait.PartialEq.html

debugLabel?: string
/**
* To ONLY be used by Jotai libraries to mark atoms as private. Subject to change.
Expand Down
12 changes: 8 additions & 4 deletions src/vanilla/store.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,10 @@ type OnUnmount = () => void
type Getter = Parameters<AnyAtom['read']>[0]
type Setter = Parameters<AnyWritableAtom['write']>[1]

const defaultAtomIs = function (this: AnyAtom, a: AnyAtom) {
return a === this
}

const hasInitialValue = <T extends Atom<AnyValue>>(
atom: T,
): atom is T & (T extends Atom<infer Value> ? { init: Value } : never) =>
Expand Down Expand Up @@ -199,7 +203,7 @@ export const createStore = () => {
const dependencies: Dependencies = new Map()
let changed = false
nextDependencies.forEach((aState, a) => {
if (!aState && a === atom) {
if (!aState && (atom.is || defaultAtomIs).call(atom, a)) {
aState = nextAtomState
}
if (aState) {
Expand Down Expand Up @@ -365,7 +369,7 @@ export const createStore = () => {
// If all dependencies haven't changed, we can use the cache.
if (
Array.from(atomState.d).every(([a, s]) => {
if (a === atom) {
if ((atom.is || defaultAtomIs).call(atom, a)) {
return true
}
const aState = readAtomState(a)
Expand All @@ -381,7 +385,7 @@ export const createStore = () => {
const nextDependencies: NextDependencies = new Map()
let isSync = true
const getter: Getter = <V>(a: Atom<V>) => {
if ((a as AnyAtom) === atom) {
if ((atom.is || defaultAtomIs).call(atom, a)) {
const aState = getAtomState(a)
if (aState) {
nextDependencies.set(a, aState)
Expand Down Expand Up @@ -524,7 +528,7 @@ export const createStore = () => {
...args: As
) => {
let r: R | undefined
if ((a as AnyWritableAtom) === atom) {
if ((atom.is || defaultAtomIs).call(atom, a)) {
if (!hasInitialValue(a)) {
// NOTE technically possible but restricted as it may cause bugs
throw new Error('atom not writable')
Expand Down
Loading