Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Form - Migrate setNativeProps to state. #11039

Merged
merged 31 commits into from
Oct 7, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
1e1765a
make form controlled by moving inputValues to state
rushatgabhane Sep 16, 2022
58505b4
Make Picker controlled only and remove setNativeProps
rushatgabhane Sep 16, 2022
50373ab
make picker work with Form
rushatgabhane Sep 16, 2022
a7f4f88
remove defaultValue prop from Picker
rushatgabhane Sep 16, 2022
3fb74b2
fix eslint error
rushatgabhane Sep 16, 2022
d3e630b
Merge branch 'Expensify:main' into rm-setNativeProps-form
rushatgabhane Sep 18, 2022
68c1389
Merge branch 'Expensify:main' into rm-setNativeProps-form
rushatgabhane Oct 4, 2022
c5ce2d3
remove default value
rushatgabhane Oct 4, 2022
c8f4299
Revert "remove default value"
rushatgabhane Oct 4, 2022
55bcc8f
use default value for pickers
rushatgabhane Oct 4, 2022
951be8f
use props.value in Form
rushatgabhane Oct 4, 2022
44b8a95
remove console log
rushatgabhane Oct 4, 2022
2cf3288
allow form consumer to get the value changes
rushatgabhane Oct 5, 2022
92dc75d
fix form stories
rushatgabhane Oct 5, 2022
f505376
add onInputChange to picker stories
rushatgabhane Oct 5, 2022
f19213c
eslint
rushatgabhane Oct 5, 2022
addf6fc
eslint
rushatgabhane Oct 5, 2022
c7bef85
rename custom callback onInputChange to onValueChange
rushatgabhane Oct 5, 2022
3b01ab9
cleanup hack
rushatgabhane Oct 5, 2022
150f736
fix lint errors
rushatgabhane Oct 5, 2022
28f5db3
Merge branch 'main' into rm-setNativeProps-form
rushatgabhane Oct 5, 2022
426e24f
refactor: don't pass inputID
rushatgabhane Oct 5, 2022
b1a6d98
refactor: move callback to named function
rushatgabhane Oct 5, 2022
d8e66e8
fix: eslint errors
rushatgabhane Oct 5, 2022
41d0ebd
docs: remove defaultValue, and add value
rushatgabhane Oct 5, 2022
24bd378
docs: add all props available for form inputs
rushatgabhane Oct 5, 2022
ff83af2
fix typo
rushatgabhane Oct 6, 2022
ddc4050
fix: pickers on company info page
rushatgabhane Oct 6, 2022
1273efc
Merge branch 'rm-setNativeProps-form' of github.com:rushatgabhane/Exp…
rushatgabhane Oct 6, 2022
aab173b
fix: picker closing before clicking done on ios
rushatgabhane Oct 6, 2022
c72e7ac
Merge branch 'Expensify:main' into rm-setNativeProps-form
rushatgabhane Oct 7, 2022
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
6 changes: 5 additions & 1 deletion contributingGuides/FORMS.md
Original file line number Diff line number Diff line change
Expand Up @@ -203,11 +203,15 @@ function onSubmit(values) {
The following prop is available to form inputs:

- inputID: An unique identifier for the input.
- shouldSaveDraft: Saves a draft of the input value.
- defaultValue: The initial value of the input.
- value: The value to show for the input.
- onValueChange: A callback that is called when the input's value changes.

Form.js will automatically provide the following props to any input with the inputID prop.

- ref: A React ref that must be attached to the input.
- defaultValue: The input default value.
- value: The input value.
- errorText: The translated error text that is returned by validate for that specific input.
- onBlur: An onBlur handler that calls validate.
- onInputChange: An onChange handler that saves draft values and calls validate for that input (inputA). Passing an inputID as a second param allows inputA to manipulate the input value of the provided inputID (inputB).
34 changes: 21 additions & 13 deletions src/components/Form.js
Original file line number Diff line number Diff line change
Expand Up @@ -66,10 +66,10 @@ class Form extends React.Component {

this.state = {
errors: {},
inputValues: {},
};

this.inputRefs = {};
this.inputValues = {};
this.touchedInputs = {};

this.setTouchedInput = this.setTouchedInput.bind(this);
Expand Down Expand Up @@ -101,12 +101,12 @@ class Form extends React.Component {
));

// Validate form and return early if any errors are found
if (!_.isEmpty(this.validate(this.inputValues))) {
if (!_.isEmpty(this.validate(this.state.inputValues))) {
return;
}

// Call submit handler
this.props.onSubmit(this.inputValues);
this.props.onSubmit(this.state.inputValues);
}

/**
Expand Down Expand Up @@ -159,30 +159,38 @@ class Form extends React.Component {
const defaultValue = this.props.draftValues[inputID] || child.props.defaultValue;

// We want to initialize the input value if it's undefined
if (_.isUndefined(this.inputValues[inputID])) {
this.inputValues[inputID] = defaultValue;
if (_.isUndefined(this.state.inputValues[inputID])) {
this.state.inputValues[inputID] = defaultValue;
}

if (!_.isUndefined(child.props.value)) {
this.state.inputValues[inputID] = child.props.value;
Comment on lines +162 to +167
Copy link
Contributor

Choose a reason for hiding this comment

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

I just bumped into this code and noticed that we are mutating the state during rendering. I haven't seen any bug caused by this, but, as far as I know, a base assumption when working in React that the state won't mutate like this, otherwise, basic checks equality checks by reference become not reliable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any strong reason to mutate the state in this case?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think you're right that this shouldn't be done. I think the state will change, but we won't re-render. AFAIK we should always be using setState cc @rushatgabhane

Copy link
Contributor

@aldo-expensify aldo-expensify Nov 28, 2022

Choose a reason for hiding this comment

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

It seems like this can be a technique to exactly do that, avoid re-rendering. In some cases it makes sense to avoid re-rendering.. but there are other ways of achieving this without having to mutate the state. There was a similar conversation here about a proposal suggesting to do this: #11086 (comment)

Summary:

Copy link
Member Author

Choose a reason for hiding this comment

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

Is there any strong reason to mutate the state in this case?

Great question. I was aware that it isn't recommended to mutate state but chose to mutate because -

App/src/components/Form.js

Lines 166 to 167 in c72e7ac

if (!_.isUndefined(child.props.value)) {
this.state.inputValues[inputID] = child.props.value;

When value is passed as a prop, the parent component is expected to maintain (init and update) the state and cause a re-render.

If we had used setState, it'll cause an unnecessary re-render.

The form component also needs the correct inputValues to call the validate function.

this.validate(this.state.inputValues);

Copy link
Member Author

Choose a reason for hiding this comment

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

There are times when we need to trigger a re-render. eg: uncontrolled input changes. setState is called for inputValues

App/src/components/Form.js

Lines 178 to 181 in c72e7ac

onInputChange: (value, key) => {
const inputKey = key || inputID;
this.setState(prevState => ({
inputValues: {

Copy link
Member Author

@rushatgabhane rushatgabhane Nov 28, 2022

Choose a reason for hiding this comment

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

To have a "state" you can change without re-rendering, I think the recommended way is:
Class component: member variable

Agree. But why maintain another duplicate variable that we'll need to always keep in sync with inputValues?

Copy link
Contributor

Choose a reason for hiding this comment

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

To have a "state" you can change without re-rendering, I think the recommended way is:
Class component: member variable

Agree. But why maintain another duplicate variable that we'll need to always keep in sync with inputValues?

I haven't really gone in depth in this code, and I don't really plan to at the moment because of priorities, but even in the worse case of having duplicate variables, I still think it is better that doing something that is not expected in the framework.

Copy link
Member Author

@rushatgabhane rushatgabhane Nov 28, 2022

Choose a reason for hiding this comment

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

Cool, I'll raise a PR to clean this up (hopefully by end of this week)
This way I can dive deep and see if it causes any bugs.

note to self: deep clone everything

Copy link
Contributor

Choose a reason for hiding this comment

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

Awesome, thanks guys!

}

return React.cloneElement(child, {
ref: node => this.inputRefs[inputID] = node,
defaultValue,
value: this.state.inputValues[inputID],
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
errorText: this.state.errors[inputID] || '',
onBlur: () => {
this.setTouchedInput(inputID);
this.validate(this.inputValues);
this.validate(this.state.inputValues);
},
onInputChange: (value, key) => {
const inputKey = key || inputID;
this.inputValues[inputKey] = value;
const inputRef = this.inputRefs[inputKey];
this.setState(prevState => ({
inputValues: {
...prevState.inputValues,
[inputKey]: value,
},
}), () => this.validate(this.state.inputValues));

if (key && inputRef && _.isFunction(inputRef.setNativeProps)) {
inputRef.setNativeProps({value});
}
if (child.props.shouldSaveDraft) {
FormActions.setDraftValues(this.props.formID, {[inputKey]: value});
}
this.validate(this.inputValues);

if (child.props.onValueChange) {
child.props.onValueChange(value);
luacmartins marked this conversation as resolved.
Show resolved Hide resolved
}
},
});
});
Expand Down
34 changes: 5 additions & 29 deletions src/components/Picker/BasePicker/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -10,26 +10,7 @@ class BasePicker extends React.Component {
constructor(props) {
super(props);

this.pickerValue = this.props.defaultValue;

this.updateSelectedValueAndExecuteOnChange = this.updateSelectedValueAndExecuteOnChange.bind(this);
this.executeOnCloseAndOnBlur = this.executeOnCloseAndOnBlur.bind(this);
this.setNativeProps = this.setNativeProps.bind(this);
}

/**
* This method mimicks RN's setNativeProps method. It's exposed to Picker's ref and can be used by other components
* to directly manipulate Picker's value when Picker is used as an uncontrolled input.
*
* @param {*} value
*/
setNativeProps({value}) {
this.pickerValue = value;
}

updateSelectedValueAndExecuteOnChange(value) {
this.props.onInputChange(value);
this.pickerValue = value;
}

executeOnCloseAndOnBlur() {
Expand All @@ -42,13 +23,12 @@ class BasePicker extends React.Component {
const hasError = !_.isEmpty(this.props.errorText);
return (
<RNPickerSelect
onValueChange={this.updateSelectedValueAndExecuteOnChange}
onValueChange={this.props.onInputChange}
items={this.props.items}
style={this.props.size === 'normal' ? basePickerStyles(this.props.disabled, hasError, this.props.focused) : styles.pickerSmall}
useNativeAndroidPickerStyle={false}
placeholder={this.props.placeholder}
value={this.props.value || this.pickerValue}
key={this.props.value || this.pickerValue}
value={this.props.value}
Icon={() => this.props.icon(this.props.size)}
disabled={this.props.disabled}
fixAndroidTouchableBug
Expand All @@ -58,15 +38,11 @@ class BasePicker extends React.Component {
onFocus: this.props.onOpen,
onBlur: this.executeOnCloseAndOnBlur,
}}
ref={(node) => {
if (!node || !_.isFunction(this.props.innerRef)) {
ref={(el) => {
if (!_.isFunction(this.props.innerRef)) {
return;
}

this.props.innerRef(node);

// eslint-disable-next-line no-param-reassign
node.setNativeProps = this.setNativeProps;
this.props.innerRef(el);
}}
/>
);
Expand Down
21 changes: 21 additions & 0 deletions src/components/Picker/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,9 @@ const propTypes = {

/** Saves a draft of the input value when used in a form */
shouldSaveDraft: PropTypes.bool,

/** A callback method that is called when the value changes and it receives the selected value as an argument */
onInputChange: PropTypes.func.isRequired,
};

const defaultProps = {
Expand All @@ -47,6 +50,23 @@ class Picker extends PureComponent {
this.state = {
isOpen: false,
};

this.onInputChange = this.onInputChange.bind(this);
}

/**
* Forms use inputID to set values. But Picker passes an index as the second parameter to onInputChange
* We are overriding this behavior to make Picker work with Form
* @param {String} value
* @param {Number} index
*/
onInputChange(value, index) {
if (this.props.inputID) {
this.props.onInputChange(value);
return;
}

this.props.onInputChange(value, index);
Copy link
Member Author

@rushatgabhane rushatgabhane Sep 16, 2022

Choose a reason for hiding this comment

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

Question from @parasharrajat - How is this change related to removing setNativeProps?

Rushat:

Anytime a Form input's value is changed, onInputChange() is called.

If you'll look at Form.js

App/src/components/Form.js

Lines 161 to 166 in 97ebb65

onInputChange: (value, key) => {
const inputKey = key || inputID;
this.setState(prevState => ({
inputValues: {
...prevState.inputValues,
[inputKey]: value,

It keeps track of input values based on the key provided to it.
And this "key" is supposed to be the formID used by the Form input.

So our expected function call looks something like this - onInputChange('Portugal', 'countryPicker')


Prerequisite: Picker is used in a Form.

Now RNPickerSelect comes into picture.

It passes the picker item's index as the second parameter. (source)

So this is what actually gets called - onInputChange('Portugal', 156). (where 156 is the index of Portugal in a list)

Our Form based picker now will try to change the value of an input whose formID is 156.
And bam 💥 our Form based picker doesn't work anymore.

I'm just fixing this by passing the formID as the second param, because we don't care aobut the index lol

This wasn't a problem before because Form previously set the value using ref.setNativeProps.
We're getting rid of that now. So we gotta fix it here.

Copy link
Contributor

Choose a reason for hiding this comment

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

The explanation makes sense, but I'm not sure that this is necessary. From the Form docs, the intent of the second param is to:

Passing an inputID as a second param allows inputA to manipulate the input value of the provided inputID (inputB).

If you look at Form, we have const inputKey = key || inputID; so if key (the 2nd param) is not provided, we will automatically use the inputID provided to that Picker. The only instance in which we want to pass a 2nd param is if that picker is also controlling the value for a different input. In which case we would provide a custom callback (not yet supported, see this issue) and pass the inputID of the second input.

Copy link
Contributor

Choose a reason for hiding this comment

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

@rushatgabhane what are your thoughts about my comment above?

Copy link
Member Author

Choose a reason for hiding this comment

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

just started reading this, will get back soon

Copy link
Member Author

Choose a reason for hiding this comment

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

Passing an inputID as a second param allows inputA to manipulate the input value of the provided inputID (inputB).

Thank you, I understand the use of 2nd parameter better now.
I've updated the call to not pass the 2nd parameter.

}

render() {
Expand All @@ -72,6 +92,7 @@ class Picker extends PureComponent {
value={this.props.value}
// eslint-disable-next-line react/jsx-props-no-spreading
{...pickerProps}
onInputChange={this.onInputChange}
/>
</View>
<InlineErrorText styles={[styles.mh3]}>
Expand Down
5 changes: 0 additions & 5 deletions src/components/StatePicker.js
Original file line number Diff line number Diff line change
Expand Up @@ -32,16 +32,12 @@ const propTypes = {
/** Error text to display */
errorText: PropTypes.string,

/** The default value of the state picker */
defaultValue: PropTypes.string,

...withLocalizePropTypes,
};

const defaultProps = {
label: '',
value: undefined,
defaultValue: undefined,
errorText: '',
shouldSaveDraft: false,
inputID: undefined,
Expand All @@ -56,7 +52,6 @@ const StatePicker = forwardRef((props, ref) => (
items={STATES}
onInputChange={props.onInputChange}
value={props.value}
defaultValue={props.defaultValue}
label={props.label || props.translate('common.state')}
errorText={props.errorText}
onBlur={props.onBlur}
Expand Down
4 changes: 2 additions & 2 deletions src/pages/ReimbursementAccount/CompanyStep.js
Original file line number Diff line number Diff line change
Expand Up @@ -256,7 +256,7 @@ class CompanyStep extends React.Component {
label={this.props.translate('companyStep.companyType')}
items={_.map(this.props.translate('companyStep.incorporationTypes'), (label, value) => ({value, label}))}
onInputChange={value => this.clearErrorAndSetValue('incorporationType', value)}
defaultValue={this.state.incorporationType}
value={this.state.incorporationType}
placeholder={{value: '', label: '-'}}
errorText={this.getErrorText('incorporationType')}
/>
Expand All @@ -275,7 +275,7 @@ class CompanyStep extends React.Component {
<StatePicker
label={this.props.translate('companyStep.incorporationState')}
onInputChange={value => this.clearErrorAndSetValue('incorporationState', value)}
defaultValue={this.state.incorporationState}
value={this.state.incorporationState}
errorText={this.getErrorText('incorporationState')}
/>
</View>
Expand Down
44 changes: 35 additions & 9 deletions src/pages/settings/Profile/ProfilePage.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,6 +78,8 @@ class ProfilePage extends Component {
this.getLogins = this.getLogins.bind(this);
this.validate = this.validate.bind(this);
this.updatePersonalDetails = this.updatePersonalDetails.bind(this);
this.setPronouns = this.setPronouns.bind(this);
this.setAutomaticTimezone = this.setAutomaticTimezone.bind(this);
}

componentDidUpdate(prevProps) {
Expand All @@ -96,6 +98,36 @@ class ProfilePage extends Component {
this.setState(stateToUpdate);
}

/**
* @param {String} pronouns
*/
setPronouns(pronouns) {
const hasSelfSelectedPronouns = pronouns === CONST.PRONOUNS.SELF_SELECT;
this.pronouns = hasSelfSelectedPronouns ? '' : pronouns;

if (this.state.hasSelfSelectedPronouns === hasSelfSelectedPronouns) {
return;
}

this.setState({hasSelfSelectedPronouns});
Copy link
Member Author

@rushatgabhane rushatgabhane Oct 6, 2022

Choose a reason for hiding this comment

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

NAB: This function is doing too many things.
But I'm not sure how to clean things up here 😕

}

/**
* Update the timezone picker's value to guessed timezone
* @param {Boolean} isAutomaticTimezone
*/
setAutomaticTimezone(isAutomaticTimezone) {
if (!isAutomaticTimezone) {
this.setState({isAutomaticTimezone});
return;
}

this.setState({
selectedTimezone: moment.tz.guess(),
isAutomaticTimezone,
});
}

/**
* Get the most validated login of each type
*
Expand Down Expand Up @@ -166,14 +198,6 @@ class ProfilePage extends Component {
[values.firstName, values.lastName, values.pronouns],
);

const hasSelfSelectedPronouns = values.pronouns === CONST.PRONOUNS.SELF_SELECT;
this.pronouns = hasSelfSelectedPronouns ? '' : values.pronouns;
this.setState({
hasSelfSelectedPronouns,
isAutomaticTimezone: values.isAutomaticTimezone,
selectedTimezone: values.isAutomaticTimezone ? moment.tz.guess() : values.timezone,
});

if (hasFirstNameError) {
errors.firstName = Localize.translateLocal('personalDetails.error.characterLimit', {limit: CONST.FORM_CHARACTER_LIMIT});
}
Expand Down Expand Up @@ -262,6 +286,7 @@ class ProfilePage extends Component {
label: this.props.translate('profilePage.selectYourPronouns'),
}}
defaultValue={pronounsPickerValue}
onValueChange={this.setPronouns}
/>
{this.state.hasSelfSelectedPronouns && (
<View style={styles.mt2}>
Expand Down Expand Up @@ -291,14 +316,15 @@ class ProfilePage extends Component {
label={this.props.translate('profilePage.timezone')}
items={timezones}
isDisabled={this.state.isAutomaticTimezone}
defaultValue={this.state.selectedTimezone}
value={this.state.selectedTimezone}
onValueChange={selectedTimezone => this.setState({selectedTimezone})}
/>
</View>
<CheckboxWithLabel
inputID="isAutomaticTimezone"
label={this.props.translate('profilePage.setMyTimezoneAutomatically')}
defaultValue={this.state.isAutomaticTimezone}
onValueChange={this.setAutomaticTimezone}
/>
</Form>
</ScreenWrapper>
Expand Down
4 changes: 2 additions & 2 deletions src/stories/Form.stories.js
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ const Template = (args) => {
// Form consumes data from Onyx, so we initialize Onyx with the necessary data here
NetworkConnection.setOfflineStatus(false);
FormActions.setIsLoading(args.formID, args.formState.isLoading);
FormActions.setErrorMessage(args.formID, args.formState.error);
FormActions.setErrors(args.formID, args.formState.error);
FormActions.setDraftValues(args.formID, args.draftValues);

return (
Expand Down Expand Up @@ -132,7 +132,7 @@ const WithNativeEventHandler = (args) => {
// Form consumes data from Onyx, so we initialize Onyx with the necessary data here
NetworkConnection.setOfflineStatus(false);
FormActions.setIsLoading(args.formID, args.formState.isLoading);
FormActions.setErrorMessage(args.formID, args.formState.error);
FormActions.setErrors(args.formID, args.formState.error);
FormActions.setDraftValues(args.formID, args.draftValues);

return (
Expand Down
Loading