Skip to content

Commit

Permalink
fix(js): support updating Element options
Browse files Browse the repository at this point in the history
Before this fix, a call to update() like this:

  update({panelContainer: document.createElement('div')})

would throw the following error:

  TypeError: panelContainer.contains is not a function

The reason lies in an incorrect object deep merge implementation where
object entry values of Element type are merged like plain objects
instead of being replaced, resulting in broken non-Element entries.

This change fixes the issue with HTML Elements (Nodes of any kind to be
more precise) but it might be worth considering replacing the current
mergeDeep implementation with something more robust and future proof.
Deep merging of objects surprisingly hard to get right, see the
implementation in Lodash for example:
https://github.com/lodash/lodash/blob/4.17.15/lodash.js#L3633
  • Loading branch information
salomvary committed Oct 20, 2021
1 parent d6d6ba9 commit 285b3e8
Show file tree
Hide file tree
Showing 3 changed files with 124 additions and 2 deletions.
73 changes: 73 additions & 0 deletions packages/autocomplete-js/src/__tests__/api.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -259,6 +259,79 @@ describe('api', () => {
);
});

test('correctly merges DOM Element options', async () => {
let inputElement: HTMLInputElement;
const onStateChange = jest.fn();
const container = document.createElement('div');
document.body.appendChild(container);
const panelContainer = document.createElement('div');
document.body.appendChild(panelContainer);
const panelContainer2 = document.createElement('div');
document.body.appendChild(panelContainer2);

const { update } = autocomplete<{ label: string }>({
container,
onStateChange,
panelContainer,
shouldPanelOpen: () => true,
openOnFocus: true,
getSources() {
return [
{
sourceId: 'testSource',
getItems() {
return [];
},
templates: {
item({ item }) {
return item.label;
},
noResults() {
return 'No results template';
},
},
},
];
},
});

// Focusing the input should render the panel
inputElement = container.querySelector('.aa-Input');
inputElement.focus();

// Focusing the input should open the panel
expect(onStateChange).toHaveBeenLastCalledWith(
expect.objectContaining({
state: expect.objectContaining({
isOpen: true,
}),
})
);

// Panel is rendered into the original container
await waitFor(() => {
expect(
panelContainer.querySelector<HTMLElement>('.aa-Panel')
).toHaveTextContent('No results template');
});

// Update options (set `panelContainer` to a different element)
update({
panelContainer: panelContainer2,
});

// Focusing the input should render the panel
inputElement = container.querySelector('.aa-Input');
inputElement.focus();

// Panel is rendered into the new container
await waitFor(() => {
expect(
panelContainer2.querySelector<HTMLElement>('.aa-Panel')
).toHaveTextContent('No results template');
});
});

test('overrides the default id', async () => {
const container = document.createElement('div');

Expand Down
46 changes: 46 additions & 0 deletions packages/autocomplete-js/src/utils/__tests__/mergeDeep.test.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
import { mergeDeep } from '../mergeDeep';

describe('mergeDeep', () => {
test('arrays', () => {
expect(mergeDeep({ key: ['test1'] }, { key: ['test2'] })).toEqual({
key: ['test1', 'test2'],
});
});

test('plain objects', () => {
expect(
mergeDeep({ key: { test1: 'value1' } }, { key: { test2: 'value2' } })
).toEqual({
key: { test1: 'value1', test2: 'value2' },
});
});

test('HTML Elements', () => {
expect(
mergeDeep(
{ key: document.createElement('div') },
{ key: document.createElement('span') }
)
).toEqual({
key: document.createElement('span'),
});
});

test('primitives', () => {
expect(mergeDeep({ key: 1 }, { key: 2 })).toEqual({
key: 2,
});
});

test('null', () => {
expect(mergeDeep({ key: 1 }, { key: null })).toEqual({
key: null,
});
});

test('undefined', () => {
expect(mergeDeep({ key: 1 }, { key: undefined })).toEqual({
key: undefined,
});
});
});
7 changes: 5 additions & 2 deletions packages/autocomplete-js/src/utils/mergeDeep.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,7 @@
const isObject = (value: unknown) => value && typeof value === 'object';
const isPlainObject = (value: unknown) =>
value &&
typeof value === 'object' &&
Object.prototype.toString.call(value) === '[object Object]';

export function mergeDeep(...values: any[]) {
return values.reduce((acc, current) => {
Expand All @@ -8,7 +11,7 @@ export function mergeDeep(...values: any[]) {

if (Array.isArray(accValue) && Array.isArray(currentValue)) {
acc[key] = accValue.concat(...currentValue);
} else if (isObject(accValue) && isObject(currentValue)) {
} else if (isPlainObject(accValue) && isPlainObject(currentValue)) {
acc[key] = mergeDeep(accValue, currentValue);
} else {
acc[key] = currentValue;
Expand Down

0 comments on commit 285b3e8

Please sign in to comment.