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

Remove usage of setNativeProps #10934

Closed
wants to merge 35 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
8666d97
composer - move setNativeProps to state
rushatgabhane Sep 8, 2022
ecabcfc
reportEdit - remove setNativeProps
rushatgabhane Sep 8, 2022
1295ebb
SigninPageForm - remove setNativeProps
rushatgabhane Sep 8, 2022
27020f5
TextInput - replace setNativeProps with setAttribute
rushatgabhane Sep 8, 2022
72bbc0b
Revert "SigninPageForm - remove setNativeProps"
rushatgabhane Sep 8, 2022
ddfabdf
SignInPageForm - replace setNativeProps with setAttribute
rushatgabhane Sep 8, 2022
12fa935
TextInputLabel - replace setNativeProps with setAttribute
rushatgabhane Sep 8, 2022
e45288e
BaseOptionsSelector - move setNativeProps to state
rushatgabhane Sep 8, 2022
e2b07d2
Form - move inputValues to state to remove setNativeProps
rushatgabhane Sep 8, 2022
1cce3d5
InvertedFlatList - remove setNativeProps, pass style as a prop instead.
rushatgabhane Sep 8, 2022
4c90e34
Make Picker controlled only and remove setNativeProps
rushatgabhane Sep 8, 2022
1bfc4e2
fix picker state
rushatgabhane Sep 8, 2022
a0ff498
Merge remote-tracking branch 'origin/fix-storybook' into remove-setNa…
rushatgabhane Sep 10, 2022
8528c0d
Merge remote-tracking branch 'origin/fix-storybook' into remove-setNa…
rushatgabhane Sep 10, 2022
deeaef9
Merge remote-tracking branch 'origin/fix-storybook' into remove-setNa…
rushatgabhane Sep 10, 2022
b77ab39
Revert "Make Picker controlled only and remove setNativeProps"
rushatgabhane Sep 10, 2022
3298fa7
Revert "fix picker state"
rushatgabhane Sep 10, 2022
28fdeac
fix stale Form values validation
rushatgabhane Sep 10, 2022
38fd5ad
remove setNativeProps from Picker
rushatgabhane Sep 10, 2022
e23f51c
remove unnecessary value from profile page
rushatgabhane Sep 10, 2022
103e340
remove setNativeProps from reportSettingsPage
rushatgabhane Sep 10, 2022
1192191
fix eslint errors
rushatgabhane Sep 10, 2022
237d0dd
fix eslint errors
rushatgabhane Sep 10, 2022
7dd836f
fix setAttribute
rushatgabhane Sep 10, 2022
0644e1d
make picker work with Form
rushatgabhane Sep 11, 2022
e0fb68b
fix lint errors
rushatgabhane Sep 11, 2022
a8b8e7b
pass value to Form inputs
rushatgabhane Sep 11, 2022
97ebb65
Merge branch 'Expensify:main' into remove-setNativeProps
rushatgabhane Sep 13, 2022
5f1ecfe
cleaner styleee
rushatgabhane Sep 13, 2022
b4d3bdd
Merge branch 'Expensify:main' into remove-setNativeProps
rushatgabhane Sep 14, 2022
74cb3cb
init value in state
rushatgabhane Sep 15, 2022
cbf0415
fix picker ref
rushatgabhane Sep 15, 2022
358a77f
rm unnecessary picker prop
rushatgabhane Sep 15, 2022
aaab06b
profile page - use value prop for pickers
rushatgabhane Sep 15, 2022
2122178
Merge branch 'Expensify:main' into remove-setNativeProps
rushatgabhane Sep 15, 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
25 changes: 13 additions & 12 deletions src/components/Form.js
Original file line number Diff line number Diff line change
Expand Up @@ -57,10 +57,10 @@ class Form extends React.Component {

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

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

this.setTouchedInput = this.setTouchedInput.bind(this);
Expand All @@ -87,12 +87,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 @@ -145,30 +145,31 @@ 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;
}

return React.cloneElement(child, {
ref: node => this.inputRefs[inputID] = node,
defaultValue,
value: this.state.inputValues[inputID],
Copy link
Contributor

Choose a reason for hiding this comment

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

So this means that inputs are no longer uncontrolled?

Copy link
Member Author

Choose a reason for hiding this comment

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

@luacmartins yes, that's right

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);
},
});
});
Expand Down
17 changes: 7 additions & 10 deletions src/components/InvertedFlatList/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -36,17 +36,13 @@ class InvertedFlatList extends React.Component {
this.props.innerRef(this.list);
}

