-
Notifications
You must be signed in to change notification settings - Fork 516
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(search-client): Add support for Custom Search Clients #2894
Conversation
Deploy preview for algolia-instantsearch ready! Built with commit fa0ca7f https://deploy-preview-2894--algolia-instantsearch.netlify.com |
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.
I like this feature!
src/lib/InstantSearch.js
Outdated
const client = createAlgoliaClient(algoliasearch, appId, apiKey); | ||
client.addAlgoliaAgent(`instantsearch.js ${version}`); | ||
// eslint-disable-next-line prefer-rest-params | ||
const [THROWAWAY_PARAMS] = arguments; |
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 line is a bit odd, what about giving the args to this function this name, and destructure after this line?
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 would indeed be cleaner. I wanted to change the fewest possible number of lines since this is meant to be thrown away soon.
Is it still worth it?
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 leave the code as-is but add a comment that this is the first argument, which will be removed? Not totally sure what to do here
search.start(); | ||
|
||
expect(search.helper.state.query).toBe(''); | ||
expect(searchClientSpy.search).toHaveBeenCalledTimes(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.
also useful to assert with what it's been called?
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.
For your information this test was taken from: https://github.com/algolia/instantsearch.js/blob/develop/src/lib/__tests__/InstantSearch-test-2.js#L11
It's called with this:
[
{
"indexName": "",
"params": { "facets": [], "page": 0, "query": "test", "tagFilters": "" }
}
]
Is it worth creating a snapshot for 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.
I'd go with a regular test here rather than a snapshot one, but not sure if it needs to be tested if already asserted enough in other spots
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.
Good call on testing the search() parameters here 👍 Considering the black box nature of the parameters, I'd go for a snapshot. Either would work (it's just more maintainance to not use snapshots)
it('and nothing else', () => { | ||
expect(() => { | ||
new InstantSearch({ | ||
searchClient: {}, |
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.
Miss indexName
}).toThrow(); | ||
}); | ||
|
||
it('and indexname, appId, apiKey', () => { |
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 not necessary.
search.start(); | ||
|
||
expect(search.helper.state.query).toBe(''); | ||
expect(searchClientSpy.search).toHaveBeenCalledTimes(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.
Good call on testing the search() parameters here 👍 Considering the black box nature of the parameters, I'd go for a snapshot. Either would work (it's just more maintainance to not use snapshots)
search.start(); | ||
|
||
expect(searchFunctionSpy).toHaveBeenCalledTimes(1); | ||
expect(search.helper.state.query).toBe('test'); |
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.
I'd use the same strategy as for the previous test
src/lib/InstantSearch.js
Outdated
} | ||
|
||
const client = { | ||
__proto__: Object.getPrototypeOf(searchClient), |
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.
I don't think that this is standardised properly.
What about doing instead:
if (typeof searchClient.clearCache === 'function') {
searchClient.clearCache = function() {};
}
if (typeof searchClient.addAlgoliaAgent === 'function') {
searchClient.addAlgoliaAgent = function() {};
}
or doing this check in the places in InstantSearch those two functions are used?
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.
or better:
const client = Object.create(person);
client.addAlgoliaAgent = client.addAlgoliaAgent || function() {};
client.clearCache = client.clearCache || function() {};
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: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/Object/proto for more info on why __proto__
probably shouldn't be used. Object.create
seems to be supported on all browsers we support 😄
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.
I like your second solution, thanks! I needed __proto__
because algoliasearch
relies on it. We come back to the initial solution but with Object.create()
.
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.
I like your second solution, thanks! I needed proto because algoliasearch relies on it.
Quick question, since this is a custom search client we have here, why do we need to have __proto or Object.create(). Which part of the code will expect this object to be made this way?
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.
What if you just tested if methods you need are existing and if that's the case then do not even care about creating a new object (most probably this is already a good client, ours). And only create a new object if those methods are missing
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, my preferred option would be to check if addAlgoliaAgent
exists when we are using it, rather than this workaround of making it always callable
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 could have done this:
const client = searchClient;
But this kind of assignation is a reference in JavaScript. That means that if ever we do client.hello = true
, it will mutate searchClient
. I think we shouldn't risk it. Object.create()
creates a deep clone of the object.
We could also have done this:
const client = { ...searchClient };
But this doesn't copy the prototype, on which algoliasearch
relies (it is not a simple object). We'd lose the reference to client.search()
(becoming undefined
).
Therefore, I think that Object.create()
is the most appropriate thing to do anyway.
Does that answer your question? Do you agree with this decision?
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.
Do you agree with this decision?
Yes and thanks for the explanations, it might be good to add a small comment saying "We don't want to use the same reference so we clone the client no matter if it already have the necessary methods"
src/lib/InstantSearch.js
Outdated
if (options.createAlgoliaClient) { | ||
// eslint-disable-next-line no-console | ||
console.warn( | ||
'InstantSearch.js: `createAlgoliaClient` option is deprecated and will be removed in the next major version. Please use `searchClient` instead.' |
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 a link to the documentation that explains how to do this switch? Always good to have more than just an error message, but an actual actionable link
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, I'm working on a guide "Preparing for v3" that I will link in this warning message.
src/lib/main.js
Outdated
@@ -110,6 +116,37 @@ import * as stateMappings from './stateMappings/index.js'; | |||
* @property {number} [stalledSearchDelay=200] Time before a search is considered stalled. | |||
* @property {RoutingOptions} [routing] the router configuration used to save the UI State into the URL or | |||
* any client side persistence. | |||
* @property {SearchClient} [searchClient] The search client to plug to instantsearch.js. You should start updating with this | |||
* syntax to ease the migration to InstantSearch 3. |
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 could link to the "Preparing for v3" guide once it's published.
src/lib/main.js
Outdated
* @property {function} [createAlgoliaClient] _Deprecated in favor of [`searchClient`](instantsearch.html#struct-InstantSearchOptions-searchClient)._<br> | ||
* Allows you to provide your own algolia client instead of the one instantiated internally by instantsearch.js. | ||
* Useful in situations where you need to setup complex mechanism on the client or if you need to share it easily. | ||
* <br>Usage: |
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 also leave a newline instead of br?
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, didn't realize we could.
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 if the guide is linked
* docs(guide): Add "Prepare for v3" guide * feat(guide): Lower nav weight for routing guide This makes the "Prepare for v3" guide last in the nav list. * feat(menu): Add "Prepare for v3" guide in the menu * docs: Add intro and change titles * docs: Add "Deprecations" * docs: Rephrase introduction * docs: Move second part of intro in first section
Waiting for #2903 to get merged to remove these lines: https://github.com/algolia/instantsearch.js/blob/de182ab7386bb442851bc2540fa125b52b68efae/src/lib/InstantSearch.js#L115-L116 |
}).toThrowErrorMatchingSnapshot(); | ||
}); | ||
|
||
it('should have default `addAlgoliaAgent()` and `clearCache()` methods', () => { |
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'll get rid of this test once #2903 is merged.
@francoischalifour, in that case, is the |
Nope, I don't think so. It is our responsibility to never mutate the given client though. |
This reverts commit 2b4d47b.
<a name=2.8.0></a> # [2.8.0](v2.7.6...v2.8.0) (2018-05-30) ### Features * **connectors:** add connectAutocomplete ([#2841](#2841)) ([4bec81e](4bec81e)), closes [/github.com//pull/2841#discussion_r188383882](https://github.com//github.com/algolia/instantsearch.js/pull/2841/issues/discussion_r188383882) [#2313](#2313) * **search-client:** Add support for Universal Search Clients ([#2894](#2894)) ([5df3c74](5df3c74)), closes [#2905](#2905)
This PR adds support for the
searchClient
parameter. This allows any InstantSearch user to plug their own search client (including a custom Algolia backend).Summary
The goal is to add support for this new feature but to still make InstantSearch work as it works right now. This is meant to be released in the next minor version. Checks have been made to make sure both APIs (current and new) don't collide.
Features
searchClient
option to plug a custom search client (other thanalgoliasearch
)createAlgoliaClient
in favor ofsearchClient
Releasing strategy
algoliasearch
, although we ship this newsearchClient
parameteralgoliasearch
that will need to be imported beforehand and plugged tosearchClient
Demo
You can check the Calendar Widget demo using InstantSearch with
searchClient
: https://instantsearch-search-client.now.shLet me know if I've forgotten any parameter collision 🙌