feat(search-client): Add support for Custom Search Clients #2894

francoischalifour · 2018-04-17T11:29:08Z

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.

// With the Algolia JS client
instantsearch({
  indexName: 'concert_events_instantsearchjs',
  searchClient: algoliasearch('latency', '059c79ddd276568e990286944276464a'),
});

// With a custom search client
instantsearch({
  indexName: 'concert_events_instantsearchjs',
  searchClient: {
    search(requests) {
      // perform requests...
      return response;
    },
  },
});

Features

Add searchClient option to plug a custom search client (other than algoliasearch)
Deprecate createAlgoliaClient in favor of searchClient
Add tests to make sure the current and new API don't collide

Releasing strategy

the next minor version will embed algoliasearch, although we ship this new searchClient parameter
the next major version will get rid of algoliasearch that will need to be imported beforehand and plugged to searchClient

Demo

You can check the Calendar Widget demo using InstantSearch with searchClient: https://instantsearch-search-client.now.sh

Let me know if I've forgotten any parameter collision 🙌

algobot · 2018-04-17T11:32:56Z

Deploy preview for algolia-instantsearch ready!

Built with commit fa0ca7f

https://deploy-preview-2894--algolia-instantsearch.netlify.com

Haroenv

I like this feature!

Haroenv · 2018-04-17T11:33:08Z

src/lib/InstantSearch.js

-    const client = createAlgoliaClient(algoliasearch, appId, apiKey);
-    client.addAlgoliaAgent(`instantsearch.js ${version}`);
+    // eslint-disable-next-line prefer-rest-params
+    const [THROWAWAY_PARAMS] = arguments;


this line is a bit odd, what about giving the args to this function this name, and destructure after this line?

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?

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

Haroenv · 2018-04-17T11:33:56Z

src/lib/__tests__/search-client-test.js

+    search.start();
+
+    expect(search.helper.state.query).toBe('');
+    expect(searchClientSpy.search).toHaveBeenCalledTimes(1);


also useful to assert with what it's been called?

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?

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

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)

bobylito · 2018-04-18T09:16:54Z

src/lib/__tests__/THROWAWAY-instanciation-test.js

