-
Notifications
You must be signed in to change notification settings - Fork 515
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(types): make all usages of UiState in InstantSearch generic #5060
Conversation
This allows someone with custom widgets to still use setUiState etc. without type errors
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 8d01a07:
|
@@ -2038,7 +2038,7 @@ See documentation: https://www.algolia.com/doc/api-reference/widgets/instantsear | |||
|
|||
test('with function form sets indices state', async () => { | |||
const searchClient = createSearchClient(); | |||
const search = new InstantSearch({ | |||
const search = new InstantSearch<UiState>({ |
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.
This is needed because initialUiState seems to "fill in" the generic. That makes sense, but in this case it makes things required that aren't actually required. Not sure how to solve that though! This already was the case before the PR, but became visible due to usage of TUiState in setUiState
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.
#5061 fixes this, but I'll merge this as two PRs
if you pass initialUiState, that should be asserted by the generic UiState, not automatically accepted. Otherwise you get use cases like the "with function form sets indices state" test where setUiState thinks query is required, just because it has the default given The way this works is by using `NoInfer` from microsoft/TypeScript#14829 (comment) which seems to be quite relied upon This is a follow-up on #5060
* fix(types): allow useInstantSearch to be generic * avoid algolia/instantsearch#5060
* fix(types): make all usages of UiState in InstantSearch generic This allows someone with custom widgets to still use setUiState etc. without type errors * fix lint * fix(types): avoid inferring UiState type from initialUiState if you pass initialUiState, that should be asserted by the generic UiState, not automatically accepted. Otherwise you get use cases like the "with function form sets indices state" test where setUiState thinks query is required, just because it has the default given The way this works is by using `NoInfer` from microsoft/TypeScript#14829 (comment) which seems to be quite relied upon This is a follow-up on #5060
Summary
This allows someone with custom widgets to still use setUiState etc. without type errors
Result
all places
UiState
is used in the InstantSearch instance are now generic and use TUiState.There are still places left in individual widgets, middleware which aren't generic, but that can be solved in a different PR
A sandbox showing the fixed problem: https://codesandbox.io/s/instantsearch-js-forked-m8nm7s?file=/src/app.ts:1152-1262