if (this.list) {
this.list
.getScrollableNode()
.addEventListener('wheel', this.invertedWheelEvent);

this.list.setNativeProps({
style: {
transform: 'translate3d(0,0,0) scaleY(-1)',
},
});
if (!this.list) {
return;
}

this.list
.getScrollableNode()
.addEventListener('wheel', this.invertedWheelEvent);
}

componentWillUnmount() {
Expand All @@ -67,6 +63,7 @@ class InvertedFlatList extends React.Component {
ref={el => this.list = el}
shouldMeasureItems
contentContainerStyle={StyleSheet.compose(this.props.contentContainerStyle, styles.justifyContentEnd)}
style={styles.translate0InvertY}
Copy link
Member Author

@rushatgabhane rushatgabhane Sep 10, 2022

Choose a reason for hiding this comment

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

Context - we have to remove usage of setNativeProps from app as a prereq to enabling Fabric.

@marcaaron do you remember if there's any downside to pass this style as a prop vs using setNativeProps?

Copy link
Member

Choose a reason for hiding this comment

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

Good Change if this works. the setnativeProps were causing issues.

Copy link
Member Author

@rushatgabhane rushatgabhane Sep 13, 2022

Choose a reason for hiding this comment

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

setnativeProps were causing issues

out of curiousity, do you remember what kind of issues?

/>
);
}
Expand Down
11 changes: 7 additions & 4 deletions src/components/OptionsSelector/BaseOptionsSelector.js
Original file line number Diff line number Diff line change
Expand Up @@ -45,6 +45,10 @@ class BaseOptionsSelector extends Component {
this.state = {
allOptions,
focusedIndex: this.props.shouldTextInputAppearBelowOptions ? allOptions.length : 0,
selection: {
start: this.props.value.length,
end: this.props.value.length,
},
};
}

Expand Down Expand Up @@ -206,7 +210,7 @@ class BaseOptionsSelector extends Component {
selectRow(option, ref) {
if (this.props.shouldFocusOnSelectRow) {
// Input is permanently focused on native platforms, so we always highlight the text inside of it
this.textInput.setNativeProps({selection: {start: 0, end: this.props.value.length}});
this.setState({selection: {start: 0, end: this.props.value.length}});
if (this.relatedTarget && ref === findNodeHandle(this.relatedTarget)) {
this.textInput.focus();
}
Expand Down Expand Up @@ -237,9 +241,6 @@ class BaseOptionsSelector extends Component {
value={this.props.value}
label={this.props.textInputLabel}
onChangeText={(text) => {
if (this.props.shouldFocusOnSelectRow) {
this.textInput.setNativeProps({selection: null});
}
this.props.onChangeText(text);
}}
placeholder={this.props.placeholderText || this.props.translate('optionsSelector.nameEmailOrPhoneNumber')}
Expand All @@ -251,6 +252,8 @@ class BaseOptionsSelector extends Component {
}}
selectTextOnFocus
blurOnSubmit={Boolean(this.state.allOptions.length)}
selection={this.state.selection}
onSelectionChange={e => this.setState({selection: e.nativeEvent.selection})}
/>
);
const optionsList = this.props.shouldShowOptions ? (
Expand Down
33 changes: 5 additions & 28 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,12 +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}
value={this.props.value || this.props.defaultValue}
Comment on lines -50 to +31
Copy link
Member

Choose a reason for hiding this comment

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

The concept of defaultValue is unnecessary when we are using state-bound value prop.

Copy link
Member Author

@rushatgabhane rushatgabhane Sep 13, 2022

Choose a reason for hiding this comment

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

Agree! I'll explain why I didn't remove it in this PR.

I think all Form components are state based / value prop driven. But Form.js passes defaultValue to all it's children anyway because it was designed to be uncontrolled.

I reckon we should do the cleanup in a follow-up PR.

defaultValue,

I wanted to avoid adding more tests or break anything basically

Copy link
Member Author

@rushatgabhane rushatgabhane Sep 13, 2022

Choose a reason for hiding this comment

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

For example, StatePicker uses defaultValue.

We'd be introducing unrelated diffs and tests.

Icon={() => this.props.icon(this.props.size)}
disabled={this.props.disabled}
fixAndroidTouchableBug
Expand All @@ -57,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 received 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, this.props.inputID);
return;
}

