Skip to content
This repository has been archived by the owner on Nov 26, 2018. It is now read-only.

Keep track if a field is pristine or not. #6

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Keep track if a field is pristine or not. #6

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
75ab99b
Keep track if a field is pristine or not.
DJWassink Apr 22, 2017
af009d8
Create initialiseField functions instead of magic boolean in setValue…
DJWassink Apr 23, 2017
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
52 changes: 32 additions & 20 deletions src/business/form.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,44 +1,56 @@
export class Form {

private values: { [key: string]: any } = {};
export interface FieldState {
value: any;
validationError?: string;
pristine: boolean;
}

private validationErrors: { [fieldName: string]: any } = {};
export class Form {

private fieldStates: { [key: string]: FieldState } = {};
private fieldListeners: { [fieldName: string]: Array<(value: any) => void> } = {};

public getFieldValue(fieldName: string): any {
return this.values[fieldName] || '';
public getFieldState(fieldName: string): FieldState | undefined {
return this.fieldStates[fieldName];
}

public setFieldValues(values: { [fieldName: string]: any }) {
public setFieldValues(values: { [fieldName: string]: any }, pristineValues = false) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not digging the magic third boolean, why not introduce something like a resetFieldValues method

const fieldNames = Object.keys(values);
fieldNames.forEach((fieldName) => {
this.values[fieldName] = values[fieldName];
delete this.validationErrors[fieldName];
this.updateFieldValue(fieldName, values[fieldName], pristineValues);
});

this.triggerMultipleFieldListeners(fieldNames);
}

public setFieldValue(fieldName: string, value: any) {
this.values[fieldName] = value;
delete this.validationErrors[fieldName];
public setFieldValue(fieldName: string, value: any, pristineValue = false) {
this.updateFieldValue(fieldName, value, pristineValue);

this.triggerFieldListeners(fieldName);
}

private updateFieldValue(fieldName: string, value: any, pristineValue) {
Copy link
Contributor

Choose a reason for hiding this comment

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

implicit any on pristine value

this.fieldStates[fieldName] = {
...this.fieldStates[fieldName],
value,
validationError: undefined,
pristine: pristineValue
};
}

public setValidationErrors(errors: { [fieldName: string]: string }) {
this.validationErrors = errors;
const fieldNames = Object.keys(errors);

const fieldNames = Object.keys(this.validationErrors);
this.triggerMultipleFieldListeners(fieldNames);
}
fieldNames.forEach((fieldName) => {
this.fieldStates[fieldName] = {
...this.fieldStates[fieldName],
validationError: errors[fieldName]
};
});

public getValidationError(fieldName: string): string | undefined {
return this.validationErrors[fieldName];
this.triggerMultipleFieldListeners(fieldNames);
}

public listenForFieldChange(fieldName: string, callback: (value: any) => void) {
public listenForFieldChange(fieldName: string, callback: (value: FieldState) => void) {
if (typeof this.fieldListeners[fieldName] === 'undefined') {
this.fieldListeners[fieldName] = [];
}
Expand All @@ -53,7 +65,7 @@ export class Form {

private triggerFieldListeners(fieldName: string) {
if (typeof this.fieldListeners[fieldName] !== 'undefined') {
this.fieldListeners[fieldName].forEach((callback) => callback(this.getFieldValue(fieldName)));
this.fieldListeners[fieldName].forEach((callback) => callback(this.getFieldState(fieldName)));
}
}

Expand Down
4 changes: 2 additions & 2 deletions src/composers/bonn.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ export function Bonn<Props>(WrappedComponent: IncomingForm<Props>): OutgoingForm
public componentWillMount() {
const values = this.props.values;
if (typeof values !== 'undefined') {
this.form.setFieldValues(values as { [fieldName: string]: any });
this.form.setFieldValues(values as { [fieldName: string]: any }, true);
}
}

Expand All @@ -34,7 +34,7 @@ export function Bonn<Props>(WrappedComponent: IncomingForm<Props>): OutgoingForm
}
}
});
this.form.setFieldValues(updatedValues);
this.form.setFieldValues(updatedValues, true);

}
}
Expand Down
14 changes: 9 additions & 5 deletions src/composers/field.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import * as React from 'react';
import {FormProps} from './bonn';
import {FieldState} from '../business/form';
export interface FieldProps {
value: any;
validationError: string | undefined;
Expand All @@ -20,7 +21,9 @@ export function Field<Props>(WrappedComponent: IncomingField<Props>,
return class extends React.Component<Props & OwnProps & FormProps, { value: any }> {

public state: { value: any } = {
value: this.props.form.getFieldValue(this.getFieldName())
value: (typeof this.props.form.getFieldState(this.getFieldName()) !== 'undefined')
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be better to put this in a seperate method, unreadable like this, if you do that you can extract the getFieldState result to a seperate variable and remove the need for a second call to the method

? (this.props.form.getFieldState(this.getFieldName()) as FieldState).value
: ''
};

public getFieldName(): string {
Expand All @@ -36,13 +39,13 @@ export function Field<Props>(WrappedComponent: IncomingField<Props>,

public componentWillMount() {
if (typeof this.props.value !== 'undefined') {
this.props.form.setFieldValue(this.getFieldName(), this.props.value);
this.props.form.setFieldValue(this.getFieldName(), this.props.value, true);
}
}

public componentWillUpdate(nextProps: Props & OwnProps & FormProps) {
if (typeof nextProps.value !== 'undefined' && this.props.value !== nextProps.value) {
this.props.form.setFieldValue(this.getFieldName(), nextProps.value);
this.props.form.setFieldValue(this.getFieldName(), nextProps.value, true);
}
}

Expand All @@ -55,14 +58,15 @@ export function Field<Props>(WrappedComponent: IncomingField<Props>,
}

private handleChange(value: any) {
this.props.form.setFieldValue(this.getFieldName(), value);
this.props.form.setFieldValue(this.getFieldName(), value, false);
}

public render() {
const fieldState = this.props.form.getFieldState(this.getFieldName());
Copy link
Contributor

Choose a reason for hiding this comment

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

Why are we storing a copy of the value if we're still going to call getFieldState in the render method?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good question. This is following the initial design we had and maybe should change. The getFieldState gets called in favor of the getValidationError we had before. Do we wan't to store the validationError localy also?

Copy link
Contributor

Choose a reason for hiding this comment

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

This might be the textbook use-case for forceUpdate, since our state is in another place.. so we can just remove local state and call the getFieldState in the render function https://facebook.github.io/react/docs/react-component.html#forceupdate

Copy link
Member Author

Choose a reason for hiding this comment

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

So we would push the local state to the bonn form, so instead of setting the local state in the keyPressHandler we push it to bonn and let it forceUpdate any listening components? Sounds a bit tricky tho. Gotta be carefull to not render too much (e.g. having mutliple things inside a field would already force multiple renders). But I guess that would also mean a user mis-used the field decorator.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ehm, this is already what we're doing.. I just say we remove the state from the Field and Listener components and introduce a this.forceUpdate() within the listenForFieldChange callback. Then in the render we just fetch the values we need.

return <WrappedComponent
{...this.props}
value={this.state.value}
validationError={this.props.form.getValidationError(this.getFieldName())}
validationError={(typeof fieldState !== 'undefined') ? fieldState.validationError : undefined}
onChange={this.handleChange.bind(this)}
/>;
}
Expand Down
5 changes: 4 additions & 1 deletion src/composers/listener.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,10 @@ export function Listener<Props>(WrappedComponent: IncomingListener<Props>, field
public getValues(): any {
const result: any = {};
fieldNames.forEach(fieldName => {
result[fieldName] = this.props.form.getFieldValue(fieldName);
const fieldState = this.props.form.getFieldState(fieldName);
if (typeof fieldState !== 'undefined') {
result[fieldName] = fieldState.value;
}
});

return result;
Expand Down
92 changes: 79 additions & 13 deletions tests/bonn.test.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,8 +42,8 @@ describe('Bonn', function () {
mount(<Component values={values}/>);

/* Then */
expect(passedForm.getFieldValue('foo')).toBe('bar');
expect(passedForm.getFieldValue('blub')).toBe('blab');
expect(passedForm.getFieldState('foo').value).toBe('bar');
expect(passedForm.getFieldState('blub').value).toBe('blab');
});

it('should propagate changed initial values', function () {
Expand Down Expand Up @@ -71,8 +71,8 @@ describe('Bonn', function () {
});

/* Then */
expect(passedForm.getFieldValue('foo')).toBe('bar');
expect(passedForm.getFieldValue('blub')).toBe('blob');
expect(passedForm.getFieldState('foo').value).toBe('bar');
expect(passedForm.getFieldState('blub').value).toBe('blob');
});

it('should handle newly added initial value', function () {
Expand Down Expand Up @@ -101,9 +101,9 @@ describe('Bonn', function () {
});

/* Then */
expect(passedForm.getFieldValue('foo')).toBe('bar');
expect(passedForm.getFieldValue('blub')).toBe('blab');
expect(passedForm.getFieldValue('woo')).toBe('waa');
expect(passedForm.getFieldState('foo').value).toBe('bar');
expect(passedForm.getFieldState('blub').value).toBe('blab');
expect(passedForm.getFieldState('woo').value).toBe('waa');
});

it('should only trigger changed listeners of changed fields while propagating updated initial value', function () {
Expand Down Expand Up @@ -170,7 +170,7 @@ describe('Bonn', function () {
});

/* Then */
expect(form.getFieldValue('field')).toBe('Hello');
expect(form.getFieldState('field').value).toBe('Hello');
});

it('should use initial value', function () {
Expand All @@ -190,7 +190,7 @@ describe('Bonn', function () {
mount(<Component form={form} value="Hello" name="field"/>);

/* Then */
expect(form.getFieldValue('field')).toBe('Hello');
expect(form.getFieldState('field').value).toBe('Hello');
});

it('should update value if value in props changes', function () {
Expand All @@ -213,7 +213,7 @@ describe('Bonn', function () {
});

/* Then */
expect(form.getFieldValue('field')).toBe('Hi');
expect(form.getFieldState('field').value).toBe('Hi');
});

it('should pass use given name from props', function () {
Expand All @@ -238,7 +238,7 @@ describe('Bonn', function () {
});

/* Then */
expect(form.getFieldValue('field')).toBe('Hello');
expect(form.getFieldState('field').value).toBe('Hello');
});

it('should receive validation error', function () {
Expand Down Expand Up @@ -288,7 +288,73 @@ describe('Bonn', function () {

/* Then */
expect(result.text()).not.toContain('Foutje');
})
});

it('should be a pristine field when the field is untouched', function () {
/* Given */
const form = new Form();

class MyField extends React.Component<FieldProps, {}> {
render() {
return <div>
<input name="field"/>
</div>;
}
}
const Component = Field<{}>(MyField);

/* When*/
const result = mount(<Component name="field" value="klaas" form={form}/>);

expect(form.getFieldState('field').value).toBe('klaas');
expect(form.getFieldState('field').pristine).toBe(true);
});

it('should not be pristine if the field receives a new value', function () {
/* Given */
const form = new Form();

class MyField extends React.Component<FieldProps, {}> {
render() {
return <div>
<input name="field"/>
</div>;
}
}
const Component = Field<{}>(MyField);

/* When*/
const result = mount(<Component name="field" value="klaas" form={form}/>);
form.setFieldValue('field', 'blaat');

expect(form.getFieldState('field').value).toBe('blaat');
expect(form.getFieldState('field').pristine).toBe(false);
});

it('should not be pristine again if the prop value changed', function () {
/* Given */
const form = new Form();

class MyField extends React.Component<FieldProps, {}> {
render() {
return <div>
<input name="field"/>
</div>;
}
}
const Component = Field<{}>(MyField);

/* When*/
const result = mount(<Component name="field" value="klaas" form={form}/>);
form.setFieldValue('field', 'blaat');

result.setProps({
'value': 'piet'
});

expect(form.getFieldState('field').value).toBe('piet');
expect(form.getFieldState('field').pristine).toBe(true);
});

});

Expand Down Expand Up @@ -317,7 +383,7 @@ describe('Bonn', function () {
form.setFieldValue('field', 'blub');

/* Then */
expect(lastFieldValue).toBe('blub');
expect(lastFieldValue.value).toBe('blub');
});

it('should not trigger updates when not component is not listening to certain field', function () {
Expand Down