Skip to content

Commit

Permalink
fix(queryHook): restore behaviour of queryHook (#4202)
Browse files Browse the repository at this point in the history 
* 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:
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 #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
  • Loading branch information
@tkrugg
tkrugg committed Nov 26, 2019
1 parent c9c57a8 commit 7bf96cb
Show file tree
Hide file tree
Showing 7 changed files with 198 additions and 61 deletions.
2 changes: 1 addition & 1 deletion package.json
View file
Expand Up @@ -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",
Expand Down
4 changes: 4 additions & 0 deletions src/components/RefinementList/__tests__/__snapshots__/RefinementList-test.js.snap
View file
Expand Up @@ -381,6 +381,8 @@ Array [
className="input"
disabled={false}
maxLength={512}
onBlur={[Function]}
onFocus={[Function]}
onInput={[Function]}
placeholder="Search"
spellCheck={false}
Expand Down Expand Up @@ -568,6 +570,8 @@ Array [
className="input"
disabled={false}
maxLength={512}
onBlur={[Function]}
onFocus={[Function]}
onInput={[Function]}
placeholder="Search"
spellCheck={false}
Expand Down
45 changes: 32 additions & 13 deletions src/components/SearchBox/SearchBox.js
View file
Expand Up @@ -52,7 +52,8 @@ class SearchBox extends Component {
};

state = {
query: this.props.searchAsYouType ? '' : this.props.query,
query: this.props.query,
focused: false,
};

/**
Expand All @@ -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;

Expand All @@ -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,
Expand All @@ -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
Expand All @@ -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"
Expand All @@ -149,6 +162,8 @@ class SearchBox extends Component {
spellCheck={false}
maxLength={512}
onInput={this.onInput}
onBlur={this.onBlur}
onFocus={this.onFocus}
/>

<Template
Expand All @@ -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 }}
Expand Down
181 changes: 137 additions & 44 deletions src/components/SearchBox/__tests__/SearchBox-test.js
View file
Expand Up @@ -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();
Expand Down Expand Up @@ -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);
});
});
});
});
Expand Down
4 changes: 4 additions & 0 deletions src/components/SearchBox/__tests__/__snapshots__/SearchBox-test.js.snap
View file
Expand Up @@ -21,6 +21,8 @@ Array [
className="input"
disabled={false}
maxLength={512}
onBlur={[Function]}
onFocus={[Function]}
onInput={[Function]}
placeholder=""
spellCheck={false}
Expand Down Expand Up @@ -90,6 +92,8 @@ Array [
className="input"
disabled={false}
maxLength={512}
onBlur={[Function]}
onFocus={[Function]}
onInput={[Function]}
placeholder=""
spellCheck={false}
Expand Down
17 changes: 17 additions & 0 deletions stories/search-box.stories.js
View file
Expand Up @@ -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);
},
}),
]);
})
);
6 changes: 3 additions & 3 deletions yarn.lock
View file
Expand Up @@ -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"
Expand Down

0 comments on commit 7bf96cb

Please sign in to comment.