-
-
Notifications
You must be signed in to change notification settings - Fork 273
fix(vanilla, react): use experimental_use and some refactors #545
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
Changes from 16 commits
53d799d
05ebf63
10b8b6b
10c57d0
11509c5
e5d1f03
35ca1ea
5eaa309
9878289
7f68e12
6ec3ca5
d451288
2045a9f
f2d76a2
f3071fb
f89e534
934a028
8ed2378
d926e35
346ac60
36a8f1a
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,17 @@ | ||
import { useCallback, useDebugValue, useEffect, useMemo, useRef } from 'react' | ||
import { | ||
affectedToPathList, | ||
// eslint-disable-next-line @typescript-eslint/ban-ts-comment | ||
// @ts-ignore | ||
experimental_use, | ||
useCallback, | ||
useDebugValue, | ||
useEffect, | ||
useMemo, | ||
useRef, | ||
} from 'react' | ||
import { | ||
// affectedToPathList, | ||
createProxy as createProxyToCompare, | ||
getUntracked, | ||
isChanged, | ||
} from 'proxy-compare' | ||
// import { useSyncExternalStore } from 'use-sync-external-store/shim' | ||
|
@@ -14,6 +24,38 @@ import type { INTERNAL_Snapshot } from './vanilla' | |
|
||
const { useSyncExternalStore } = useSyncExternalStoreExports | ||
|
||
// customized version of affectedToPathList | ||
// we need to avoid invoking getters | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dai-shi ahhh, okay, this change (skipping getters in the output of I've been watching the output of This made me think "valtio is not working" / "doesn't know my getters were invoked", when obviously I could tell it did, b/c the re-renders still worked correctly. I finally reproduced the behavior in What was the rationale here? I'm sure it makes sense, just curious b/c it tripped me up. Wondering if I can work the rationale/behavior in a doc/"gotchas" update. Thanks! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Without it, it loops infinitely and hangs, AFAIR. |
||
const affectedToPathList = ( | ||
obj: unknown, | ||
affected: WeakMap<object, unknown> | ||
) => { | ||
const list: (string | symbol)[][] = [] | ||
const seen = new WeakSet() | ||
const walk = (x: unknown, path?: (string | symbol)[]) => { | ||
if (seen.has(x as object)) { | ||
// for object with cycles | ||
return | ||
} | ||
let used: Set<string | symbol> | undefined | ||
if (typeof x === 'object' && x !== null) { | ||
seen.add(x) | ||
used = affected.get(getUntracked(x) || x) as any | ||
} | ||
if (used) { | ||
used.forEach((key) => { | ||
if ('value' in (Object.getOwnPropertyDescriptor(x, key) || {})) { | ||
walk((x as any)[key], path ? [...path, key] : [key]) | ||
} | ||
}) | ||
} else if (path) { | ||
list.push(path) | ||
} | ||
} | ||
walk(obj) | ||
return list | ||
} | ||
|
||
const useAffectedDebugValue = ( | ||
state: object, | ||
affected: WeakMap<object, unknown> | ||
|
@@ -119,7 +161,7 @@ export function useSnapshot<T extends object>( | |
[proxyObject, notifyInSync] | ||
), | ||
() => { | ||
const nextSnapshot = snapshot(proxyObject) | ||
const nextSnapshot = snapshot(proxyObject, experimental_use) | ||
try { | ||
if ( | ||
!inRender && | ||
|
@@ -140,7 +182,7 @@ export function useSnapshot<T extends object>( | |
} | ||
return nextSnapshot | ||
}, | ||
() => snapshot(proxyObject) | ||
() => snapshot(proxyObject, experimental_use) | ||
) | ||
inRender = false | ||
const currAffected = new WeakMap() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DefinitelyTyped/DefinitelyTyped#62189
It seems that the typing of
experimental_use
is now added to the@types/react
, andts-ignore
could be removed.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, yeah, I updated there and there, but forgot here. Thanks.