+    it('and nothing else', () => {
+      expect(() => {
+        new InstantSearch({
+          searchClient: {},


Miss indexName

bobylito · 2018-04-18T09:17:40Z

src/lib/__tests__/THROWAWAY-instanciation-test.js

+      }).toThrow();
+    });
+
+    it('and indexname, appId, apiKey', () => {


Maybe not necessary.

bobylito · 2018-04-18T09:27:33Z

src/lib/__tests__/search-client-test.js

+    search.start();
+
+    expect(search.helper.state.query).toBe('');
+    expect(searchClientSpy.search).toHaveBeenCalledTimes(1);


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)

bobylito · 2018-04-18T09:33:05Z

src/lib/__tests__/search-client-test.js

+    search.start();
+
+    expect(searchFunctionSpy).toHaveBeenCalledTimes(1);
+    expect(search.helper.state.query).toBe('test');


I'd use the same strategy as for the previous test

Haroenv · 2018-04-18T14:15:55Z

src/lib/InstantSearch.js

+      }
+
+      const client = {
+        __proto__: Object.getPrototypeOf(searchClient),


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?

or better:

const client = Object.create(person); client.addAlgoliaAgent = client.addAlgoliaAgent || function() {}; client.clearCache = client.clearCache || function() {};

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 😄

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().

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?

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

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

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?

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"

vvo · 2018-04-19T09:57:46Z

src/lib/InstantSearch.js

+    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.'


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

Yes, I'm working on a guide "Preparing for v3" that I will link in this warning message.

francoischalifour · 2018-04-19T13:24:15Z

src/lib/main.js

@@ -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.


We could link to the "Preparing for v3" guide once it's published.

Haroenv · 2018-04-19T13:51:38Z

src/lib/main.js

+ * @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:


can you also leave a newline instead of br?

Yes, didn't realize we could.

Haroenv

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

francoischalifour · 2018-04-25T12:01:45Z

Waiting for #2903 to get merged to remove these lines: https://github.com/algolia/instantsearch.js/blob/de182ab7386bb442851bc2540fa125b52b68efae/src/lib/InstantSearch.js#L115-L116

francoischalifour · 2018-04-25T12:03:39Z

src/lib/__tests__/search-client-test.js

+      }).toThrowErrorMatchingSnapshot();
+    });
+
+    it('should have default `addAlgoliaAgent()` and `clearCache()` methods', () => {


We'll get rid of this test once #2903 is merged.

Haroenv · 2018-04-25T12:10:22Z

@francoischalifour, in that case, is the object.create even still necessary?

francoischalifour · 2018-04-25T12:12:32Z

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-beta.0></a> # [2.8.0-beta.0](v2.7.2...v2.8.0-beta.0) (2018-04-26) ### Features * **search-client:** Add support for Universal Search Clients ([#2894](#2894)) ([5df3c74](5df3c74)), closes [#2905](#2905)

<a name=2.8.0-beta.1></a> # [2.8.0-beta.1](v2.7.4...v2.8.0-beta.1) (2018-05-03) ### Features * **search-client:** Add support for Universal Search Clients ([#2894](#2894)) ([5df3c74](5df3c74)), closes [#2905](#2905)

<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)

francoischalifour added 2 commits April 17, 2018 13:03

feat(search-client): Add support and checks for searchClient

b085d77

test(search-client): Add tests for searchClient

a700991

Haroenv reviewed Apr 17, 2018

View reviewed changes

bobylito reviewed Apr 18, 2018

View reviewed changes

bobylito changed the base branch from develop to feat/2.8 April 18, 2018 09:36

francoischalifour added 8 commits April 18, 2018 14:30

refactor(search-client): Refactor code

76c242e

feat: Throw if searchClient doesn't implement search()

113a56c

test: Test searchClient throws error if no search() method

af50f97

feat: Deprecate createAlgoliaClient

3c4da58

test: Test error message thrown

fd78e86

test: Snapshot test the search client method input

1b50bb4

fix: Deep copy of the search client

dae8d23

ci(lint): Rename FIXME to THROWAWAY for eslint to not fail

9fc0eb9

Haroenv reviewed Apr 18, 2018

View reviewed changes

vvo reviewed Apr 19, 2018

View reviewed changes

francoischalifour added 2 commits April 19, 2018 14:18

refactor(searchClient): Clone searchClient with Object.create()

28c8f0e

docs(searchClient): Add docs for searchClient

0bc3cf7

francoischalifour commented Apr 19, 2018

View reviewed changes

Haroenv reviewed Apr 19, 2018

View reviewed changes

francoischalifour added 2 commits April 19, 2018 16:07

docs: Replace br with new lines

d6ce1c7

docs: Document why we need Object.create()

462b0c1

Haroenv reviewed Apr 20, 2018

View reviewed changes

francoischalifour commented Apr 25, 2018

View reviewed changes

docs: Link guide to searchClient

2de38bb

francoischalifour mentioned this pull request Apr 25, 2018

feat(search-client): Add support for Custom Search Clients algolia/vue-instantsearch#432

Merged

francoischalifour and others added 8 commits April 26, 2018 11:19

Merge branch 'feat/2.8' into feat/search-client-support

35cba06

refactor: Update client assignation

627d7f2

test: Update client.search()

517c109

chore: Add lock file

2b4d47b

revert: Add lock file

837c47e

This reverts commit 2b4d47b.

feat: Change usage message for search clients

ae351c8

test: Update usage

dca3fab

feat: Update warning for createAlgoliaClient

fa0ca7f

bobylito approved these changes Apr 26, 2018

View reviewed changes

bobylito merged commit 5df3c74 into feat/2.8 Apr 26, 2018

bobylito deleted the feat/search-client-support branch April 26, 2018 14:37

This was referenced May 3, 2018

feat(search-client): Add support for Custom Search Clients algolia/angular-instantsearch#111

Merged

feat(search-client): Add support for Custom Search Clients algolia/react-instantsearch#1216

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(search-client): Add support for Custom Search Clients #2894

feat(search-client): Add support for Custom Search Clients #2894

francoischalifour commented Apr 17, 2018 •

edited

Loading

algobot commented Apr 17, 2018 •

edited

Loading

Haroenv left a comment

Haroenv Apr 17, 2018

francoischalifour Apr 17, 2018

Haroenv Apr 17, 2018

Haroenv Apr 17, 2018

francoischalifour Apr 17, 2018

Haroenv Apr 17, 2018

bobylito Apr 18, 2018

bobylito Apr 18, 2018

bobylito Apr 18, 2018

bobylito Apr 18, 2018

bobylito Apr 18, 2018

Haroenv Apr 18, 2018

Haroenv Apr 18, 2018

Haroenv Apr 18, 2018

francoischalifour Apr 18, 2018

vvo Apr 19, 2018

vvo Apr 19, 2018

Haroenv Apr 19, 2018

francoischalifour Apr 19, 2018

vvo Apr 19, 2018 •

edited by Haroenv

Loading

vvo Apr 19, 2018

francoischalifour Apr 19, 2018

francoischalifour Apr 19, 2018

Haroenv Apr 19, 2018

francoischalifour Apr 19, 2018

Haroenv left a comment

francoischalifour commented Apr 25, 2018

francoischalifour Apr 25, 2018

Haroenv commented Apr 25, 2018

francoischalifour commented Apr 25, 2018

feat(search-client): Add support for Custom Search Clients #2894

feat(search-client): Add support for Custom Search Clients #2894

Conversation

francoischalifour commented Apr 17, 2018 • edited Loading

Summary

Features

Releasing strategy

Demo

algobot commented Apr 17, 2018 • edited Loading

Haroenv left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvo Apr 19, 2018 • edited by Haroenv Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Haroenv left a comment

Choose a reason for hiding this comment

francoischalifour commented Apr 25, 2018

Choose a reason for hiding this comment

Haroenv commented Apr 25, 2018

francoischalifour commented Apr 25, 2018

francoischalifour commented Apr 17, 2018 •

edited

Loading

algobot commented Apr 17, 2018 •

edited

Loading

vvo Apr 19, 2018 •

edited by Haroenv

Loading