Skip to content

Commit

Permalink
refactor: update patchProps method to pass all props not only iterabl…
Browse files Browse the repository at this point in the history
…e ones
  • Loading branch information
ftonato committed Jun 14, 2021
1 parent 7d5da11 commit 35108e6
Show file tree
Hide file tree
Showing 2 changed files with 24 additions and 8 deletions.
18 changes: 18 additions & 0 deletions src/__test__/patch.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,24 @@ describe('.patch', () => {
expect(el.title).toEqual('');
});

it('should keep old props and add new ones', () => {
const el = document.createElement('div');
el.id = 'app';

patchProps(<HTMLElement>el, { id: 'app' }, { title: 'bar', id: 'app' });

expect(el.id).toEqual('app');
expect(el.title).toEqual('bar');

patchProps(<HTMLElement>el, { title: 'bar', id: 'app', lang: 'pt', spellcheck: false }, { title: 'foo', id: '', lang: '', hidden: true});

expect(el.id).toEqual('app'); // keeped
expect(el.lang).toEqual('pt'); // keeped
expect(el.title).toEqual('foo'); // updated
expect(el.hidden).toEqual(true); // setted
expect(el.spellcheck).toBeFalsy(); // removed
});

it('should patch children', () => {
const virtualArrayToDOMNodes = (children: (string | VNode)[]): (HTMLElement | Text)[] =>
children.map((child: string | VNode) => createElement(child));
Expand Down
14 changes: 6 additions & 8 deletions src/patch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,12 @@ import { VElement, VNode, VProps } from './m';
* @param {VProps} newProps - New VNode props
*/
export const patchProps = (el: HTMLElement, oldProps: VProps = {}, newProps: VProps = {}): void => {
if (!oldProps && !newProps) return;

const oldPropKeys = Object.keys(oldProps);
const newPropKeys = Object.keys(newProps);

if ((newProps && !oldProps) || oldPropKeys.length > newPropKeys.length) {
// Deletion has occured
for (const propName of oldPropKeys) {
[...oldPropKeys].forEach((propName) => {
const newPropValue = newProps[propName];
/* istanbul ignore if */
if (newPropValue) {
Expand All @@ -25,17 +23,17 @@ export const patchProps = (el: HTMLElement, oldProps: VProps = {}, newProps: VPr
}
delete el[propName];
el.removeAttribute(propName);
}
})
} else {
// Addition/No change/Content modification has occured
for (const propName of newPropKeys) {
[...newPropKeys].forEach((propName) => {
const oldPropValue = oldProps[propName];
if (oldPropValue) {
if (oldPropValue !== oldProps[propName]) el[propName] = newProps[propName];
if (oldPropValue && !newProps[propName]) {
el[propName] = oldPropValue;
return;
}
el[propName] = newProps[propName];
}
})
}
};

Expand Down

2 comments on commit 35108e6

@aidenybai
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are some performance issues with this commit

@ftonato
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @aidenybai,

Definitely forEach is worse than for...of but they have different purposes, my suggestion is to replace forEach by for, what do you thing about it?

image

Please sign in to comment.