fix(react): infer defaultValue as primitive type #223
Conversation
👷 Deploy request for slash-libraries pending review.Visit the deploys page to approve it
|
| export function useStorageState<T extends Serializable>( | ||
| key: string | ||
| ): readonly [Serializable<T> | undefined, (value: SetStateAction<Serializable<T> | undefined>) => void, () => void]; | ||
| ): readonly [T | undefined, (value: SetStateAction<T | undefined>) => void, () => void]; | ||
| export function useStorageState<T>( | ||
| key: string, | ||
| { storage, defaultValue }: StorageStateOptionsWithDefaultValue<T> | ||
| ): readonly [Serializable<T>, (value: SetStateAction<Serializable<T>>) => void, () => void]; | ||
| ): readonly [T, (value: SetStateAction<T>) => void, () => void]; | ||
| export function useStorageState<T>( | ||
| key: string, | ||
| { storage, defaultValue }: StorageStateOptions<T> | ||
| ): readonly [Serializable<T> | undefined, (value: SetStateAction<Serializable<T> | undefined>) => void, () => void]; | ||
| ): readonly [T | undefined, (value: SetStateAction<T | undefined>) => void, () => void]; | ||
| export function useStorageState<T>( | ||
| key: string, | ||
| { storage = safeLocalStorage, defaultValue }: StorageStateOptions<T> = {} | ||
| ): readonly [Serializable<T> | undefined, (value: SetStateAction<Serializable<T> | undefined>) => void, () => void] { | ||
| ): readonly [T | undefined, (value: SetStateAction<T | undefined>) => void, () => void] { |
There was a problem hiding this comment.
@Jinho1011
First of all, thank you for your interest in my issue.
Is there a reason for putting a generic constraint only in the first function definition? Could it be a mistake?
There was a problem hiding this comment.
Thank you for letting me know. I fixed the missing one and pushed a new commit with the changes.
|
Thanks for your contribution! |
| setState(curr => { | ||
| const nextValue = typeof value === 'function' ? value(curr) : value; | ||
| const nextValue = value instanceof Function ? value(curr) : value; |
There was a problem hiding this comment.
Is there a reason a typeof check is replaced with instanceof here?
There was a problem hiding this comment.
This change was caused by the mistake of generic constraint only on the first function among the overloading functions.
If generic T is not constrained to extend serializable, T will not be callable, so I modified it like this.
Since I fixed the generic constraint from the above conversation to be added to all overloaded functions, this change can be reversed.
There was a problem hiding this comment.
Thanks! What about reverting this change and making this pull request more focused on the main idea?
There was a problem hiding this comment.
I reverted the change as you said. Thank you for taking the time to review my pull request and provide feedback!
|
@Jinho1011 Could you please re-review? @raon0211 This PR didn't solve the issue #220 either: In fact, I also tried with a If you insert a type constraint that prevents passing a I think that const [state, setState, refresh] = useStorageState<number>('number', {
defaultValue: 0,
});What do you think? @Jinho1011 @raon0211 |
|
I also confirmed that this PR did not solve the issue. I apologize for my mistake. But I found that the method below widens the type, but what is your opinion on its appearance, even if it is not pretty? type ToPrimitive<T> = T extends string
? string
: T extends number
? number
: T extends boolean
? boolean
: T extends unknown[]
? unknown[]
: T extends Record<string, unknown>
? Record<string, unknown>
: T; |
defaultValue in useStorageState inferred as a literal type
This pull request updates the type definition of defaultValue in codebase to be inferred as a primitive type instead of a literal value.
To achieve this, I updated the
useStorageStatefunction's generic type parameter to extendsSerializableinstead of using defaultValue as aSerializable<T>.Pull Request about #220
PR Checklist