fix(context): revert removing overloads in UseContextStore
#817
Conversation
|
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 52cc3f4:
|
tests/context.test.tsx
Outdated
| const { useStore } = createContext<CounterState>() | ||
| function _Counter() { | ||
| let x = useStore(useCallback((state) => state.count, [])) | ||
| expectAreTypesEqual<typeof x, number>().toBe(true) |
There was a problem hiding this comment.
We have to use this because writing x: number = useStore(..) won't test the inference as even when the inference fails x will be inferred as any and it'll still compile.
There was a problem hiding this comment.
Interesting. Nice to learn a new technique.
Just wondered why it was inferred as any instead of unknown before this PR...
There was a problem hiding this comment.
Just wondered why it was inferred as any instead of unknown before this PR...
Oops sorry I missed this. It's because in useCallback's callback's constraint is (...a: any[]) => any as you can see here1. Hence here you will see x is any in useFoov37. But the correct constraint is for an unknown function is (...a: never[]) => unknown, in that case the code wouldn't even compile. (I hope this is what you were asking)
Footnotes
-
They have changed it to
Functionin react 18 but that still is not correct. ↩
|
Can we get this merged and published? |
|
I have one or two more PRs to work on to make a release. Meanwhile, you can keep using the codesandbox build. |
| export type UseContextStore<T extends State> = { | ||
| (): T | ||
| <U>(selector: StateSelector<T, U>, equalityFn?: EqualityChecker<U>): U | ||
| } |
There was a problem hiding this comment.
@dai-shi Shouldn't this have been the following?
export type UseContextStore<T extends State> = {
(): T
// note the "?" after selector
<U>(selector?: StateSelector<T, U>, equalityFn?: EqualityChecker<U>): U
}As useContextStore(undefined, myEquals) is also valid.
Should I correct it in #725 or make a new PR?
There was a problem hiding this comment.
That causes the DX issue in #812.
We don't recommend/allow passing undefined for selector as a practice. So, think it like TS types are correct, and JS impl is just naive. (We could use arguments, but it's not worth the size&complexity.)
btw, this is the similar discussion for combine in #725 (comment). TS types are correct for usage, so no need to be too precise about typing JS impl (if it's not the recommended usage. Could be done with defineProperty, but...)
There was a problem hiding this comment.
That's trivial to fix, we can just add another overload — https://tsplay.dev/wggQbw — we already have 2 overloads so adding another doesn't make a difference.
I think not supporting undefined selector is an unnecessary compromise, what if users want to actually use custom equality function without selector then they'd have to do useStore(x => x, eq) which looks less expressive and less intuitive than useStore(undefined, eq).
So I insist that we add another overload, I'm doing it in #725, if you don't like it I'll revert.
And in case of combine we're being intentionally unprecise with types to make the inference work, just that I think it's important to inform the users about the unsoundness we have produced, even if it is negligible. (Also shallow merge is good, defineProperty would make it more confusing and won't even solve the problem.)
There was a problem hiding this comment.
I don't like useStore(undefined, eq), and it's almost always a mistake in the usage. But, having another overload is acceptable for an edge case. Will look at #725 and comment there.
Fixes #812