keep results on search failure #1747
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
tmeyer2115
reviewed
May 16, 2022
oshi97
reviewed
May 16, 2022
src/core/core.js
Outdated
| @@ -323,9 +323,6 @@ export default class Core { | |||
| window.performance.mark('yext.answers.verticalQueryResponseRendered'); | |||
| }).catch(error => { | |||
There was a problem hiding this comment.
Would it make sense to move this catch immediately after coreLibrary.verticalSearch is called, so that it only catches errors thrown by answers-core? We could then have another catch here at the bottom for all other types of errors
There was a problem hiding this comment.
yea that works, I will update
There was a problem hiding this comment.
per discussion with oshi offline, the throw error should already be clear on where the issue is from. Adding additional catch statements here would require handling breaking the then/catch chain mid way or add logic to avoid bleeding into the later catch statements (potentially end up with duplicate update loading state calls and console errors)
tmeyer2115
reviewed
May 16, 2022
tmeyer2115
reviewed
May 16, 2022
tmeyer2115
approved these changes
May 17, 2022
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.
Rendering failures leading to the catch statements in Universal and Vertical search would clear all results. User would still expects other results from the response to render on page. This pr updates the
storage.setcalls to only update the searchState, instead of storing a new empty results, so the non-problematic result sets can still be render.The catch statements in core.js will terminate any further rendering of sub components once it encounter a problematic sub component. User expects sdk to render as much as it can. To ensure non-problematic sub-components can still render, this pr added more catch statements in component.js to gracefully handle errors coming from individual child component's data transformation, initialization and mount stage.
J=none
TEST=manual
use
dev-keep-data-on-search-failurefor SDK version (from this test branch) in the config file of the site with the issue. Searched 'Listings', and see that the same error appeared in console but the other cards still render as normal.use
dev-dont-clear-data-on-failureto test changes in this pr through local html pages that use SDK directly. See that errors are logged but the loading icon is reset to complete during error handling process