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

Set cache on queries preview results #1360

Merged
merged 27 commits into from
Dec 11, 2023

Conversation

annacv
Copy link
Contributor

@annacv annacv commented Nov 21, 2023

EMP-2974

Pull request template

We could face a memory leak if we use the Query Results Preview in the Empathize to show something as an Instant Search as we showed in the Sprint Review, in that case, we wouldn’t want to cache the results.

CHANGES APPLIED:

  • Add a cache prop and only cache the results accordingly (saveCache prop set to false by default, we will only catch the queries in BrandRecommendations to avoid requesting twice).

    • saveCache is set at true only with brand recommendations, because is the only feature where we want to save the queries.
  • Only trigger request if there are no cached results

    • When the queryPreview component is created we also check if the query is cached before triggering the request. If so, we emit load to render the queryPreview, otherwise, we emit QueryPreviewRequestUpdated.

PENDING TO RESOLVE: --> New task: Note that in the current implementation, we are only saving queries. As a second step, we should also support filters, but then it should be the hash of query+filters+any other property that we might need or think about another workaround.

PENDING TO RESOLVE: --> Maybe we should deal with this in a separated PR

  • Animation effect now it is linked to requests, DOM renders as soon as the response is received, by catching queries we lose that effect.
  • Adding an animation component doesn’t solve the issue, it causes max call stack if used as a default in prop, or the lists are not rendered if passed as a computed prop. If this worked we could use the StaggeredFadeAndSlide when the caché is activated and ul when not. Before adding extraParams & filters, it worked only as a computed as follows:
         public get animation(): Vue | string | VueClass<Vue> {
       return StaggeredFadeAndSlide;
     } ```
    
  • Adding a debounce when we emit load doesn’t solve the issue, then only one query preview is rendered and is kept in the loading state

Motivation and context

Now, We are requesting the Query Preview search requests every time that they are rendered, so, every time that the user deletes the query.

  • Dependencies. If any, specify:
  • Open issue. If applicable, link: EMP-2867

Type of change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that causes existing functionality to not work as expected)
  • Change requires a documentation update

What is the destination branch of this PR?

  • Main
  • Other. Specify:

How has this been tested?

TEST:

  • Open vue dev tools and check if brand recommendations are being saved in queryPreview state.
  • Search for battery (has semantics) and check if brand recommendations still keep saving and if semantic queries are added to queriesPreview list.
  • Clean the query in the search box and check the queriesPreview list. It should contain only the brand recommendations queries, semantic-queries have saveCache false, which means that these queries should be removed.

Tests performed according to testing guidelines:

Checklist:

  • My code follows the style guidelines of this project.
  • I have performed a self-review on my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.
  • I have added tests that prove my fix is effective or that my feature works.
  • New and existing unit tests pass locally with my changes.

@annacv annacv requested a review from a team as a code owner November 21, 2023 14:07
@annacv
Copy link
Contributor Author

annacv commented Nov 21, 2023

Tests should be fixed after the implementation is validated 👀

@annacv annacv force-pushed the feature/EMP-2867-Set-cache-query-preview branch from 3ff4473 to 4692d20 Compare November 27, 2023 15:59
Copy link
Contributor Author

@annacv annacv left a comment

Choose a reason for hiding this comment

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

🎉

@annacv annacv requested a review from herrardo December 6, 2023 10:37
@CachedaCodes CachedaCodes merged commit 2579ca4 into main Dec 11, 2023
4 checks passed
@CachedaCodes CachedaCodes deleted the feature/EMP-2867-Set-cache-query-preview branch December 11, 2023 10:53
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.

4 participants