fix(status): send render event when loading starts #5127
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Without this it's impossible to use `loading` for anything. I opted to only emit the event, as calling render on the widgets would require changes in all the tests. If we render (fully), you still don't get updated of `loading` initially because `render` only gets called when there are results: https://github.com/algolia/instantsearch.js/blob/4d43bd8292446101742a10d1da9f042f780d8d45/src/widgets/index/index.ts#L573-L575 To fix all of these inconsistencies, we should join `init` and `render` in a single function (always calling render, even if it's the first render, also when there's no results), but this would be a significant breaking change, so shouldn't be done lightly
|
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 9c4c8e6:
|
Haroenv
added a commit
to algolia/react-instantsearch
that referenced
this pull request
Sep 30, 2022
This was referenced Sep 30, 2022
Merged
sarahdayan
reviewed
Oct 3, 2022
Comment on lines
+1299
to
+1301
| expect(renderSpy).toHaveBeenCalledTimes(2); | ||
| expect(renderSpy).toHaveBeenNthCalledWith(1, { widgetRenderTimes: 0 }); | ||
| expect(renderSpy).toHaveBeenNthCalledWith(2, { widgetRenderTimes: 1 }); |
There was a problem hiding this comment.
This isn't obvious what the behavior is when just reading those lines, can you add comments?
sarahdayan
reviewed
Oct 3, 2022
| @@ -185,7 +185,7 @@ describe('status', () => { | |||
| }); | |||
|
|
|||
| test('lets users render on error with the `render` event', async () => { | |||
| expect.assertions(4); | |||
| // expect.assertions(4); | |||
sarahdayan
approved these changes
Oct 3, 2022
Haroenv
added a commit
to algolia/react-instantsearch
that referenced
this pull request
Oct 3, 2022
<!-- Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. --> **Summary** <!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? Are there any linked issues? --> Introduces `status` and `error` in the response of `useInstantSearch`, as well as a new option on `useInstantSearch` to indicate errors in the search lifecycle should be caught. built on top of algolia/instantsearch#5127 **Result** <!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes UI. --> ```jsx function Status() { const { status, error } = useInstantSearch({ catchError: true }); return ( <> <span>Search status: {status}</span> {error && <span>a search error occurred: {error.message}</span>} </> ); } ``` FX-1769
Haroenv
added a commit
that referenced
this pull request
Jan 4, 2023
…earch#3645) <!-- Thanks for submitting a pull request! Please provide enough information so that others can review your pull request. --> **Summary** <!-- Explain the **motivation** for making this change. What existing problem does the pull request solve? Are there any linked issues? --> Introduces `status` and `error` in the response of `useInstantSearch`, as well as a new option on `useInstantSearch` to indicate errors in the search lifecycle should be caught. built on top of #5127 **Result** <!-- Demonstrate the code is solid. Example: The exact commands you ran and their output, screenshots / videos if the pull request changes UI. --> ```jsx function Status() { const { status, error } = useInstantSearch({ catchError: true }); return ( <> <span>Search status: {status}</span> {error && <span>a search error occurred: {error.message}</span>} </> ); } ``` FX-1769
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Without this it's impossible to use
loadingfor anything. I opted to only emit the event, as calling render on the widgets would require changes in all the tests.If we render (fully), you still don't get updated of
loadinginitially becauserenderonly gets called when there are results:https://github.com/algolia/instantsearch.js/blob/4d43bd8292446101742a10d1da9f042f780d8d45/src/widgets/index/index.ts#L573-L575
To fix all of these inconsistencies, we should join
initandrenderin a single function (always calling render, even if it's the first render, also when there's no results), but this would be a significant breaking change, so shouldn't be done lightly.Without this PR,
useInstantSearchin React InstantSearch Hooks doesn't get updated when the status changes toloading, as that doesn't emit render (yet)