Skip to content

Commit

Permalink
performance optimization and support improvements
Browse files Browse the repository at this point in the history
- refactor form attributes update routines to get rid of
`cloneDeep` calls: deep cloning is unefficient since it
potentially most of cloned data should not be cloned. instead,
implement `update` and `updated` helper functions that carefully
clone only path-related collections.
- refactor code to depend only on 'lodash.get' and 'lodash.set'
packages
- extend form from `Component` if `PureComponent` is unavailable.
this allows to support older versions of react
- corresponding package.json updates
- minor update README update
  • Loading branch information
akuzko committed Mar 20, 2017
1 parent 9a26b4f commit 2cefad8
Show file tree
Hide file tree
Showing 6 changed files with 86 additions and 39 deletions.
2 changes: 1 addition & 1 deletion .eslintrc
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,6 @@
"react/jsx-equals-spacing": [1, "never"],
"react/jsx-indent-props": [1, 2],
"react/jsx-indent": [1, 2],
"react/jsx-no-duplicate-props": 2,
"react/jsx-no-duplicate-props": 2
}
}
3 changes: 2 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,8 @@ Details on how to run it locally are at the end of README.
Most of forms developers deal with are quite complicated and encapsulate
vast amount of validation and rendering logic. After some basic setup described
in the [Wiki](https://github.com/akuzko/react-form-base/wiki) your form may
look like following:
look like following (please note that `$render` is a helper method and it is
possible to use classic `render` method with a slightly more verbose result):

```js
class UserForm extends Form {
Expand Down
7 changes: 4 additions & 3 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,11 @@
"rimraf": "^2.5.4"
},
"dependencies": {
"lodash": "^4.16.4"
"lodash.get": "^4.4.2",
"lodash.set": "^4.3.2"
},
"peerDependencies": {
"react": ">= 15.0.0",
"react-dom": ">= 15.0.0"
"react": "^0.14.8 || >=15.0.0",
"react-dom": "^0.14.8 || >=15.0.0"
}
}
37 changes: 15 additions & 22 deletions src/Form.jsx
Original file line number Diff line number Diff line change
@@ -1,13 +1,9 @@
import React, { PropTypes, PureComponent } from 'react';
import { PropTypes, PureComponent, Component } from 'react';

import { nameToPath, buildFormValidator } from './utils';
import isPlainObject from 'lodash/isPlainObject';
import cloneDeep from 'lodash/cloneDeep';
import get from 'lodash/get';
import set from 'lodash/set';
import noop from 'lodash/noop';
import { update, buildFormValidator, noop } from './utils';
import get from 'lodash.get';

export default class Form extends PureComponent {
export default class Form extends (PureComponent || Component) {
static propTypes = {
attrs: PropTypes.object.isRequired,
onChange: PropTypes.func,
Expand Down Expand Up @@ -76,27 +72,27 @@ export default class Form extends PureComponent {
get(name) {
if (name === undefined) return this.props.attrs;

return get(this.props.attrs, nameToPath(name));
return get(this.props.attrs, name.split('.'));
}

set(name, value) {
if (isPlainObject(name)) return this._setObject(name);
if (name && (typeof name === 'object') && (name.constructor === Object)) return this._setObject(name);

return this._setAttr(name, value);
}

_setObject(obj) {
return this._set((attrs, errors) => {
for (const name in obj) {
set(attrs, nameToPath(name), obj[name]);
update(attrs, name, obj[name], false);
this._updateErrors(errors, name, obj[name]);
}
});
}

_setAttr(name, value) {
return this._set((attrs, errors) => {
set(attrs, nameToPath(name), value);
update(attrs, name, value, false);
this._updateErrors(errors, name, value);
});
}
Expand All @@ -113,13 +109,12 @@ export default class Form extends PureComponent {

_set(updater) {
const { attrs, onChange } = this.props;
const newAttrs = cloneDeep(attrs);
const newErrors = { ...this.getErrors() };
const nextAttrs = { ...attrs };
const nextErrors = { ...this.getErrors() };
updater(nextAttrs, nextErrors);

updater(newAttrs, newErrors);

this._nextErrors = newErrors;
onChange(newAttrs);
this._nextErrors = nextErrors;
onChange(nextAttrs);
}

_shouldClearError(name) {
Expand Down Expand Up @@ -165,10 +160,8 @@ export default class Form extends PureComponent {

merge(name, value) {
const current = this.get(name) || {};
const attrs = cloneDeep(this.props.attrs);

set(attrs, name, { ...current, ...value });
this.props.onChange(attrs);
return this.set(name, { ...current, ...value });
}

push(name, value) {
Expand All @@ -181,7 +174,7 @@ export default class Form extends PureComponent {
const ary = this.get(name);

return this._set((attrs, errors) => {
set(attrs, nameToPath(name), [...ary.slice(0, i), ...ary.slice(i + 1)]);
update(attrs, name, [...ary.slice(0, i), ...ary.slice(i + 1)]);
this._updateErrors(errors, `${name}.${i}`, null);
});
}
Expand Down
40 changes: 32 additions & 8 deletions src/utils.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import isArray from 'lodash/isArray';
import isPlainObject from 'lodash/isPlainObject';
import set from 'lodash.set';

export function noop(){}

export function bindState(component, key = 'form') {
return {
Expand All @@ -8,8 +9,31 @@ export function bindState(component, key = 'form') {
};
}

export function nameToPath(name) {
return name.replace(/\.(\d+)(\.)?/g, (_match, i, dot) => `[${i}]` + (dot ? '.' : ''));
export function update(obj, name, value) {
_update(obj, name, value);

return obj;
}

export function updated(obj, name, value) {
const current = { ...obj };

return update(current, name, value);
}

function _update(current, name, value) {
const match = name.match(/^([\w\d]+)\.?(.+)?$/);
const { 1: key, 2: rest } = match;

if (current[key] === undefined) {
return set(current, name.split('.'), value);
}
if (!rest) {
return current[key] = value;
}

current[key] = Array.isArray(current[key]) ? [...current[key]] : { ...current[key] };
_update(current[key], rest, value);
}

function wildcard(name) {
Expand All @@ -32,10 +56,7 @@ export function buildFormValidator(form) {
return null;

function callValidator(validator) {
if (isPlainObject(validator)) {
return callObjectValidator(validator);
}
if (isArray(validator)) {
if (Array.isArray(validator)) {
return callArrayValidator(validator);
}
if (typeof validator === 'string') {
Expand All @@ -44,6 +65,9 @@ export function buildFormValidator(form) {
if (typeof validator === 'function') {
return validator.call(form, value);
}
if (validator && (typeof validator === 'object')) {
return callObjectValidator(validator);
}
throw new Error(`unable to use '${validator}' as validator function`);
}

Expand Down
36 changes: 32 additions & 4 deletions test/utils.test.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,40 @@
import React, { Component } from 'react';
import Form from '../src/Form';
import { nameToPath, bindState } from '../src/utils';
import { bindState, updated } from '../src/utils';
import { shallow } from 'enzyme';
import expect from 'expect';

describe('nameToPath', function() {
it('generates a lodash path based on passed name', function() {
expect(nameToPath('foo.1.bar.baz')).toEqual('foo[1].bar.baz');
describe('updated', function() {
it('carefully sets deeply nested item: deeply nested array', function() {
const obj = { foo: { bar: { baz: [1, 2, 3] } }, bak: { big: 1 } };
const upd = updated(obj, 'foo.bar.baz.1', 4);

expect(obj === upd).toBe(false, 'obj should not be updated in place');
expect(obj.foo === upd.foo).toBe(false, 'obj.foo should not be updated in place');
expect(obj.foo.bar === upd.foo.bar).toBe(false, 'obj.foo.bar should not be updated in place');
expect(obj.foo.bar.baz === upd.foo.bar.baz).toBe(false, 'obj.foo.bar.baz should not be updated in place');
expect(obj.bak === upd.bak).toBe(true, 'obj.bak should not be cloned');
expect(upd.foo.bar.baz).toMatch([1, 4, 3], 'value under desired name should be updated');
});

it('carefully sets deeply nested item: deeply nested object', function() {
const obj = { foo: { bar: [{ baz: 'baz1' }, { baz: 'baz2' }] }, bak: { big: 1 } };
const upd = updated(obj, 'foo.bar.1.baz', 'baz3');

expect(obj === upd).toBe(false, 'obj should not be updated in place');
expect(obj.foo === upd.foo).toBe(false, 'obj.foo should not be updated in place');
expect(obj.foo.bar === upd.foo.bar).toBe(false, 'obj.foo.bar should not be updated in place');
expect(obj.foo.bar[0] === upd.foo.bar[0]).toBe(true, 'obj.foo.bar items should not be cloned');
expect(obj.bak === upd.bak).toBe(true, 'obj.bak should not be cloned');
expect(upd.foo.bar[1]).toMatch({ baz: 'baz3' }, 'value under desired name should be updated');
});

it('carefully sets deeply nested item, path collections are not defined', function() {
const obj = { bak: { big: 1 } };
const upd = updated(obj, 'foo.bar.baz.1', 4);

expect(obj.bak === upd.bak).toBe(true, 'obj.bak should not be cloned');
expect(upd.foo.bar.baz).toMatch([undefined, 4], 'value under desired name should be updated');
});
});

Expand Down

0 comments on commit 2cefad8

Please sign in to comment.