-
Notifications
You must be signed in to change notification settings - Fork 2.8k
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
feat(ui): Add caching to search, entity profile for better UX #9362
feat(ui): Add caching to search, entity profile for better UX #9362
Conversation
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 so exciting. the app is gonna feel so nice after this
@@ -88,6 +91,7 @@ export default function BasicFilters({ | |||
<span id={SEARCH_RESULTS_FILTERS_ID}> | |||
<FlexSpacer> | |||
<FlexWrapper> | |||
{loading && !visibleFilters?.length && <BasicFiltersLoadingSection />} |
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.
nice
@@ -188,6 +188,7 @@ export const EmbeddedListSearch = ({ | |||
variables: { | |||
input: searchInput, | |||
}, | |||
fetchPolicy: 'cache-first', |
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.
oh man what a time to be alive - these possibleTypes making search caching possible!
@@ -32,6 +32,7 @@ export default function useGetDataForProfile<T>({ urn, entityType, useEntityQuer | |||
refetch, | |||
} = useEntityQuery({ | |||
variables: { urn }, | |||
fetchPolicy: 'cache-first', |
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.
and this won't prevent showing updates when we add tags, terms, domains, documentation etc?
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.
testing locally things look alright to me... but never hurts to double check
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.
thats right -- generally we issue a refetch on changes and that will always trigger a network call!
@@ -62,6 +62,7 @@ export const SearchPage = () => { | |||
searchFlags: { getSuggestions: true }, | |||
}, | |||
}, | |||
fetchPolicy: 'cache-and-network', |
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.
sweet so this will show cached results first and then once the network request returns it will update that cache and show updated results?
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.
yes! So if you change tags, owners, etc and return to search page we will indeed get the update for the user
unrelated tests failing.. merging! |
Summary
Checklist