-
Notifications
You must be signed in to change notification settings - Fork 386
fix(MultiIndex): ensure getResults return only hits matching index in the context #136
Conversation
Deploy preview ready! Built with commit 86fab70 |
@@ -41,7 +42,7 @@ describe('connectInfiniteHits', () => { | |||
const hits2 = [{}, {}, {}, {}, {}, {}]; | |||
const hits3 = [{}, {}, {}, {}, {}, {}, {}, {}]; | |||
const res1 = getProvidedProps(null, null, { | |||
results: { hits, page: 0, hitsPerPage: 6, nbPages: 10 }, | |||
results: { hits, page: 0, hitsPerPage: 6, nbPages: 10, index: 'index' }, |
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.
Should we use "indexName" like other parts of the lib? I would say any moment we speak about an index name we should use the "indexName" token as we do in every JS lib.
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.
index
is the name used on the SearchResults
object from the helper.
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.
Does it bubbles up to the React InstantSearch user API?
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.
If you're using the createConnector
API yes, because you access searchResults
otherwise it shouldn't.
Do you have a small example usage of when this happened? On a side note, this is not only a fix but also a new feature where the index name is always passed down to the results (multi index or not) right? |
@vvo Not sure to get what you mean but this bug is happening when switching from one indices to multiple indices. For example we saw it on an example showcasing "tabs". You have one tab that is all, and then one tab per index. There is actually a better way to handle this (by namespacing every index, even when only one), but it would imply a breaking change. Initially we were using it, you can see this PR when I revert the breaking change. 44f7de0 |
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.
LGTM, that .index prop was already here when using createConnector
<a name="4.0.5"></a> ## [4.0.5](v4.0.4...v4.0.5) (2017-06-26) ### Bug Fixes * **MultiIndex:** ensure getResults return only hits matching index in the context (#136) ([124ffe6](124ffe6)) * **MultiIndex:** handle if namespace isn't in search state (#139) ([1aab324](1aab324)) * **storybook:** process CSS through autoprefixer (#138) ([62cf512](62cf512))
No description provided.