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

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

Open
DJWassink 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 all commits
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
61 changes: 44 additions & 17 deletions src/business/form.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,44 +1,71 @@
export interface FieldState {
value: any;
validationError?: string;
pristine: boolean;
}

export class Form {

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

private validationErrors: { [fieldName: string]: any } = {};
public getFieldState(fieldName: string): FieldState | undefined {
return this.fieldStates[fieldName];
}

private fieldListeners: { [fieldName: string]: Array<(value: any) => void> } = {};
public initialiseFieldValues(values: { [fieldName: string]: any }) {
const fieldNames = Object.keys(values);
fieldNames.forEach((fieldName) => {
this.updateFieldValue(fieldName, values[fieldName], true);
});

public getFieldValue(fieldName: string): any {
return this.values[fieldName] || '';
this.triggerMultipleFieldListeners(fieldNames);
}

public setFieldValues(values: { [fieldName: string]: any }) {
const fieldNames = Object.keys(values);
fieldNames.forEach((fieldName) => {
this.values[fieldName] = values[fieldName];
delete this.validationErrors[fieldName];
this.updateFieldValue(fieldName, values[fieldName], false);
});

this.triggerMultipleFieldListeners(fieldNames);
}

public initialiseFieldValue(fieldName: string, value: any) {
Copy link
Copy Markdown
Contributor

@JBlaak JBlaak Apr 24, 2017

Choose a reason for hiding this comment

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

initialize?

Copy link
Copy Markdown
Author

@DJWassink DJWassink Apr 24, 2017

Choose a reason for hiding this comment

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

uk vs us? which do we choose? initialize is US definition with a s is UK

Copy link
Copy Markdown
Contributor

@JBlaak JBlaak Apr 24, 2017

Choose a reason for hiding this comment

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

Ah true, I think we should opt for US, I'm not so much of a tea drinker

Copy link
Copy Markdown
Author

@DJWassink DJWassink Apr 24, 2017

Choose a reason for hiding this comment

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

Oh ya bloody imbecile

this.updateFieldValue(fieldName, value, true);

this.triggerFieldListeners(fieldName);
}

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

this.triggerFieldListeners(fieldName);
}

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

@JBlaak JBlaak Apr 24, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I'm expecting the value to be named pristineValue containing the value, do you mean isPristineValue?

Copy link
Copy Markdown
Author

@DJWassink DJWassink Apr 24, 2017

Choose a reason for hiding this comment

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

agree

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 +80,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.initialiseFieldValues(values as { [fieldName: string]: any });
}
}

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

}
}
Expand Down
12 changes: 8 additions & 4 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
Copy Markdown
Contributor

@JBlaak JBlaak Apr 24, 2017

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.initialiseFieldValue(this.getFieldName(), this.props.value);
}
}

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.initialiseFieldValue(this.getFieldName(), nextProps.value);
}
}

Expand All @@ -59,10 +62,11 @@ export function Field<Props>(WrappedComponent: IncomingField<Props>,
}

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

@JBlaak JBlaak Apr 24, 2017

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
Copy Markdown
Author

@DJWassink DJWassink Apr 24, 2017

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
Copy Markdown
Contributor

@JBlaak JBlaak Apr 24, 2017

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
Copy Markdown
Author

@DJWassink DJWassink Apr 24, 2017

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
Copy Markdown
Contributor

@JBlaak JBlaak Apr 25, 2017

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