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

fix(routing): ensure UiState is cleaned up allowing "undefined" for values #5956

Merged
merged 5 commits into from
Dec 7, 2023

Conversation

Haroenv
Copy link
Contributor

@Haroenv Haroenv commented Dec 7, 2023

Summary

Since #5912, we ensure to clean the UiState from empty values, however this was done in a slightly inefficient (looping over values) and unsafe (no checking for undefined) way, as the types implied that RouteState/UiState always is completely given (if you're using TypeScript that's actually the case, you'd get an error returning undefined for refinementList[attribute] for example).

Result

fixes #5954

@Haroenv Haroenv changed the title test(routing): enforce compatibility with "unsafe" uiState fix(routing): ensure UiState is cleaned up allowing "undefined" for values Dec 7, 2023
Copy link

codesandbox-ci bot commented Dec 7, 2023

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 ab708dd:

Sandbox Source
InstantSearch.js Configuration
react-instantsearch-app Configuration
example-react-instantsearch-default-theme Configuration
example-react-instantsearch-next-app-dir-example Configuration
example-vue-instantsearch-default-theme Configuration

@Haroenv Haroenv marked this pull request as ready for review December 7, 2023 10:27
@Haroenv Haroenv requested review from dhayab, a team and sarahdayan and removed request for a team December 7, 2023 10:27
Copy link
Member

@sarahdayan sarahdayan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks correct to me.

As mentioned previously, removeEmptyRefinementsFromUiState looks factorizable, we could have a single util instead of the same function everywhere. I'll let you be the judge of whether we should do it now or keep it for later.

@Haroenv
Copy link
Contributor Author

Haroenv commented Dec 7, 2023

The types make it too hard to make factorable, as well as the conditions not being exactly the same anymore. I tried it but will focus again after this is merged/released

@Haroenv Haroenv enabled auto-merge (squash) December 7, 2023 11:00
Copy link
Member

@dhayab dhayab left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙇

@Haroenv Haroenv merged commit 18cfece into master Dec 7, 2023
@Haroenv Haroenv deleted the fix/refinement-clean-safe branch December 7, 2023 11:05
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 this pull request may close these issues.

Statemapping not working with instantsearch.js 4.61.0
3 participants