Skip to content
This repository has been archived by the owner on Dec 30, 2022. It is now read-only.

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

Merged
merged 15 commits into from
May 17, 2018

Conversation

francoischalifour
Copy link
Member

@francoischalifour francoischalifour commented May 4, 2018

This PR adds support for the searchClient prop. This allows any React InstantSearch user to plug their own search client (including a custom Algolia backend).

Related

@algobot
Copy link
Contributor

algobot commented May 4, 2018

Deploy preview for react-instantsearch ready!

Built with commit 4e33880

https://deploy-preview-1216--react-instantsearch.netlify.com

Copy link
Contributor

@Haroenv Haroenv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good when reading the code

@Haroenv
Copy link
Contributor

Haroenv commented May 4, 2018

some tests are failing and snapshot tests not updated though :)

@francoischalifour
Copy link
Member Author

Yep, I blindly coded the feature. It should be on top of #1215 to work.

@francoischalifour
Copy link
Member Author

I've updated the branch.

I'm not sure about updating the snapshot in 6b70d2b though @samouss. Did I have to do that or did I do something wrong? (algoliaClient didn't appear in the snapshot before)

algoliaClient: PropTypes.object.isRequired,
algoliaClient: PropTypes.object,

searchClient: PropTypes.object.isRequired,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need to duplicate the props, we already do the logic to chose the correct implementation on the previous layer. But we can still rename it to searchClient instead of algoliaClient.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We somehow need to keep algoliaClient since we deprecate it until the next major version, right?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes but this component is not part of the public API. The part that is exposed is createInstantSearch, it's required to keep both algoliaClient & searchClient in this component but not in InstantSearch.

const InstantSearch = createInstantSearch(algoliasearch, {
Root: 'div',
props: { className: 'ais-InstantSearch__root' },
});
export { InstantSearch };

],
],
},
},
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the previous implementation we don't pass the alogliaClient to the Snapshot, otherwise every time we release a new version we need to update them. We need to do the same with searchClient

it('wraps InstantSearch', () => {
const wrapper = shallow(
<CustomInstantSearch appId="app" apiKey="key" indexName="name" />
);
// eslint-disable-next-line no-shadow
const { algoliaClient, ...propsWithoutClient } = wrapper.props();
expect(wrapper.is(InstantSearch)).toBe(true);
expect(propsWithoutClient).toMatchSnapshot();
expect(wrapper.props().algoliaClient).toBe(algoliaClient);
});

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get it!

@@ -42,24 +43,49 @@ export default function createInstantSearch(defaultAlgoliaClient, root) {
constructor(...args) {
super(...args);

if (this.props.searchClient) {
if (this.props.appId || this.props.apiKey || this.props.algoliaClient) {
// eslint-disable-next-line no-console
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove this comment, we don't use the console.

@@ -42,24 +43,49 @@ export default function createInstantSearch(defaultAlgoliaClient, root) {
constructor(...args) {
super(...args);

if (this.props.searchClient) {
if (this.props.appId || this.props.apiKey || this.props.algoliaClient) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can cover those cases with tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I messed up with my rebase. Will add it back!

if (props.searchClient) {
if (props.appId || props.apiKey || props.algoliaClient) {
// eslint-disable-next-line no-console
console.warn(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the implementation of createInstantSearch, we should throw an error in this case. We should also warn the user that algoliaClient is deprecated when he's using it.

this.props.algoliaClient ||
defaultAlgoliaClient(this.props.appId, this.props.apiKey);

this.client.addAlgoliaAgent(`react-instantsearch ${version}`);
if (typeof this.client.addAlgoliaAgent === 'function') {
this.client.addAlgoliaAgent(`react-instantsearch ${version}`);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can cover this case with a test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

if (nextProps.algoliaClient) {

if (nextProps.searchClient) {
this.client = nextProps.searchClient;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can cover this case with a test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done! (I messed up with my rebase)

this.client.addAlgoliaAgent(`react-instantsearch ${version}`);

if (typeof this.client.addAlgoliaAgent === 'function') {
this.client.addAlgoliaAgent(`react-instantsearch ${version}`);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can cover this case with a test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

this.client =
this.props.searchClient ||
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can cover this case with a test.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@francoischalifour
Copy link
Member Author

All changes have been made @samouss.

@@ -29,6 +29,7 @@ function validateNextProps(props, nextProps) {
* @propType {string} indexName - Main index in which to search.
* @propType {boolean} [refresh=false] - Flag to activate when the cache needs to be cleared so that the front-end is updated when a change occurs in the index.
* @propType {object} [algoliaClient] - Provide a custom Algolia client instead of the internal one.
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove this prop from the documentation since we deprecate the option. What is the strategy in the others flavours?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe description can have “deprecated” in it

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, we deprecate algoliaClient before removing it in the next major version. I'll add a mention.

if (typeof client.addAlgoliaAgent === 'function') {
client.addAlgoliaAgent(`react-instantsearch ${version}`);
}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can cover those cases with tests.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@samouss samouss merged commit 174cc28 into master May 17, 2018
@samouss samouss deleted the feat/search-client branch May 17, 2018 10:11
samouss added a commit that referenced this pull request May 28, 2018
<a name="5.1.0"></a>
# [5.1.0](v5.0.3...v5.1.0) (2018-05-28)

### Bug Fixes

* **connectInfiniteHits:** always set a value for previous page ([#1195](#1195)) ([4c218d5](4c218d5))
* **indexUtils:** avoid throw an error on cleanUp multi indices ([#1265](#1265)) ([12f5ace](12f5ace))

### Features

* **searchClient:** Add support for custom Search Clients ([#1216](#1216)) ([174cc28](174cc28))
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants