fix(queryHook): restore behaviour of queryHook (algolia#4202)

* fix(queryHook): restore behaviour of queryHook There has been many complaints from user on our [debouncing guide](https://www.algolia.com/doc/guides/building-search-ui/going-further/improve-performance/js/#debouncing) no longer working as expected. This issue was introduced when the SearchBox component was converted into a Preact, and the following condition removed: algolia@c073a9a#diff-530222e0c4597f2110dc6ba173a306b0L98 **Before SearchBox refactor**: - when input was focused, the query exposed by the connector didn't update the input value - when input was not focused, the query exposed by the connector did update the input value **After SearchBox refactor**: - regardless on whether input was focused, the query exposed by the connector updates the input value This PR restores the logic as found before the refactor. fixes algolia#4141 * chore: update snapshots * fix: reset works again * add a comment explaining the bahviour of the widget on focus * add variable names to comment * chore: update e2e tests * add comment explaining test puropose
7Mat7 · Nov 26, 2019 · 7bf96cb · 7bf96cb
1 parent c9c57a8
commit 7bf96cb
Show file tree

Hide file tree

Showing 7 changed files with 198 additions and 61 deletions.
diff --git a/package.json b/package.json
@@ -113,7 +113,7 @@
     "eslint-plugin-react": "7.16.0",
     "eslint-plugin-react-hooks": "2.3.0",
     "inquirer": "6.5.2",
-    "instantsearch-e2e-tests": "algolia/instantsearch-e2e-tests#v1.2.3",
+    "instantsearch-e2e-tests": "algolia/instantsearch-e2e-tests#1.2.4",
     "jest": "24.9.0",
     "jest-diff": "24.9.0",
     "jest-environment-jsdom": "24.9.0",

diff --git a/src/components/RefinementList/__tests__/__snapshots__/RefinementList-test.js.snap b/src/components/RefinementList/__tests__/__snapshots__/RefinementList-test.js.snap
@@ -381,6 +381,8 @@ Array [
               className="input"
               disabled={false}
               maxLength={512}
+              onBlur={[Function]}
+              onFocus={[Function]}
               onInput={[Function]}
               placeholder="Search"
               spellCheck={false}
@@ -568,6 +570,8 @@ Array [
               className="input"
               disabled={false}
               maxLength={512}
+              onBlur={[Function]}
+              onFocus={[Function]}
               onInput={[Function]}
               placeholder="Search"
               spellCheck={false}

diff --git a/src/components/SearchBox/SearchBox.js b/src/components/SearchBox/SearchBox.js
@@ -52,7 +52,8 @@ class SearchBox extends Component {
   };
 
   state = {
-    query: this.props.searchAsYouType ? '' : this.props.query,
+    query: this.props.query,
+    focused: false,
   };
 
   /**
@@ -72,13 +73,23 @@ class SearchBox extends Component {
 
     if (searchAsYouType) {
       refine(query);
-    } else {
-      this.setState({ query });
     }
+    this.setState({ query });
 
     onChange(event);
   };
 
+  componentWillReceiveProps(nextProps) {
+    /**
+     * when the user is typing, we don't want to replace the query typed
+     * by the user (state.query) with the query exposed by the connector (props.query)
+     * see: https://github.com/algolia/instantsearch.js/issues/4141
+     */
+    if (!this.state.focused && nextProps.query !== this.state.query) {
+      this.setState({ query: nextProps.query });
+    }
+  }
+
   onSubmit = event => {
     const { searchAsYouType, refine, onSubmit } = this.props;
 
@@ -96,20 +107,25 @@ class SearchBox extends Component {
   };
 
   onReset = event => {
-    const { searchAsYouType, refine, onReset } = this.props;
+    const { refine, onReset } = this.props;
     const query = '';
 
     this.input.focus();
 
     refine(query);
-
-    if (!searchAsYouType) {
-      this.setState({ query });
-    }
+    this.setState({ query });
 
     onReset(event);
   };
 
+  onBlur = () => {
+    this.setState({ focused: false });
+  };
+
+  onFocus = () => {
+    this.setState({ focused: true });
+  };
+
   render() {
     const {
       cssClasses,
@@ -120,11 +136,8 @@ class SearchBox extends Component {
       showLoadingIndicator,
       templates,
       isSearchStalled,
-      searchAsYouType,
     } = this.props;
 
-    const query = searchAsYouType ? this.props.query : this.state.query;
-
     return (
       <div className={cssClasses.root}>
         <form
@@ -137,7 +150,7 @@ class SearchBox extends Component {
         >
           <input
             ref={inputRef => (this.input = inputRef)}
-            value={query}
+            value={this.state.query}
             disabled={this.props.disabled}
             className={cssClasses.input}
             type="search"
@@ -149,6 +162,8 @@ class SearchBox extends Component {
             spellCheck={false}
             maxLength={512}
             onInput={this.onInput}
+            onBlur={this.onBlur}
+            onFocus={this.onFocus}
           />
 
           <Template
@@ -171,7 +186,11 @@ class SearchBox extends Component {
               className: cssClasses.reset,
               type: 'reset',
               title: 'Clear the search query.',
-              hidden: !(showReset && query.trim() && !isSearchStalled),
+              hidden: !(
+                showReset &&
+                this.state.query.trim() &&
+                !isSearchStalled
+              ),
             }}
             templates={templates}
             data={{ cssClasses }}

diff --git a/src/components/SearchBox/__tests__/SearchBox-test.js b/src/components/SearchBox/__tests__/SearchBox-test.js
@@ -184,6 +184,40 @@ describe('SearchBox', () => {
   });
 
   describe('Events', () => {
+    describe('focus/blur', () => {
+      test('does not derive value from prop when focused', () => {
+        // This makes sure we don't override the user's input while they're typing.
+        // This issue is more obvious when using queryHook to add debouncing.
+
+        const props = {
+          ...defaultProps,
+          query: 'Initial query',
+        };
+        const { container, rerender } = render(<SearchBox {...props} />);
+        const input = container.querySelector('input');
+        expect(input.value).toEqual('Initial query');
+
+        fireEvent.focus(input);
+        rerender(<SearchBox {...props} query={'Query updated through prop'} />);
+
+        expect(input.value).toEqual('Initial query');
+      });
+
+      test('derives value from prop when not focused', () => {
+        const props = {
+          ...defaultProps,
+          query: 'Initial query',
+        };
+        const { container, rerender } = render(<SearchBox {...props} />);
+        const input = container.querySelector('input');
+        expect(input.value).toEqual('Initial query');
+
+        fireEvent.blur(input);
+        rerender(<SearchBox {...props} query={'Query updated through prop'} />);
+
+        expect(input.value).toEqual('Query updated through prop');
+      });
+    });
     describe('searchAsYouType to true', () => {
       test('refines input value on input', () => {
         const refine = jest.fn();
@@ -281,64 +315,123 @@ describe('SearchBox', () => {
       });
     });
 
-    // @TODO This test suite will pass once we upgrade Jest to 25.x.
-    // See https://github.com/algolia/instantsearch.js/issues/4160
-    // eslint-disable-next-line jest/no-disabled-tests
-    describe.skip('onReset', () => {
-      test('resets the input value with searchAsYouType to true', () => {
-        const props = {
-          ...defaultProps,
-          searchAsYouType: true,
-        };
-        const { container } = render(<SearchBox {...props} />);
-        const input = container.querySelector('input');
-        const resetButton = container.querySelector('button[type="reset"]');
+    describe('onReset', () => {
+      // @TODO This test suite will pass once we upgrade Jest to 25.x.
+      // See https://github.com/algolia/instantsearch.js/issues/4160
+      // eslint-disable-next-line jest/no-disabled-tests
+      describe.skip('with button click', () => {
+        test('resets the input value with searchAsYouType to true', () => {
+          const props = {
+            ...defaultProps,
+            searchAsYouType: true,
+          };
+          const { container } = render(<SearchBox {...props} />);
+          const input = container.querySelector('input');
+          const resetButton = container.querySelector('button[type="reset"]');
+
+          fireEvent.change(input, {
+            target: { value: 'hello' },
+          });
+
+          expect(input.value).toEqual('hello');
+
+          fireEvent.click(resetButton);
+
+          expect(input.value).toEqual('');
+          expect(input).toHaveFocus();
+        });
 
-        fireEvent.change(input, {
-          target: { value: 'hello' },
+        test('resets the input value with searchAsYouType to false', () => {
+          const props = {
+            ...defaultProps,
+            searchAsYouType: false,
+          };
+          const { container } = render(<SearchBox {...props} />);
+          const form = container.querySelector('form');
+          const input = container.querySelector('input');
+          const resetButton = container.querySelector('button[type="reset"]');
+
+          fireEvent.input(input, { target: { value: 'hello' } });
+          fireEvent.submit(form);
+
+          expect(input.value).toEqual('hello');
+
+          fireEvent.click(resetButton);
+
+          expect(input.value).toEqual('');
+          expect(input).toHaveFocus();
         });
 
-        expect(input.value).toEqual('hello');
+        test('calls custom onReset', () => {
+          const onReset = jest.fn();
+          const props = {
+            ...defaultProps,
+            onReset,
+          };
+          const { container } = render(<SearchBox {...props} />);
+          const resetButton = container.querySelector('button[type="reset"]');
 
-        fireEvent.click(resetButton);
+          fireEvent.click(resetButton);
 
-        expect(input.value).toEqual('');
-        expect(input).toHaveFocus();
+          expect(onReset).toHaveBeenCalledTimes(1);
+        });
       });
 
-      test('resets the input value with searchAsYouType to false', () => {
-        const props = {
-          ...defaultProps,
-          searchAsYouType: false,
-        };
-        const { container } = render(<SearchBox {...props} />);
-        const form = container.querySelector('form');
-        const input = container.querySelector('input');
-        const resetButton = container.querySelector('button[type="reset"]');
+      describe('when form is reset programmatically', () => {
+        test('resets the input value with searchAsYouType to true', () => {
+          const props = {
+            ...defaultProps,
+            searchAsYouType: true,
+          };
+          const { container } = render(<SearchBox {...props} />);
+          const input = container.querySelector('input');
+          const form = container.querySelector('form');
 
-        fireEvent.input(input, { target: { value: 'hello' } });
-        fireEvent.submit(form);
+          fireEvent.change(input, {
+            target: { value: 'hello' },
+          });
 
-        expect(input.value).toEqual('hello');
+          expect(input.value).toEqual('hello');
 
-        fireEvent.click(resetButton);
+          fireEvent.reset(form);
 
-        expect(input.value).toEqual('');
-        expect(input).toHaveFocus();
-      });
+          expect(input.value).toEqual('');
+          expect(input).toHaveFocus();
+        });
 
-      test('calls custom onReset', () => {
-        const onReset = jest.fn();
-        const props = {
-          ...defaultProps,
-          onReset,
-        };
-        const { container } = render(<SearchBox {...props} />);
-        const resetButton = container.querySelector('button[type="reset"]');
+        test('resets the input value with searchAsYouType to false', () => {
+          const props = {
+            ...defaultProps,
+            searchAsYouType: false,
+          };
+          const { container } = render(<SearchBox {...props} />);
+          const form = container.querySelector('form');
+          const input = container.querySelector('input');
+
+          fireEvent.input(input, { target: { value: 'hello' } });
+          fireEvent.submit(form);
 
-        fireEvent.click(resetButton);
+          expect(input.value).toEqual('hello');
 
-        expect(onReset).toHaveBeenCalledTimes(1);
+          fireEvent.reset(form);
+
+          expect(input.value).toEqual('');
+          expect(input).toHaveFocus();
+        });
+
+        test('calls custom onReset', () => {
+          const onReset = jest.fn();
+          const props = {
+            ...defaultProps,
+            onReset,
+          };
+          const { container } = render(<SearchBox {...props} />);
+          const form = container.querySelector('form');
+
+          fireEvent.reset(form);
+
+          expect(onReset).toHaveBeenCalledTimes(1);
+        });
       });
     });
   });

diff --git a/src/components/SearchBox/__tests__/__snapshots__/SearchBox-test.js.snap b/src/components/SearchBox/__tests__/__snapshots__/SearchBox-test.js.snap
@@ -21,6 +21,8 @@ Array [
         className="input"
         disabled={false}
         maxLength={512}
+        onBlur={[Function]}
+        onFocus={[Function]}
         onInput={[Function]}
         placeholder=""
         spellCheck={false}
@@ -90,6 +92,8 @@ Array [
         className="input"
         disabled={false}
         maxLength={512}
+        onBlur={[Function]}
+        onFocus={[Function]}
         onInput={[Function]}
         placeholder=""
         spellCheck={false}

diff --git a/stories/search-box.stories.js b/stories/search-box.stories.js
@@ -82,4 +82,21 @@ storiesOf('Basics|SearchBox', module)
         }),
       ]);
     })
+  )
+  .add(
+    'with debounced queryHook',
+    withHits(({ search, container, instantsearch }) => {
+      let timerId;
+      search.addWidgets([
+        instantsearch.widgets.searchBox({
+          container,
+          queryHook(query, refine) {
+            clearTimeout(timerId);
+            timerId = setTimeout(() => {
+              refine(query);
+            }, 100);
+          },
+        }),
+      ]);
+    })
   );
diff --git a/yarn.lock b/yarn.lock
@@ -7606,9 +7606,9 @@ insert-css@^2.0.0:
   resolved "https://registry.yarnpkg.com/insert-css/-/insert-css-2.0.0.tgz#eb5d1097b7542f4c79ea3060d3aee07d053880f4"
   integrity sha1-610Ql7dUL0x56jBg067gfQU4gPQ=
 
-instantsearch-e2e-tests@algolia/instantsearch-e2e-tests#v1.2.3:
-  version "1.2.3"
-  resolved "https://codeload.github.com/algolia/instantsearch-e2e-tests/tar.gz/856d5d788ff25cfeeab43d3aa7dbd10ab98b0182"
+instantsearch-e2e-tests@algolia/instantsearch-e2e-tests#1.2.4:
+  version "1.2.4"
+  resolved "https://codeload.github.com/algolia/instantsearch-e2e-tests/tar.gz/da70c1c4c0d1d0f3b6db2bbce1491e3103cf44fd"
   dependencies:
     dotenv "^8.0.0"
     ts-node "^8.3.0"