-
-
Notifications
You must be signed in to change notification settings - Fork 245
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
fix(core): improve DeepResolveType type #324
Conversation
This pull request is being automatically deployed with Vercel (learn more). 🔍 Inspect: https://vercel.com/pmndrs/valtio/9GwJNSE846Aw5KhMB3hac6GYE8oP |
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 3fbbeb7:
|
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.
Nice suggestion. Just curious, was the previous typing somehow causing a trouble?
Wow, that was a fast code review!
No, not at all. I am a freelance developer and am looking for state management libraries I could potentially recommend in future projects. |
src/vanilla.ts
Outdated
} | ||
: T | ||
: Readonly<T> |
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.
: Readonly<T> | |
: T |
Looks like we don't need this.
Check this out: https://tsplay.dev/NlxQXN
Or, please let us know the case it might break.
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 that's true, I actually hadn't studied the Readonly<>
type in detail.
While checking this I noticed that the Map
, Set
and Date
types aren't immutable with the current implementation. Do you think we should cover those cases as well, or would that be overkill?
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.
They are indeed not immutable. So, it's correct. Actually, it's not deterministic. With #321, we could technically change the behavior at runtime. So, we can't provide 100% static type anyway.
I think it would be a nice addition to declare snapshots as immutable on Typescript level. This makes it easier for users to notice that they are only supposed to modify the store, not the snapshot.
I also took the liberty to rename the type to
Snapshot
, it's probably better thanDeepResolveTypeReadonly
.Awesome library by the way!