-
Notifications
You must be signed in to change notification settings - Fork 386
Conversation
Deploy preview ready! Built with commit bd57502 |
7f40928
to
252d5dc
Compare
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.
Really nice guide! some smaller issues and a question
docgen/src/guide/Handling_cache.md
Outdated
|
||
```jsx | ||
import React, { Component } from 'react'; | ||
import { InstantSearch, SearchBox } from '../packages/react-instantsearch/dom'; |
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.
... from 'react-instantsearch/dom'
docgen/src/guide/Handling_cache.md
Outdated
|
||
```jsx | ||
import React, { Component } from 'react'; | ||
import { InstantSearch, SearchBox } from '../packages/react-instantsearch/dom'; |
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.
react-instantsearch/dom
docgen/src/guide/Handling_cache.md
Outdated
|
||
See the example in this [story](https://community.algolia.com/react-instantsearch/storybook/?selectedKind=RefreshCache&selectedStory=with%20a%20refresh%20button&full=0&down=1&left=1&panelRight=1&downPanel=storybooks%2Fstorybook-addon-knobs). | ||
|
||
## Refresh the cache periodically |
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 has a leading space
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.
Yep, that space is here on purpose to get the heading format (see other guides)
docgen/src/guide/Handling_cache.md
Outdated
} | ||
``` | ||
|
||
See the example in this [story](https://community.algolia.com/react-instantsearch/storybook/?selectedKind=RefreshCache&selectedStory=with%20setInterval&full=0&down=1&left=1&panelRight=1&downPanel=storybooks%2Fstorybook-addon-knobs). |
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.
Would be nice to have an example that explains
- you do some action
- index backend
waitTask
- send some event to the frontend (maybe web sockets)
- refresh
but it might be too long, because it has quite some setup with the sockets
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 you are right it would be nice, but it's a bit harder to summarise. We point users to the right direction, they have the basics for using the update cache feature. IMO, how to receive the event for trigger the refresh is an implementation detail.
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.
It might still be nice to mention that if they are waiting for an Algolia action, they'll need to use waitTask
and send some event, otherwise the refreshing will be too early
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.
Very well written guide, to the point. Congrats! Minor changes proposals, let us know
docgen/src/guide/Handling_cache.md
Outdated
|
||
While it is a very convenient feature, in some cases, it is useful to have the ability to clear the cache and make a new request to Algolia. For instance, when changes are made on some records on your index, you might want the frontend side of your application to be updated to reflect that change (in order to avoid displaying stale results retrieved from the cache). | ||
|
||
To do so, there is a prop on the `<InstantSearch>` component called `refresh` that, when set to true, clears the cache and triggers a new search. |
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.
proposal: true
instead of true
docgen/src/guide/Handling_cache.md
Outdated
|
||
To do so, there is a prop on the `<InstantSearch>` component called `refresh` that, when set to true, clears the cache and triggers a new search. | ||
|
||
There are two different approaches to handle the cache: |
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.
Proposal:
There are two different use cases where you would want to discard the cache:
- your application data is being updated by another process that you don't manage, for example a CRON job updates users inside Algolia. For this you might want to periodically refresh the cache of your app
- you application data is being updated by the users of your app directly (for example, in a dashboard). In this use case you would want to refresh the cache base on some application state like the last modification of the user
wdyt?
I put this to better explains the scenarios.
docgen/src/guide/Handling_cache.md
Outdated
} | ||
``` | ||
|
||
See the example in this [story](https://community.algolia.com/react-instantsearch/storybook/?selectedKind=RefreshCache&selectedStory=with%20a%20refresh%20button&full=0&down=1&left=1&panelRight=1&downPanel=storybooks%2Fstorybook-addon-knobs). |
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.
See a live example in storybook
docgen/src/guide/Handling_cache.md
Outdated
} | ||
``` | ||
|
||
See the example in this [story](https://community.algolia.com/react-instantsearch/storybook/?selectedKind=RefreshCache&selectedStory=with%20setInterval&full=0&down=1&left=1&panelRight=1&downPanel=storybooks%2Fstorybook-addon-knobs). |
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.
See a live example in storybook
docgen/src/guide/Handling_cache.md
Outdated
@@ -0,0 +1,113 @@ | |||
--- | |||
title: Handling cache |
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.
Maybe use the title Refresh cache
since I think a lots of people will search for this keyword.
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.
Refreshing the cache
also sounds nice to me
docgen/src/guide/Handling_cache.md
Outdated
refresh = () => { | ||
this.setState(prevState => ({ | ||
refresh: !prevState.refresh, | ||
})); |
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.
We can simplify the call. When we call refresh
we always want to set the refresh
to true
.
this.setState({ refresh: true });
docgen/src/guide/Handling_cache.md
Outdated
} | ||
``` | ||
|
||
See the example in this [story](https://community.algolia.com/react-instantsearch/storybook/?selectedKind=RefreshCache&selectedStory=with%20setInterval&full=0&down=1&left=1&panelRight=1&downPanel=storybooks%2Fstorybook-addon-knobs). |
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 you are right it would be nice, but it's a bit harder to summarise. We point users to the right direction, they have the basics for using the update cache feature. IMO, how to receive the event for trigger the refresh is an implementation detail.
docgen/src/guide/Handling_cache.md
Outdated
setInterval( | ||
() => | ||
this.setState({ refresh: true }, () => | ||
this.setState({ refresh: false }) |
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.
It can be confusing for the user to use two different approach for reset the refresh
property.
stories/RefreshCache.stories.js
Outdated
count: prevState.count + 1, | ||
})), | ||
1000 | ||
); |
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.
We don't need to set two intervals, only one should be sufficient. Set an interval of 1000 and reset it when it reach the desired value. Then you can get rid of this component I think. Try to also use the same reset strategy as explained above.
stories/RefreshCache.stories.js
Outdated
}; | ||
} | ||
|
||
componentDidMount = () => { |
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.
Don't need to use a class property, the regular method should work componentDidMount() { ... }
61ba049
to
e5e3b1f
Compare
docgen/src/guide/Refreshing_cache.md
Outdated
} | ||
|
||
refresh = () => { | ||
this.setState({refresh: true}); |
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.
Be careful with the code style. Prettier seems to nut run on the markdown in your editor. But you can still copy / paste the snippet inside the Prettier playground. Should be:
({ refresh: true })
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.
You should be able to put the whole markdown in the playground, but we probably should have a lint-staged command that runs prettier on all staged files, WDYT?
docgen/src/guide/Refreshing_cache.md
Outdated
> | ||
Refresh cache | ||
</button> | ||
<CustomHits /> |
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 variable doesn't exist in the snippet, use Hits
instead of CustomHits
. Don't forget to add the import.
docgen/src/guide/Refreshing_cache.md
Outdated
}; | ||
} | ||
|
||
componentDidMount = () => { |
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.
Prefer the usage of method instead of class property.
componentDidMount() { ... }
docgen/src/guide/Refreshing_cache.md
Outdated
|
||
onSearchStateChange = () => { | ||
this.setState({ refresh: false }); | ||
}; |
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.
Add a space after this declaration.
docgen/src/guide/Refreshing_cache.md
Outdated
onSearchStateChange={this.onSearchStateChange} | ||
> | ||
<SearchBox /> | ||
<CustomHits /> |
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 variable doesn't exist in the snippet, use Hits
instead of CustomHits
. Don't forget to add the import.
docgen/src/guide/Refreshing_cache.md
Outdated
This method will ensure that the cache is cleared on a regular basis. | ||
You should use this approach if you cannot use a user action as a specific event to trigger the clearing of the cache. | ||
|
||
```jsx |
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.
Same comment as above, use Prettier for the all the snippet.
51c0aad
to
9e619a3
Compare
Summary
In the wake of the refresh cache feature (#619), this is a guide on how to refresh the cache with the description of the use case and an explanation of the two different approaches that can be used.