refactor(types): make `Snapshot<T>` use read-only collections

[cAttte]

hey! this PR changes the Snapshot<T> type so that any collection types (Map, Set, WeakMap, & WeakSet) will be made read-only as well (i.e. omit mutating methods). this helps catch mistakes like the following:

const snap = useSnapshot<{ users: Set<User> }>(store)
snap.users.delete(user.id) // now, "Property 'delete' does not exist"

this is technically a build-breaking change, just like adding readonly was, but any users affected by typed immutability have probably fixed it already. i decided to go with two internal types (Collection and Readonly<T>, which uses Omit<T>), resulting in slightly uglier type names (e.g. Readonly<Set<T>> vs. the native ReadonlySet<T>), but nicer typedefs.

• [x] npm run prettier

[vercel[bot]]

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

Name	Status	Preview	Comments	Updated (UTC)
valtio	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Feb 9, 2024 1:22am

[codesandbox-ci[bot]]

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

Sandbox	Source
React	Configuration
React TypeScript	Configuration
React Browserify	Configuration
React Snowpack	Configuration
React Parcel	Configuration

[dai-shi]

Map and Set are not read-only, so it's not correct. In general, we don't recommend using Maps, Sets and so on, and the reason for typing is just to be precise. It feels like there's room for improvement in docs.

[cAttte]

@dai-shi, sorry for not making myself clear enough. i'm not actually using Map and Set directly (those are indeed unfreezable and untrackable which, i'm guessing, is why you say they're not recommended); i'm using valtio's proxyMap() and proxySet(), which do get frozen because their underlying data properties get frozen (but you probably already knew that):

const store = proxy({ set: proxySet() })
const snap = snapshot(store)
snap.set.add("!") // TypeError: ... object is not extensible

this is the type of case that should be reflected by the typings. i think the main issue lies in the fact that proxy{Set,Map}() return objects with an interface identical to their native counterparts, which due to duck-typing makes the pairs indistinguishable.

i think that there are only two solutions:

• merging this PR as-is (except for the Weak* typings, since they don't have proxy simulations; mistake on my part). as you say, Maps and Sets aren't supported anyway so i don't see why the typings should tailor to this usage; it should be assumed that any mention of Map actually refers to proxyMap(), and in the strange case it doesn't, at least the user should be hinted at not mutating the objects, even if it isn't faithful to the run-time interface.
• narrowing (or "branding") the return type of the proxy simulations, for example by adding a __proxy_collection__: unique symbol property. this would look ugly in IntelliSense, but it would allow you to distinguish between ProxyMap and the broader Map. this would be backwards-compatible for the general use-case, because any existing references to the broader Map would keep working, Snapshot<Map> would remain mutable, and any narrower references to Snapshot<ProxyMap> would omit the mutating methods (which is a reasonable breaking change, because in this case it would definitely cause an error at run-time, as seen above)

[dai-shi]

Thanks for the clarification. I don't think there's an ultimate solution. https://tsplay.dev/mZBp9m

Branding might work, but we don't want to complicate our implementation.

Our suggestion is module augmentation. https://github.com/pmndrs/valtio/blob/26caa885050b5c9ac8c80c90e216599936e925f8/readme.md?plain=1#L56-L64

Define your MySnapshot type to support proxySet:

 declare module 'valtio' { 
   function useSnapshot<T extends object>(p: T): MySnapshot<T>
 } 

Do you know if it's possible to augment types when users import valtio/utils?

[cAttte]

yeah, it seems that just putting the augmentation in utils.ts works (and stops working when you remove the imports)

[dai-shi]

Nice. As we don't always want to augment if valtio/utils is imported, it would be better if we separate valtio/utils/proxySet. (and it's a breaking change.)

[dai-shi]

On second thought, I'm not sure if it's narrowing, but I'm probably okay with this:

diff --git a/src/vanilla.ts b/src/vanilla.ts
index aceafef..26c4fcd 100644
--- a/src/vanilla.ts
+++ b/src/vanilla.ts
@@ -5,8 +5,6 @@ const isObject = (x: unknown): x is object =>

 type AnyFunction = (...args: any[]) => any

-type AsRef = { $$valtioRef: true }
-
 type ProxyObject = object

 type Path = (string | symbol)[]
@@ -25,19 +23,20 @@ type SnapshotIgnore =
   | Set<any>
   | WeakMap<any, any>
   | WeakSet<any>
-  | AsRef
   | Error
   | RegExp
   | AnyFunction
   | Primitive

-type Snapshot<T> = T extends SnapshotIgnore
-  ? T
-  : T extends Promise<unknown>
-    ? Awaited<T>
-    : T extends object
-      ? { readonly [K in keyof T]: Snapshot<T[K]> }
-      : T
+type Snapshot<T> = T extends { $$valtioSnapshot: infer S }
+  ? S
+  : T extends SnapshotIgnore
+    ? T
+    : T extends Promise<unknown>
+      ? Awaited<T>
+      : T extends object
+        ? { readonly [K in keyof T]: Snapshot<T[K]> }
+        : T

 /**
  * This is not a public API.
@@ -403,9 +402,9 @@ export function snapshot<T extends object>(
   return createSnapshot(target, ensureVersion(), handlePromise) as Snapshot<T>
 }

-export function ref<T extends object>(obj: T): T & AsRef {
+export function ref<T extends object>(obj: T) {
   refSet.add(obj)
-  return obj as T & AsRef
+  return obj as T & { $$valtioSnapshot: T }
 }

 export const unstable_buildProxyFunction = buildProxyFunction

What do you think?

[dai-shi]

In case you missed it.