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

Branded primitive types are incompatible with Snapshot<T> #747

Closed
1 of 2 tasks
pastelmind opened this issue Jul 7, 2023 · 8 comments · Fixed by #748
Closed
1 of 2 tasks

Branded primitive types are incompatible with Snapshot<T> #747

pastelmind opened this issue Jul 7, 2023 · 8 comments · Fixed by #748

Comments

@pastelmind
Copy link
Contributor

pastelmind commented Jul 7, 2023

Summary

Branded/tagged types are often used to achieve nominal typing. (1, 2) Several libraries (e.g. type-fest, ts-essentials) offer utility types such as Opaque<T> to support this.

Unfortunately, the Snapshot<T> type in Valtio does not play nice with branded types. Specifically, a branded type becomes broken when passed through Snapshot<T>:

type Brand<T, B> = T & { __brand: B }
type UserId = Brand<string, 'UserId'>  // string & { __brand: 'UserId' }

const user = proxy<{ id: UserId, name: string }>({ /* ... */ })

const snapshot = useSnapshot(user)
snapshot.userId // The type is:
                //    {
                //        readonly [x: number]: string;
                //        readonly toString: (() => string) & (() => string);
                //        readonly charAt: (pos: number) => string;
                //        readonly charCodeAt: (index: number) => number;
                //        readonly concat: (...strings: string[]) => string;
                //        ... 46 more ...;
                //        readonly __brand: "UserId";
                //    }
doSomething(snapshot.userId) // Fails because the function expected a UserId
                             // but got a broken type

Link to reproduction

https://www.typescriptlang.org/play?#code/C4TwDgpgBAQgTgQwHYBMA8AVANLAfFAXigygDIoBvKAfWoCNFUAuWKAXwChRIoBVAZwhwAkikKxG6fsDgBLJAHMcAcgFDRy3By7hoauOIocoJqLJQt9ojpx099AZSQIw-ABYB7YOKcv3XtH0tAHpg0wA9AH5tUNgIABsPAHczfigAYw8wWQgxADM4DwBbJg5Yt2BgVyZQhVlgNwBXOgA6TKLgsCLUOH5ggDcEeOBZD2C6RLpggFYUAAYIdIQAdlz0gCZphBW8gEZlvLoADjmj9Yg9gBZ1y8uEAGZpvPTd9bp14P44dIHkWXj4ggWsB+NpuNAAIJIEAAMUaSHSIw8SHEAAoWhiEHAFPwWMgQABtAC6AEpCPh8WDdFAIfwAEoXQxQAAkzMGw1GDLyLBkjWgtnBUF8rk8wGECiQHjg0AIxigAB8oAARBDACByxUAWRcaHxOHxWhMiocEGAuuhhoVUAA6hAEABrbVgc0gfUWjU2u32k1mg0e2lcj0AUTghTgHoZCiDAA8wP7oXCEUikFSeML-GaMPgiCQING1ag0unReLJdK5ZFiHKWLn8xBC1AAAqFIqyQRoeH2yVJJCWysQpIIeq5TCWmtQPMFlBpDx0ABWi2AFcoUGlCBQyPiICgBIA0mYUfaICAPHliESWMWAhg90T8JwTDWgA

Check List

Please do not ask questions in issues.

  • I've already opened a discussion before opening this issue, or already discussed in other media.

Please include a minimal reproduction.

@pastelmind
Copy link
Contributor Author

Any type that extends object is recursively passed through Snapshot<T>.

valtio/src/vanilla.ts

Lines 31 to 37 in 5d0eca7

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

All branding/tagged/opaque types I've seen use an intersection with a faux property:

// Some libraries use '__tag' or a Symbol instead of '__brand'
type Opaque<T, B> = T & { __brand: B }

This passes the T extends object check in Snapshot<T>.

Since branded types are almost always used to wrap primitives, perhaps this could be worked around by special-casing primitive types?

type Primitive = string | number | boolean | null | undefined | symbol | bigint

type Snapshot<T> = T extends SnapshotIgnore
  ? T
  : T extends Promise<unknown>
  ? Awaited<T>
  : T extends Primitive
  ? T
  : T extends object
  ? { readonly [K in keyof T]: Snapshot<T[K]> }
  : T

@dai-shi
Copy link
Member

dai-shi commented Jul 7, 2023

Thanks for reporting.
That sounds like a good idea: https://tsplay.dev/Wobblm

@pastelmind
Copy link
Contributor Author

pastelmind commented Jul 7, 2023

Unfortunately that doesn't work with all type branding solutions. For one, the Opaque<T> type from type-fest (one of the most popular type utility libraries) is defined as:

declare const tag: unique symbol;                // not exported!
type Tagged<Token> = { readonly [tag]: Token };  // not exported!

export type Opaque<Type, Token> = Type & Tagged<Token>;

So directly targeting { __brand: unknown } doesn't cover all bases.

@dai-shi
Copy link
Member

dai-shi commented Jul 7, 2023

Do we have an ultimate solution? I thought we could only cover some typical cases. The last resort would be module augmentation, maybe.

@dai-shi
Copy link
Member

dai-shi commented Jul 7, 2023

I think we just let people do useSnapshot(...) as unknown as ....

@pastelmind
Copy link
Contributor Author

Adding primitive types to SnapshotIgnore would solve the common case (branded primitive types). Would it help?

type PrimitiveType = string | number | boolean | null | undefined | symbol | bigint;
type SnapshotIgnore = /* everything else */ | PrimitiveType;

@dai-shi
Copy link
Member

dai-shi commented Jul 7, 2023

Since branded types are almost always used to wrap primitives, perhaps this could be worked around by special-casing primitive types?

Ah, I see what you mean now.

Would you like to open a PR?

@pastelmind
Copy link
Contributor Author

Sure

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 a pull request may close this issue.

2 participants