this.props.onInputChange(value, index);
Comment on lines +53 to +69
Copy link
Member

Choose a reason for hiding this comment

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

How is this change related to this PR?

Copy link
Member Author

@rushatgabhane rushatgabhane Sep 13, 2022

Choose a reason for hiding this comment

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

Great question! @parasharrajat

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

Copy link
Member Author

@rushatgabhane rushatgabhane Sep 13, 2022

Choose a reason for hiding this comment

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

This wasn't a problem before because previously, Form set the value using ref.setNativeProps.

We're getting rid of that, so I had to fix Picker in this PR only.

inputRef.setNativeProps({value});

}

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
8 changes: 4 additions & 4 deletions src/components/RoomNameInput.js
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,8 @@ const propTypes = {
/** Callback to execute when the text input is modified correctly */
onChangeText: PropTypes.func,

/** Initial room name to show in input field. This should include the '#' already prefixed to the name */
initialValue: PropTypes.string,
/** Room name to show in input field. This should include the '#' already prefixed to the name */
value: PropTypes.string,

/** Whether we should show the input as disabled */
disabled: PropTypes.bool,
Expand Down Expand Up @@ -50,7 +50,7 @@ const propTypes = {

const defaultProps = {
onChangeText: () => {},
initialValue: '',
value: '',
disabled: false,
errorText: '',
...fullPolicyDefaultProps,
Expand Down Expand Up @@ -101,7 +101,7 @@ class RoomNameInput extends Component {
prefixCharacter={CONST.POLICY.ROOM_PREFIX}
placeholder={this.props.translate('newRoomPage.social')}
onChange={this.setModifiedRoomName}
defaultValue={this.props.initialValue.substring(1)} // Since the room name always starts with a prefix, we omit the first character to avoid displaying it twice.
value={this.props.value.substring(1)} // Since the room name always starts with a prefix, we omit the first character to avoid displaying it twice.
errorText={this.props.errorText}
autoCapitalize="none"
/>
Expand Down
6 changes: 2 additions & 4 deletions src/components/SignInPageForm/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,10 +8,8 @@ class Form extends React.Component {
}

// Password Managers need these attributes to be able to identify the form elements properly.
this.form.setNativeProps({
method: 'post',
action: '/',
});
this.form.setAttribute('method', 'post');
this.form.setAttribute('action', '/');
}

render() {
Expand Down
2 changes: 1 addition & 1 deletion src/components/TextInput/TextInputLabel/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ class TextInputLabel extends PureComponent {
if (!this.props.for) {
return;
}
this.label.setNativeProps({for: this.props.for});
this.label.setAttribute('for', this.props.for);
}

render() {
Expand Down
4 changes: 2 additions & 2 deletions src/components/TextInput/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,11 @@ import * as baseTextInputPropTypes from './baseTextInputPropTypes';
class TextInput extends React.Component {
componentDidMount() {
if (this.props.disableKeyboard) {
this.textInput.setNativeProps({inputmode: 'none'});
this.textInput.setAttribute('inputmode', 'none');
}

if (this.props.name) {
this.textInput.setNativeProps({name: this.props.name});
this.textInput.setAttribute('name', this.props.name);
}
}

Expand Down
7 changes: 1 addition & 6 deletions src/pages/ReportSettingsPage.js
Original file line number Diff line number Diff line change
Expand Up @@ -125,11 +125,6 @@ class ReportSettingsPage extends Component {
*/
resetToPreviousName() {
this.setState({newRoomName: this.props.report.reportName});

// Reset the input's value back to the previously saved report name
if (this.roomNameInputRef) {
this.roomNameInputRef.setNativeProps({text: this.props.report.reportName.replace(CONST.POLICY.ROOM_PREFIX, '')});
}
Report.clearPolicyRoomNameErrors(this.props.report.reportID);
}

Expand Down Expand Up @@ -229,7 +224,7 @@ class ReportSettingsPage extends Component {
: (
<RoomNameInput
ref={el => this.roomNameInputRef = el}
initialValue={this.state.newRoomName}
value={this.state.newRoomName}
policyID={linkedWorkspace && linkedWorkspace.id}
errorText={this.state.errors.newRoomName}
onChangeText={newRoomName => this.clearErrorAndSetValue('newRoomName', newRoomName)}
Expand Down
8 changes: 3 additions & 5 deletions src/pages/home/report/ReportActionCompose.js
Original file line number Diff line number Diff line change
Expand Up @@ -144,6 +144,7 @@ class ReportActionCompose extends React.Component {
end: props.comment.length,
},
maxLines: props.isSmallScreenWidth ? CONST.COMPOSER.MAX_LINES_SMALL_SCREEN : CONST.COMPOSER.MAX_LINES,
value: props.comment,
};
}

Expand Down Expand Up @@ -311,9 +312,6 @@ class ReportActionCompose extends React.Component {
addEmojiToTextBox(emoji) {
const newComment = this.comment.slice(0, this.state.selection.start)
+ emoji + this.comment.slice(this.state.selection.end, this.comment.length);
this.textInput.setNativeProps({
text: newComment,
});
this.setState(prevState => ({
selection: {
start: prevState.selection.start + emoji.length,
Expand Down Expand Up @@ -372,9 +370,9 @@ class ReportActionCompose extends React.Component {
* @param {String} newComment
*/
updateComment(newComment) {
this.textInput.setNativeProps({text: newComment});
this.setState({
isCommentEmpty: !!newComment.match(/^(\s|`)*$/),
value: newComment,
Copy link
Contributor

Choose a reason for hiding this comment

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

We should add value to state

});

// Indicate that draft has been created.
Expand Down Expand Up @@ -627,7 +625,6 @@ class ReportActionCompose extends React.Component {
this.setState({isDraggingOver: false});
}}
style={[styles.textInputCompose, this.props.isComposerFullSize ? styles.textInputFullCompose : styles.flex4]}
defaultValue={this.props.comment}
maxLines={this.state.maxLines}
onFocus={() => this.setIsFocused(true)}
onBlur={() => this.setIsFocused(false)}
Expand All @@ -640,6 +637,7 @@ class ReportActionCompose extends React.Component {
isFullComposerAvailable={this.state.isFullComposerAvailable}
setIsFullComposerAvailable={this.setIsFullComposerAvailable}
isComposerFullSize={this.props.isComposerFullSize}
value={this.state.value}
/>
</View>
</>
Expand Down
1 change: 0 additions & 1 deletion src/pages/home/report/ReportActionItemMessageEdit.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,7 +97,6 @@ class ReportActionItemMessageEdit extends React.Component {
* @param {String} newDraft
*/
updateDraft(newDraft) {
this.textInput.setNativeProps({text: newDraft});
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's make sure this issue doesn't come back

Copy link
Member Author

@rushatgabhane rushatgabhane Sep 15, 2022

Choose a reason for hiding this comment

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

yep, I have already added tests for it

this.setState({draft: newDraft});

// This component is rendered only when draft is set to a non-empty string. In order to prevent component
Expand Down
3 changes: 1 addition & 2 deletions src/pages/settings/Profile/ProfilePage.js
Original file line number Diff line number Diff line change
Expand Up @@ -260,7 +260,7 @@ class ProfilePage extends Component {
value: '',
label: this.props.translate('profilePage.selectYourPronouns'),
}}
defaultValue={pronounsPickerValue}
value={pronounsPickerValue}
/>
{this.state.hasSelfSelectedPronouns && (
<View style={styles.mt2}>
Expand Down Expand Up @@ -290,7 +290,6 @@ class ProfilePage extends Component {
label={this.props.translate('profilePage.timezone')}
items={timezones}
isDisabled={this.state.isAutomaticTimezone}
defaultValue={this.state.selectedTimezone}
value={this.state.selectedTimezone}
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we remove other usages of this.state.selectedTimezone in this file?

Copy link
Member Author

@rushatgabhane rushatgabhane Sep 15, 2022

Choose a reason for hiding this comment

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

Can't because it's controlled.
I now removed defaultValue, and passed value.

/>
</View>
Expand Down
Loading