-
Notifications
You must be signed in to change notification settings - Fork 386
feat(refreshcache): add prop refresh to InstantSearch instance #619
Conversation
bccfd09
to
49987b4
Compare
Deploy preview ready! Built with commit 9b43b48 |
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.
Super nice feature, code wise nothing to remark. Some smaller stylistic questions/suggestions though
refresh: true, | ||
}); | ||
|
||
expect(ism.clearCache.mock.calls).toHaveLength(1); |
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.
toHaveBeenCalledTimes(1)
refresh: false, | ||
}); | ||
|
||
expect(ism.clearCache.mock.calls).toHaveLength(0); |
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.
.not. toHaveBeenCalled()
</InstantSearch> | ||
); | ||
|
||
expect(ism.clearCache.mock.calls).toHaveLength(0); |
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.
.not. toHaveBeenCalled()
</InstantSearch> | ||
); | ||
|
||
expect(ism.updateIndex.mock.calls).toHaveLength(0); |
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.
.not. toHaveBeenCalled()
indexName: 'foobar', | ||
}); | ||
|
||
expect(ism.updateIndex.mock.calls).toHaveLength(0); |
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.
.not.toHaveBeenCalled()
indexName: 'newindexname', | ||
}); | ||
|
||
expect(ism.updateIndex.mock.calls).toHaveLength(1); |
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.
toHaveBeenCalledTimes(1)
stories/RefreshCache.stories.js
Outdated
<button onClick={this.refresh} style={buttonStyle}> | ||
Refresh cache | ||
</button> | ||
<button style={divStyle} disabled> |
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.
Why a button?
} | ||
} | ||
|
||
stories.add('with a refresh button', () => <AppWithRefresh />); |
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.
Can you add the minimal jsx:
<InstantSearch
appId="latency"
apiKey="6be0576ff61c053d5f9a3225e2a90f76"
indexName="ikea"
refresh={this.state.refresh}
onSearchStateChange={() => this.setState({ refresh: false })}
>
<SearchBox />
<button
onClick={() => this.setState(({ refresh }) => ({ refresh: !refresh }))}
>
Refresh cache
</button>
<p>
Refresh is set to: <em>{displayRefresh}</em>
</p>
<Hits />
</InstantSearch>
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 story contains a customHit
component, that's why I didn't add the JSX story (see thread on the PR to add the JSX addon)
import { InstantSearch, SearchBox } from '../packages/react-instantsearch/dom'; | ||
import { CustomHits } from './util'; | ||
|
||
const stories = storiesOf('RefreshCache', module); |
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.
is that normal syntax? What is module
?
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, normal syntax, see https://storybook.js.org/basics/guide-react/#create-the-config-file (then section about writing stories)
Awesome @marielaures ❤️ . You should add a guide describing how to use this feature :) |
49987b4
to
72d865c
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.
Great work @marielaures! 🎉
Just few comments.
@@ -170,6 +177,8 @@ InstantSearch.propTypes = { | |||
|
|||
createURL: PropTypes.func, | |||
|
|||
refresh: PropTypes.bool, |
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 can put this one required, since createInstantSearch
will always provide the prop.
@@ -24,11 +24,16 @@ export default function createInstantSearch(defaultAlgoliaClient, root) { | |||
searchParameters: PropTypes.object, | |||
createURL: PropTypes.func, | |||
searchState: PropTypes.object, | |||
refresh: PropTypes.bool, |
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 can put this one required, because we will enforce the fact that the value will be true|false
instead of true|false|undefined|null
.
|
||
createInstantSearchManager.mockImplementation(() => ism); | ||
|
||
const wrapper = mount( |
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 can use shallow
instead of mount
. The former will only create a tree for the root level (much better for more focused unit test), the latter will perform a full rendering of the DOM. In you test you don't need to perform the full rendering.
|
||
createInstantSearchManager.mockImplementation(() => ism); | ||
|
||
const wrapper = mount( |
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 as before, prefer shallow
over mount
.
stories/RefreshCache.stories.js
Outdated
this.state = { | ||
refresh: false, | ||
}; | ||
this.refresh = this.refresh.bind(this); |
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 can use the function property instead of bind
, like you do with onSearchStateChange
.
stories/RefreshCache.stories.js
Outdated
background: '#3369e7', | ||
}; | ||
|
||
const divStyle = { |
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 can use inline styles like the rest of the example. Just for coherence 🙂
This looks very good and professional, GG @marielaures. Small comments to address from peers and that's it! |
3060215
to
1f6cdaa
Compare
hello, sorry is it was anounced somewhere else; is this feature already available in the main package ? usable via |
@Haroenv has right, it's not published yet. I will try to publish a beta tomorrow, if I have not time to do it ti will be next week! |
@Haroenv I'll give it a try and build it. Correct me if i'm wrong, as of now it will be the only way to "trigger" a refresh of the displayed list, right ? |
Yes, that’s the only way. It’s possible to clear the cache as described in the linked issue, but refreshing the results needs a setup like this @deesx |
<a name="4.3.0-beta.0"></a> # [4.3.0-beta.0](v4.2.0...v4.3.0-beta.0) (2017-11-27) ### Bug Fixes * **babelrc:** add a key for each env development, production, es ([#547](#547)) ([fa9528d](fa9528d)) * **localizecount:** allow localized string for count in MenuSelect ([#657](#657)) ([67ebd34](67ebd34)) * **react-router-example:** Properly update search query when using browser navigation ([#604](#604)) ([9ee6600](9ee6600)) ### Features * **refreshcache:** add prop refresh to InstantSearch instance ([#619](#619)) ([19f6de0](19f6de0))
I do not understand, has this PR ever made into official react-intantsearch package??? |
yes @sakhmedbayev, this is supported |
@Haroenv thanks for the reply. In what version it is supported? v4 or v5? |
Also, I cannot find anywhere docs for this feature except in PR's commits. |
Hi @sakhmedbayev, you can find more information about this props in our documentation. We also have a guide explaining common use case that this feature solve. Here is the link of the guide. The feature is available from the version 4.3.0. |
Is this working on React Native, specifically? Can't seem to get it working locally. |
The logic is not tied to any platform so it should work for React Native. If the issue persist please provide us a working example, it will help us to better understand the issue. |
Summary
The goal of this PR is to make it possible to refresh an InstantSearch instance.
It is allows the user to make a new request to Algolia in case a change has happened on the index for instance instead of getting the results from the cache.
Fixes #252.
Result
Adding a prop
refresh
to theInstantSearch
component will trigger the refresh.It takes a Boolean as an argument.
You can see how it works on the refreshCache story in Storybook.