Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: handle late resolving promises with promise cancelation #864

Merged
merged 41 commits into from
Jan 26, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
41 commits
Select commit Hold shift + click to select a range
6cc5451
fix: handle late resolving promises with promise cancelation
sarahdayan Jan 13, 2022
f01de26
feat: inline cancelable-promise
sarahdayan Jan 14, 2022
00526a3
chore: remove dependency
sarahdayan Jan 14, 2022
d40c06a
feat: drop unused methods
sarahdayan Jan 14, 2022
244c0db
feat: use functions instead of classes
sarahdayan Jan 18, 2022
7610dcc
refactor: rename file
sarahdayan Jan 18, 2022
a8d9b3e
fix: fix types
sarahdayan Jan 20, 2022
65a9483
fix: fix inconsistent behavior
sarahdayan Jan 20, 2022
46468ff
refactor: rename type
sarahdayan Jan 20, 2022
9ad3000
test: update tests
sarahdayan Jan 20, 2022
516875c
build: bump bundle size
sarahdayan Jan 21, 2022
8e2f05a
style: lint
sarahdayan Jan 21, 2022
9702ecf
refactor: move things around
sarahdayan Jan 21, 2022
7f484a7
feat: remove onCancel handler
sarahdayan Jan 21, 2022
1eb4980
refactor: rename type
sarahdayan Jan 21, 2022
4bf6c0e
fix: simplify code by narrowing type
sarahdayan Jan 21, 2022
a60e0d8
refactor: further simplify
sarahdayan Jan 21, 2022
6e633fd
build: adjust bundlesize
sarahdayan Jan 21, 2022
00b9cd0
build: drop yarn.lock changes
sarahdayan Jan 21, 2022
511d7e3
refactor: alphabetize
sarahdayan Jan 21, 2022
467ac44
test: clean up tests
sarahdayan Jan 24, 2022
14a70d4
feat: catch in promise consumer to avoid unhandled promises
sarahdayan Jan 24, 2022
494eb7c
chore: rename type and generic
sarahdayan Jan 24, 2022
84a259f
test: use terser descriptions
sarahdayan Jan 24, 2022
24e2a66
test: better descriptions
sarahdayan Jan 24, 2022
cef48a0
test: make the test more exhaustive
sarahdayan Jan 24, 2022
aedb8c2
refactor: rename internal parameter
sarahdayan Jan 24, 2022
267b700
refactor: rename type
sarahdayan Jan 24, 2022
a431e71
test: refactor test
sarahdayan Jan 24, 2022
dcc7d23
test: further test concurrency behavior
sarahdayan Jan 24, 2022
971eeb0
refactor: inline item removal
sarahdayan Jan 24, 2022
d68de50
feat: remove runWhenCanceled option
sarahdayan Jan 24, 2022
3d65e25
feat: clear callbacks once triggered
sarahdayan Jan 24, 2022
5163e7d
test: add missing test in both suites
sarahdayan Jan 24, 2022
de4a205
fix: s/running/pending/
sarahdayan Jan 25, 2022
ffd16fe
refactor: always pass all parameters and avoid defaults
sarahdayan Jan 25, 2022
c14844c
fix: avoid swallowing errors and return promise chain instead
sarahdayan Jan 25, 2022
6038735
fix: apply suggestions from code review
sarahdayan Jan 26, 2022
d6b9d64
build: adjust bundlesize
sarahdayan Jan 26, 2022
5735c23
chore(cancelable): simplify code (#876)
Haroenv Jan 26, 2022
4fc760a
build: adjust bundlesize
sarahdayan Jan 26, 2022
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
112 changes: 98 additions & 14 deletions packages/autocomplete-core/src/__tests__/concurrency.test.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,13 @@
import { noop } from '@algolia/autocomplete-shared';
import userEvent from '@testing-library/user-event';

import { AutocompleteState } from '..';
import { createPlayground, createSource, defer } from '../../../../test/utils';
import {
createPlayground,
createSource,
defer,
runAllMicroTasks,
} from '../../../../test/utils';
import { createAutocomplete } from '../createAutocomplete';

type Item = {
Expand Down Expand Up @@ -31,7 +37,7 @@ describe('concurrency', () => {
userEvent.type(input, 'b');
userEvent.type(input, 'c');

await defer(() => {}, timeout);
await defer(noop, timeout);

let stateHistory: Array<
AutocompleteState<Item>
Expand All @@ -57,7 +63,7 @@ describe('concurrency', () => {

userEvent.type(input, '{backspace}'.repeat(3));

await defer(() => {}, timeout);
await defer(noop, timeout);

stateHistory = onStateChange.mock.calls.flatMap((x) => x[0].state);

Expand Down Expand Up @@ -88,19 +94,44 @@ describe('concurrency', () => {
getSources,
});

userEvent.type(inputElement, 'ab{esc}');
userEvent.type(inputElement, 'ab');

await defer(() => {}, timeout);
// The search request is triggered
expect(onStateChange).toHaveBeenLastCalledWith(
expect.objectContaining({
state: expect.objectContaining({
status: 'loading',
query: 'ab',
}),
})
);

userEvent.type(inputElement, '{esc}');

// The status is immediately set to "idle" and the panel is closed
expect(onStateChange).toHaveBeenLastCalledWith(
expect.objectContaining({
state: expect.objectContaining({
status: 'idle',
isOpen: false,
query: '',
}),
})
);

await defer(noop, timeout);

// Once the request is settled, the state remains unchanged
expect(onStateChange).toHaveBeenLastCalledWith(
expect.objectContaining({
state: expect.objectContaining({
status: 'idle',
isOpen: false,
}),
})
);
expect(getSources).toHaveBeenCalledTimes(2);

expect(getSources).toHaveBeenCalledTimes(3);
sarahdayan marked this conversation as resolved.
Show resolved Hide resolved
});

test('keeps the panel closed on blur', async () => {
Expand All @@ -115,19 +146,46 @@ describe('concurrency', () => {
getSources,
});

userEvent.type(inputElement, 'a{enter}');
userEvent.type(inputElement, 'a');

await runAllMicroTasks();

// The search request is triggered
expect(onStateChange).toHaveBeenLastCalledWith(
expect.objectContaining({
state: expect.objectContaining({
status: 'loading',
query: 'a',
}),
})
);

await defer(() => {}, timeout);
userEvent.type(inputElement, '{enter}');

// The status is immediately set to "idle" and the panel is closed
expect(onStateChange).toHaveBeenLastCalledWith(
expect.objectContaining({
state: expect.objectContaining({
status: 'idle',
isOpen: false,
query: 'a',
}),
})
);

await defer(noop, timeout);

// Once the request is settled, the state remains unchanged
expect(onStateChange).toHaveBeenLastCalledWith(
expect.objectContaining({
state: expect.objectContaining({
status: 'idle',
isOpen: false,
}),
})
);
expect(getSources).toHaveBeenCalledTimes(1);

expect(getSources).toHaveBeenCalledTimes(2);
});

test('keeps the panel closed on touchstart blur', async () => {
Expand Down Expand Up @@ -156,19 +214,45 @@ describe('concurrency', () => {
window.addEventListener('touchstart', onTouchStart);

userEvent.type(inputElement, 'a');

await runAllMicroTasks();

// The search request is triggered
expect(onStateChange).toHaveBeenLastCalledWith(
expect.objectContaining({
state: expect.objectContaining({
status: 'loading',
query: 'a',
}),
})
);

const customEvent = new CustomEvent('touchstart', { bubbles: true });
window.document.dispatchEvent(customEvent);

await defer(() => {}, timeout);

// The status is immediately set to "idle" and the panel is closed
expect(onStateChange).toHaveBeenLastCalledWith(
expect.objectContaining({
state: expect.objectContaining({
status: 'idle',
isOpen: false,
query: 'a',
}),
})
);

await defer(noop, timeout);

// Once the request is settled, the state remains unchanged
expect(onStateChange).toHaveBeenLastCalledWith(
expect.objectContaining({
state: expect.objectContaining({
status: 'idle',
isOpen: false,
}),
})
);

expect(getSources).toHaveBeenCalledTimes(1);

window.removeEventListener('touchstart', onTouchStart);
Expand Down Expand Up @@ -197,7 +281,7 @@ describe('concurrency', () => {

userEvent.type(inputElement, 'a{esc}');

await defer(() => {}, delay);
await defer(noop, delay);

expect(onStateChange).toHaveBeenLastCalledWith(
expect.objectContaining({
Expand Down Expand Up @@ -229,7 +313,7 @@ describe('concurrency', () => {

userEvent.type(inputElement, 'a{enter}');

await defer(() => {}, delay);
await defer(noop, delay);

expect(onStateChange).toHaveBeenLastCalledWith(
expect.objectContaining({
Expand Down Expand Up @@ -276,7 +360,7 @@ describe('concurrency', () => {
const customEvent = new CustomEvent('touchstart', { bubbles: true });
window.document.dispatchEvent(customEvent);

await defer(() => {}, delay);
await defer(noop, delay);

expect(onStateChange).toHaveBeenLastCalledWith(
expect.objectContaining({
Expand Down
99 changes: 99 additions & 0 deletions packages/autocomplete-core/src/__tests__/debouncing.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,99 @@
import { noop } from '@algolia/autocomplete-shared';
import userEvent from '@testing-library/user-event';

import { createAutocomplete, InternalAutocompleteSource } from '..';
import { createPlayground, createSource, defer } from '../../../../test/utils';

type Source = InternalAutocompleteSource<{ label: string }>;

const delay = 10;

const debounced = debouncePromise<Source[][], Source[]>(
(items) => Promise.resolve(items),
delay
);

describe('debouncing', () => {
test('only submits the final query', async () => {
const onStateChange = jest.fn();
const getItems = jest.fn(({ query }) => [{ label: query }]);
const { inputElement } = createPlayground(createAutocomplete, {
onStateChange,
getSources: () => debounced([createSource({ getItems })]),
});

userEvent.type(inputElement, 'abc');

await defer(noop, delay);

expect(getItems).toHaveBeenCalledTimes(1);
expect(onStateChange).toHaveBeenLastCalledWith(
expect.objectContaining({
state: expect.objectContaining({
status: 'idle',
isOpen: true,
collections: expect.arrayContaining([
expect.objectContaining({
items: [{ __autocomplete_id: 0, label: 'abc' }],
}),
]),
}),
})
);
});

test('triggers subsequent queries after reopening the panel', async () => {
const onStateChange = jest.fn();
const getItems = jest.fn(({ query }) => [{ label: query }]);
const { inputElement } = createPlayground(createAutocomplete, {
onStateChange,
getSources: () => debounced([createSource({ getItems })]),
});

userEvent.type(inputElement, 'abc{esc}');

expect(onStateChange).toHaveBeenLastCalledWith(
expect.objectContaining({
state: expect.objectContaining({
status: 'idle',
isOpen: false,
}),
})
);

userEvent.type(inputElement, 'def');

await defer(noop, delay);

expect(onStateChange).toHaveBeenLastCalledWith(
expect.objectContaining({
state: expect.objectContaining({
collections: expect.arrayContaining([
expect.objectContaining({
items: [{ __autocomplete_id: 0, label: 'abcdef' }],
}),
]),
status: 'idle',
isOpen: true,
}),
})
);
});
});

function debouncePromise<TParams extends unknown[], TResponse>(
fn: (...params: TParams) => Promise<TResponse>,
time: number
) {
let timerId: ReturnType<typeof setTimeout> | undefined = undefined;

return function (...args: TParams) {
if (timerId) {
clearTimeout(timerId);
}

return new Promise<TResponse>((resolve) => {
timerId = setTimeout(() => resolve(fn(...args)), time);
});
};
}
3 changes: 2 additions & 1 deletion packages/autocomplete-core/src/createStore.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import {
InternalAutocompleteOptions,
Reducer,
} from './types';
import { createCancelablePromiseList } from './utils';

type OnStoreStateChange<TItem extends BaseItem> = ({
prevState,
Expand Down Expand Up @@ -35,6 +36,6 @@ export function createStore<TItem extends BaseItem>(

onStoreStateChange({ state, prevState });
},
shouldSkipPendingUpdate: false,
pendingRequests: createCancelablePromiseList(),
};
}
20 changes: 10 additions & 10 deletions packages/autocomplete-core/src/getPropGetters.ts
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,13 @@ export function getPropGetters<
// The `onTouchStart` event shouldn't trigger the `blur` handler when
// it's not an interaction with Autocomplete. We detect it with the
// following heuristics:
// - the panel is closed AND there are no running requests
// - the panel is closed AND there are no pending requests
// (no interaction with the autocomplete, no future state updates)
// - OR the touched target is the input element (should open the panel)
const isNotAutocompleteInteraction =
store.getState().isOpen === false && !onInput.isRunning();
const isAutocompleteInteraction =
store.getState().isOpen || !store.pendingRequests.isEmpty();

if (isNotAutocompleteInteraction || event.target === inputElement) {
if (!isAutocompleteInteraction || event.target === inputElement) {
return;
}

Expand All @@ -62,12 +62,12 @@ export function getPropGetters<
if (isTargetWithinAutocomplete === false) {
store.dispatch('blur', null);

// If requests are still running when the user closes the panel, they
// If requests are still pending when the user closes the panel, they
// could reopen the panel once they resolve.
// We want to prevent any subsequent query from reopening the panel
// because it would result in an unsolicited UI behavior.
if (!props.debug && onInput.isRunning()) {
store.shouldSkipPendingUpdate = true;
if (!props.debug) {
store.pendingRequests.cancelAll();
}
}
},
Expand Down Expand Up @@ -208,12 +208,12 @@ export function getPropGetters<
if (!isTouchDevice) {
store.dispatch('blur', null);

// If requests are still running when the user closes the panel, they
// If requests are still pending when the user closes the panel, they
// could reopen the panel once they resolve.
Haroenv marked this conversation as resolved.
Show resolved Hide resolved
// We want to prevent any subsequent query from reopening the panel
// because it would result in an unsolicited UI behavior.
if (!props.debug && onInput.isRunning()) {
store.shouldSkipPendingUpdate = true;
if (!props.debug) {
store.pendingRequests.cancelAll();
}
}
},
Expand Down
Loading