Skip to content

Add cache disabling for datasets#254

Merged
Haroenv merged 5 commits intoalgolia:masterfrom
cdtinney:dataset-disable-cache
Oct 12, 2018
Merged

Add cache disabling for datasets#254
Haroenv merged 5 commits intoalgolia:masterfrom
cdtinney:dataset-disable-cache

Conversation

@cdtinney
Copy link
Copy Markdown
Contributor

@cdtinney cdtinney commented Oct 6, 2018

Summary

Adds the ability to disable cache retrieval of subsequent identical queries via a boolean flag in dataset config called disableCache.

Use case:

  • Query suggestions may differ between queries if the data on the server changes, and it is critical to display up-to-date suggestions

Related issues:

Additional notes:

  • Fixed typo in test case name

@coveralls
Copy link
Copy Markdown

coveralls commented Oct 6, 2018

Coverage Status

Coverage increased (+0.01%) to 88.854% when pulling a67320b on cdtinney:dataset-disable-cache into d343bee on algolia:master.

@Haroenv
Copy link
Copy Markdown
Contributor

Haroenv commented Oct 8, 2018

This one definitely sounds like the more reasonable of both PRs, @samouss, what do you think?

Comment thread src/autocomplete/dataset.js Outdated

this.debounce = o.debounce;

this.disableCache = !!o.disableCache;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would go for enableCache (or something similar) with a default value to true rather than disableCache. Positive condition are a bit easier to reason about.

Comment thread src/autocomplete/dataset.js Outdated
shouldFetchFromCache: function shouldFetchFromCache(query) {
if (this.disableCache) {
return false;
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Once we have inverted the condition we can inline the condition.

@samouss
Copy link
Copy Markdown
Contributor

samouss commented Oct 11, 2018

@Haroenv The tests are failing because of the Netlify ENV variable not available on contributors PR.

Copy link
Copy Markdown
Contributor

@samouss samouss left a comment

Choose a reason for hiding this comment

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

Thanks for the changes! For the option name we can go for cache rather than enableCache no? WDYT? cc @Haroenv

@Haroenv
Copy link
Copy Markdown
Contributor

Haroenv commented Oct 11, 2018

Agreed with @samouss, cache: true seems like a good name!

@Haroenv Haroenv merged commit 0e65fee into algolia:master Oct 12, 2018
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