fix(createInstantSearchManager): drop outdated response #1765
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
I like this change, this will simplify the code for the search for facet values feature when calling the helper. |
bobylito
force-pushed
the
fix/request-response-order
branch
from
72a6b68 to
136f327
Compare
|
/o\ OMG the tests are behaving differently on CI :( |
bobylito
force-pushed
the
fix/request-response-order
branch
from
136f327 to
fb4689d
Compare
|
Test are passing!! 🎉 |
mthuret
reviewed
Dec 21, 2016
| }); | ||
|
|
||
| describe('createInstantSearchManager', () => { | ||
| describe('with correct result from algolia', () => { |
There was a problem hiding this comment.
wasn't supposed to be "with error" here?
There was a problem hiding this comment.
Yep, copy pasting too quick :/ Thanks :)
mthuret
reviewed
Dec 21, 2016
| console.log('setup'); | ||
| const Helper = require.requireActual('algoliasearch-helper/src/algoliasearch.helper.js'); | ||
| Helper.prototype._handleResponse = function(state) { | ||
| this.emit('error', {count: count++}, state); |
There was a problem hiding this comment.
To avoid having several files, maybe something you could do is emitting a different event depending of the query present in the state parameter?
bobylito
force-pushed
the
fix/request-response-order
branch
from
fb4689d to
ee30547
Compare
|
Since I am not deeply into this code, could you detail a bit more:
That would help a bit grasp the PR context, thanks! |
vvo
reviewed
Dec 21, 2016
| algoliaClient, | ||
| searchParameters = {}, | ||
| }) { | ||
| const helper = algoliasearchHelper(algoliaClient); | ||
| if (!algoliaClient || !indexName) throw new Error('indexName and algoliaClient are mandatory'); |
There was a problem hiding this comment.
Can you move this to another PR? Also the error message is not exactly right
|
I updated the description @vvo Let me know if you need more details. |
The helper is implemented in a way that it will drop outdated requests. This change makes react-instantsearch use the event model of the helper that let it implement this mechanism that ensures reliability in the algolia answers. It also results in some fewer instanciation of objects for callbacks and the base search parameters.
bobylito
force-pushed
the
fix/request-response-order
branch
from
ee30547 to
8d9aac5
Compare
5 tasks
vvo
pushed a commit
that referenced
this pull request
Jan 4, 2017
<a name="2.1.0"></a> # [2.1.0](v2.0.1...v2.1.0) (2017-01-04) ### Bug Fixes * **createInstantSearchManager:** drop outdated response (#1765) ([76c5312](76c5312)) * **highlight:** highlight should work even if the attribute is missing (#1791) ([5b79b15](5b79b15)), closes [#1790](#1790) * **InfiniteHits:** better classname to loadmore btn (#1789) ([ad2ded3](ad2ded3)) * **starRatings:** click on selected range doesn't unselect it (#1766) ([beacc72](beacc72)) * **website:** broken demo links (#1802) ([0abe2f5](0abe2f5)) * **widgets:** add 300px width for the default SearchBox (#1803) ([bf5d791](bf5d791)) ### Features * **InfiniteHits:** Add class to load more button (#1787) ([416febd](416febd)) * **RefinementList, connectRefinementList:** allow to search for facet values ([e086a81](e086a81))
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is for React-InstantSearch
The event model for the helper is implemented in a way that it will drop outdated requests.
This change makes react-instantsearch use the event model. This mechanism ensures reliability in the algolia answers.
It also results in some fewer instanciations of objects for callbacks and the base search parameters.
Details of what was done
The previous implementation was based on
searchOnce. This method does not protect you from responses that come back unordered because it is based on the cb / promise from the algolia client which will always call the callback even if they are unordered.NOW
Use the helper properly with events and its protection against this kind of behaviour.
**The tests are now up to date 🍾 **
A note about the test 🤔
The tests were based on mocks that were making a lot of assumptions about the internals. That's why they were all broken after my changes. I tried to redo them by making less assumptions about the internals, and use the external API as much as possible.
That being said, this refactoring is now based on the event API of the helper, which is very hard to manipulate without some heavy mocking. Writing those tests I also learned a bit about the jest magic concerning the mocks and their limitations. For example you can't change the implementation on the fly (calling again jest.mock doesn't do anything) nor can you reference a closed variable. That's why I ended up with three test files, instead of one (one without mocks and two with different behaviours for the mocks)
If you have any other solution about how to make those tests I would be very grateful 🙇 Thanks 🤓