react-instantsearch - Update all props on InstantSearch #1828

bobylito · 2017-01-12T15:28:41Z

Summary

This PR aimed at letting the developers update the parameters of <InstantSearch/>. Doing so, we also decided to deprecate the searchParameters props and replace it with a <Configure/> widget.

Fixes #1825 #1673

Result

The tourism demo app is now GeoIP based:

Previously was creating a new client at each render.

algobot · 2017-01-12T15:29:02Z

By analyzing the blame information on this pull request, we identified @vvo, @mthuret and @Morhaus to be potential reviewers

vvo

Awesome you nailed it clearly. This is a very good PR with new tests, fixes and a attention to detail.

Thanks!

vvo · 2017-01-12T15:47:57Z

package.json

@@ -25,6 +25,7 @@
    "@kadira/storybook-addon-notes": "^1.0.1",
    "@kadira/storybook-addon-options": "^1.0.1",
    "@kadira/storybook-addons": "^1.6.1",
+    "algoliasearch-helper": "^2.18.0",


Is this needed inside the root package.json?

I guess not /o\

vvo · 2017-01-12T15:49:57Z

packages/react-instantsearch/src/core/InstantSearch.js

@@ -61,6 +61,11 @@ class InstantSearch extends Component {

  componentWillReceiveProps(nextProps) {
    validateNextProps(this.props, nextProps);
+
+    if (this.props.indexName !== nextProps.indexName) {
+      this.aisManager.updateIndex(nextProps.indexName);


vvo · 2017-01-12T15:50:35Z

packages/react-instantsearch/src/core/createInstantSearch.js

@@ -14,15 +14,34 @@ export default function createInstantSearch(defaultAlgoliaClient, root) {
      algoliaClient: PropTypes.object,
      appId: PropTypes.string,
      apiKey: PropTypes.string,
+      children: React.PropTypes.oneOfType([


Nice, how come we never had any warning on this one? Thanks

because {...props}. We used to forward any props therefore the linter couldn't know about that.

vvo · 2017-01-12T15:51:57Z

packages/react-instantsearch/src/core/createInstantSearch.js

+      if (nextProps.algoliaClient) {
+        this.client = nextProps.algoliaClient;
+      } else if (props.appId !== nextProps.appId || props.apiKey !== nextProps.apiKey) {
+        this.client = defaultAlgoliaClient(nextProps.appId, nextProps.apiKey);


vvo

Just request one update or change on the helper at the root of package.json.

vvo · 2017-01-12T15:54:55Z

You can use cd packages/react-instantsearch && yarn upgrade-interactive to ensure we have the latest helper at the right place.

Sadly the upgrade.sh seems bugged because when we want to quit one upgrade-interactive without making any changes we are not authorized to do so.

bobylito · 2017-01-12T15:59:55Z

Yep doing the proper package changes. :)

mthuret · 2017-01-12T16:01:42Z

packages/react-instantsearch/src/core/createInstantSearchManager.js

@@ -41,6 +41,11 @@ export default function createInstantSearchManager({
    searching: false,
  });

+  function updateClient(client) {
+    helper.setClient(client);
+    search(); // Will it work?


Being curious: what this comment mean?

Note for later: I should remove my own questioning.

Joke aside, I haven't done a proper and complete test on this.

Also I would be interested in your opinion here @vvo @mthuret, should we trigger a new search when the client or the indexName changes. (if not I will cry because I won't know how to test). FYI, we do trigger a new search for new values.

And I won't remove the comment until I have your opinion (because GH will hide it if there is a change at this line)

Yes we should trigger a new search when client and indexName are updated because both can have an impact on the result set:

changing the index (obvious one)

updating the client: going from logged off to logged in and having a dedicated API key that will update the result set.

Works?

Just wanted to be sure because we've discussions about related topics. Fine with you @mthuret ?

Yep all fine 👍

Also: - fix the indexName property to be able to update it. - small refactoring to explicitely define the props of createInstantSearch - update the doc accordingly

bobylito · 2017-01-13T08:25:28Z

I think I'm done here :) Thanks for the reviews :)

<a name="2.2.0"></a> # [2.2.0](v2.1.0...v2.2.0) (2017-01-17) ### Bug Fixes * **clear:** clearing wasn't working with too+ same type facets selected (#1820) ([a9a2364](a9a2364)) * **connectSearchBox:** handle `defaultRefinement` (#1829) ([7a730e2](7a730e2)), closes [#1826](#1826) * **Instantsearch:** Update all props on InstantSearch (#1828) ([2ed9b49](2ed9b49)) * **InstantSearch:** add specific `react-instantsearch ${version}` agent (#1844) ([a1113bc](a1113bc)) * **SFFV:** correct propTypes and add missing default values (#1845) ([a4c1b31](a4c1b31)) * **test:** add missing Snippet and Highliter snapshot ([4accce5](4accce5)) * **widgets:** replace setImmediate use with Promise use when update is needed (#1811) ([17e2497](17e2497)) ### Features * **Menu, connectMenu:** add search for facet values (#1822) ([a6c513e](a6c513e)) * **snippet:** add a snippet widget to be able to highlight snippet results (#1797) ([2aecc40](2aecc40)) * **widgets:** add transformItems to be able to sort and filter (#1809) ([ba539f0](ba539f0))

Alexandre Stanislawski added 2 commits January 11, 2017 10:35

fix(createInstantSearch): new credentials creates a new client

ffb939c

Previously was creating a new client at each render.

fix(isManager): implement update of client

40e621e

bobylito changed the title ~~IS v2 - Update all props on InstantSearch~~ react-instantsearch - Update all props on InstantSearch Jan 12, 2017

vvo approved these changes Jan 12, 2017

View reviewed changes

vvo suggested changes Jan 12, 2017

View reviewed changes

mthuret reviewed Jan 12, 2017

View reviewed changes

bobylito force-pushed the fix/update-all-props branch from f8a82a9 to cd643f4 Compare January 12, 2017 16:06

feat(SearchParameters): Move InstantSearch config to <Configure/>

1f89918

Also: - fix the indexName property to be able to update it. - small refactoring to explicitely define the props of createInstantSearch - update the doc accordingly

bobylito force-pushed the fix/update-all-props branch from cd643f4 to 1f89918 Compare January 13, 2017 08:25

vvo approved these changes Jan 16, 2017

View reviewed changes

bobylito merged commit 2ed9b49 into v2 Jan 16, 2017

bobylito deleted the fix/update-all-props branch January 16, 2017 09:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

react-instantsearch - Update all props on InstantSearch #1828

react-instantsearch - Update all props on InstantSearch #1828

bobylito commented Jan 12, 2017 •

edited by vvo

Loading

algobot commented Jan 12, 2017

vvo left a comment

vvo Jan 12, 2017

bobylito Jan 12, 2017

vvo Jan 12, 2017

vvo Jan 12, 2017

bobylito Jan 12, 2017

vvo Jan 12, 2017

vvo left a comment

vvo commented Jan 12, 2017 •

edited

Loading

bobylito commented Jan 12, 2017

mthuret Jan 12, 2017

bobylito Jan 12, 2017

bobylito Jan 12, 2017

vvo Jan 12, 2017

bobylito Jan 12, 2017

mthuret Jan 12, 2017

bobylito commented Jan 13, 2017

react-instantsearch - Update all props on InstantSearch #1828

react-instantsearch - Update all props on InstantSearch #1828

Conversation

bobylito commented Jan 12, 2017 • edited by vvo Loading

algobot commented Jan 12, 2017

vvo 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

vvo left a comment

Choose a reason for hiding this comment

vvo commented Jan 12, 2017 • edited Loading

bobylito commented Jan 12, 2017

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

bobylito commented Jan 13, 2017

bobylito commented Jan 12, 2017 •

edited by vvo

Loading

vvo commented Jan 12, 2017 •

edited

Loading