Remove usage of setNativeProps #10934

rushatgabhane · 2022-09-10T11:52:07Z

Why?

setNativeProps will not be supported in the post-Fabric world (docs). Removing it's usage is a prerequisite to enable Fabric in #8503

Details

For SignInPageForm, TextInputLabel/index and TextInput/index
Usage of setNativeProps is migrated to setAttribute

For ReportActionCompose, Form, OptionsSelector and ReportSettingsPage
Usage of setNativeProps is migrated to state

For InvertedFlatList
Usage of setNativeProps is migrated by passing styles.translate0InvertY as a prop

For Picker and ReportActionItemMessageEdit
Usage of setNativeProps is just removed. No migration was needed.

RoomNameInput was modified to support controlled inputs using value prop.

Fixed Issues

$ #8503
(prerequisite step)

Tests

Lotsss of tests here. Brace yourself!

Sign In page

Navigate to NewDot on the web.
Right-click on the email field.
Click on the 1Password option to show the autofill.
1Password Autofill option should be shown on the input.

New group page / OptionsSelector

Click on green fab icon (+)
New group
Type the email address of a contact that you have.
Select the contact
Verify that the entered text is selected.

Message Edit

Navigate to a chat in New expensify
Send a message.
Hover over the message and click on the edit comment option in the menu
Verify that the cursor is set to the end of the message.

And
On web and desktop

Navigate to a chat in New expensify
Send a message.
Tap up arrow key to edit the previous comment.
See the cursor is set to the end of the message.

Picker

Run npm run storybook and test picker functionalities. Make sure that we can edit the input and all values are updated accordinly.
Test ALL usages of Picker (any dropdown select input) in the app. Set new value, update value and verify error states

Settings > Payments > Add payment method > Add debit card > Select state
Workspace > Connect bank account (Additional information) > State
Policy room (#announce) > Settings > Notify me about new messages
Settings > Preferences > Priority mode
Settings > Preferences > Language
Settings > Profile > Preferred pronouns
Settings > Profile > Timezone
Workspace > Connect bank account (Company step) > Address state
Workspace > Connect bank account (Company step) > Company type
Workspace > Connect bank account (Company step) > Corporation state
Workspace > Connect bank account (Personal information) > State
Login page > Select locale (below terms and conditions)

Form component

Run npm run storybook and test Form. Make sure that we can edit the input and all values are updated accordingly.
Verify that form inputs validate on Blur and Submit.
Verify that the correct values are being submitted.

Address search

Go to Settings - Payments -> Add payment method -> Add debit card page
Verify that you can search an address.
Verify that you can select an address.

Pages using Form.js

Add debit card page
1. Go to Settings - Payments -> Add payment method -> Add debit card page
2. Verify that you can edit all inputs.
3. Verify that the input is being validated on blur, and submit.
Request a call page
1. Go to Concierge -> Click on the call button
2. Verify that you can edit all inputs.
3. Verify that the input is being validated on blur, and submit.
Profile page
1. Go to settings -> Profile
2. Verify that you can edit all inputs.
3. Verify that the input is being validated on blur, and submit.

Chat / InvertedFlatList

On Safari and Firefox,

Verify that the scrolling a chat isn't "choppy"

Composer

Go to any chat
Verify that you can type text in the composer.
Open emoji picker
Verify that you can add emojis, and that cursor postion is after emoji.

This step is not applicable for QA - Room name input

Prerequisite - comment out the validations in ReportSettingsPage

Click on green fab icon (+)
Create a new room with the name "foo"
Repeat step 1, and create a new room with the name "bar" in the same workspace as room "foo".
Go to room "bar" -> Room settings
Rename it to "foo"
Click Save
Click the "X" mark after RBR error.
Verify that room name input is now back to "bar".

Screen.Recording.2022-09-10.at.6.31.51.PM.mov

Verify that no errors appear in the JS console

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

The Contributor+ will copy/paste it into a new comment and complete it after the author checklist is completed

QA Steps

Sign In page

Navigate to NewDot on the web.
Right-click on the email field.
Click on the 1Password option to show the autofill.
1Password Autofill option should be shown on the input.

New group page / OptionsSelector

Click on green fab icon (+)
New group
Type the email address of a contact that you have.
Select the contact
Verify that the entered text is selected.

Message Edit

Navigate to a chat in New expensify
Send a message.
Hover over the message and click on the edit comment option in the menu
Verify that the cursor is set to the end of the message.

And
On web and desktop

Navigate to a chat in New expensify
Send a message.
Tap up arrow key to edit the previous comment.
See the cursor is set to the end of the message.

Picker

Got to storybook and test picker functionalities. Make sure that we can edit the input and all values are updated accordinly.
Test ALL usages of Picker (any dropdown select input) in the app. Set new value, update value and verify error states

Settings > Payments > Add payment method > Add debit card > Select state
Workspace > Connect bank account (Additional information) > State
Policy room (#announce) > Settings > Notify me about new messages
Settings > Preferences > Priority mode
Settings > Preferences > Language
Settings > Profile > Preferred pronouns
Settings > Profile > Timezone
Workspace > Connect bank account (Company step) > Address state
Workspace > Connect bank account (Company step) > Company type
Workspace > Connect bank account (Company step) > Corporation state
Workspace > Connect bank account (Personal information) > State
Login page > Select locale (below terms and conditions)

Form component

Go to storybook and test Form. Make sure that we can edit the input and all values are updated accordingly.
Verify that form inputs validate on Blur and Submit.
Verify that the correct values are being submitted.

Pages using Form.js

Add debit card page
1. Go to Settings - Payments -> Add payment method -> Add debit card page
2. Verify that you can edit all inputs.
3. Verify that the input is being validated on blur, and submit.
Request a call page
1. Go to Concierge -> Click on the call button
2. Verify that you can edit all inputs.
3. Verify that the input is being validated on blur, and submit.
Profile page
Go to settings -> Profile
Verify that you can edit all inputs.
Verify that the input is being validated on blur, and submit.

Chat / InvertedFlatList

On Safari and Firefox,

Verify that the scrolling a chat isn't "choppy"

Composer

Go to any chat
Verify that you can type text in the composer.
Open emoji picker
Verify that you can add emojis, and that cursor postion is after emoji.

Verify that no errors appear in the JS console

Screenshots

Web

Mobile Web

Desktop

iOS

Android

This reverts commit 1295ebb.

…tiveProps

This reverts commit 4c90e34.

This reverts commit 1bfc4e2.

melvin-bot · 2022-09-10T11:52:10Z

Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers!

rushatgabhane · 2022-09-10T13:29:05Z

src/components/InvertedFlatList/index.js

                ref={el => this.list = el}
                shouldMeasureItems
                contentContainerStyle={StyleSheet.compose(this.props.contentContainerStyle, styles.justifyContentEnd)}
+                style={styles.translate0InvertY}


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?

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

setnativeProps were causing issues

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

rushatgabhane

Update: This is fixed - #10934 (comment)

Bug - AddressSearch when used with Form is broken. (https://expensify.slack.com/archives/C01GTK53T8Q/p1663164808524219)

Go to Settings -> Payments -> Add payment method -> Add debit card
Search for any billing address
Click a suggested address

Expected: Street address, City, State and Zipcode are autofilled.
Actual: Nothing happens

rushatgabhane · 2022-09-11T02:20:53Z

AddressSearch when used with Form is broken.

This is because Form was designed to be uncontrolled. I fixed this by automatically passing a value prop to all Form inputs.
value: this.state.inputValues[inputID]

Todo:

Update Form docs (I'll do this once all the form changes are finalized)

rushatgabhane · 2022-09-13T19:50:07Z

asked for @parasharrajat's review to get the wheels rolling

rushatgabhane · 2022-09-13T20:06:21Z

src/components/Picker/BasePicker/index.js

-                    // eslint-disable-next-line no-param-reassign
-                    node.setNativeProps = this.setNativeProps;
-                }}
+                ref={this.props.innerRef}


did i do this right?

Form passes a function to register this ref, so we should do something similar to this, no?

parasharrajat

Just a few thoughts. I will start my review soon. There a lot of things to go over.

parasharrajat · 2022-09-13T19:57:56Z

package.json

+    "@storybook/builder-webpack5": "^6.5.10",
+    "@storybook/manager-webpack5": "^6.5.10",


Why is this change in this PR?

it's so that we can test on storybook.
storybook is currently broken on main.

I created a seperate PR for fixing storybook - #10933
This diff will go away once it's merged

Merging main should get rid of this diff since the storybook PR is merged

parasharrajat · 2022-09-13T19:59:07Z

src/components/InvertedFlatList/index.js

                ref={el => this.list = el}
                shouldMeasureItems
                contentContainerStyle={StyleSheet.compose(this.props.contentContainerStyle, styles.justifyContentEnd)}
+                style={styles.translate0InvertY}


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

parasharrajat · 2022-09-13T20:07:09Z

src/styles/styles.js

    },
+
+    translate0InvertY: {
+        transform: 'translate3d(0,0,0) scaleY(-1)',


This should be causing issues on Native if used. Even if this is used for the web, we can't put it into the global stylesheet. Use proper format for the transformation in react-native. In the future, someone can use it or can be confused by it.

Oh yeahh, didn't think about it.
I'll see where it could be moved

src/components/InvertedFlatList/index.js

parasharrajat · 2022-09-13T20:17:01Z

src/components/Picker/BasePicker/index.js

-                value={this.props.value || this.pickerValue}
+                value={this.props.value || this.props.defaultValue}


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

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.

App/src/components/Form.js

Line 154 in 97ebb65

defaultValue,

I wanted to avoid adding more tests or break anything basically

For example, StatePicker uses defaultValue.

We'd be introducing unrelated diffs and tests.

parasharrajat · 2022-09-13T20:17:31Z

src/components/Picker/BasePicker/index.js

                pickerProps={{
                    onFocus: this.props.onOpen,
                    onBlur: this.executeOnCloseAndOnBlur,
+                    ref: this.props.innerRef,


I don't know. Must be a mistake :))

I think we should remove this

parasharrajat · 2022-09-13T20:19:21Z

src/components/Picker/index.js

+
+        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);


How is this change related to this PR?

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

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.

App/src/components/Form.js

Line 166 in 1c78eb8

inputRef.setNativeProps({value});

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

luacmartins · 2022-09-14T19:59:26Z

@parasharrajat please let me know once you are done reviewing this one.

parasharrajat · 2022-09-15T06:00:22Z

Sure, Can I get assigned to the issue as well?

luacmartins · 2022-09-15T13:33:43Z

@parasharrajat hmm I see that this PR is only a part of the linked issue. Do you intend to work on the full migration to Fabric?

luacmartins

I think it would have been better to break this changes into multiple PRs so it's easier to test them. I'll test once the change requests have been addressed.

luacmartins · 2022-09-15T15:21:02Z

src/components/Form.js

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


So this means that inputs are no longer uncontrolled?

@luacmartins yes, that's right

luacmartins · 2022-09-15T15:35:28Z

src/components/Picker/BasePicker/index.js

                pickerProps={{
                    onFocus: this.props.onOpen,
                    onBlur: this.executeOnCloseAndOnBlur,
+                    ref: this.props.innerRef,


I think we should remove this

luacmartins · 2022-09-15T15:38:27Z

src/components/Picker/BasePicker/index.js

-                    // eslint-disable-next-line no-param-reassign
-                    node.setNativeProps = this.setNativeProps;
-                }}
+                ref={this.props.innerRef}


Form passes a function to register this ref, so we should do something similar to this, no?

luacmartins · 2022-09-15T16:02:22Z

src/pages/home/report/ReportActionCompose.js

-        this.textInput.setNativeProps({text: newComment});
        this.setState({
            isCommentEmpty: !!newComment.match(/^(\s|`)*$/),
+            value: newComment,


We should add value to state

luacmartins · 2022-09-15T16:04:24Z

src/pages/home/report/ReportActionItemMessageEdit.js

     * @param {String} newDraft
     */
    updateDraft(newDraft) {
-        this.textInput.setNativeProps({text: newDraft});


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

yep, I have already added tests for it

luacmartins · 2022-09-15T16:05:30Z

src/pages/settings/Profile/ProfilePage.js

                            items={timezones}
                            isDisabled={this.state.isAutomaticTimezone}
                            defaultValue={this.state.selectedTimezone}
-                            value={this.state.selectedTimezone}


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

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

luacmartins · 2022-09-15T16:11:31Z

src/styles/styles.js

    },
+
+    translate0InvertY: {
+        transform: 'translate3d(0,0,0) scaleY(-1)',


I think this is the correct format?

Suggested change

transform: 'translate3d(0,0,0) scaleY(-1)',

transform: [{translateX: 0}, {translateY: 0}, {scaleY: -1}],

Will have to verify. I'll breakup all this into multiple PRs.

rushatgabhane · 2022-09-15T22:02:49Z

I think it would have been better to break this changes into multiple PRs so it's easier to test them. I'll test once the change requests have been addressed.

@luacmartins I agree.
I'll work on breaking them into multiple PRs.

rushatgabhane · 2022-09-16T00:39:53Z

Closing and splitting this PR into logical chunks for easy review and testing.

#11039
#11038
#11037
#11036
#11035
#11034
#11033

rushatgabhane added 23 commits September 8, 2022 21:47

composer - move setNativeProps to state

8666d97

reportEdit - remove setNativeProps

ecabcfc

SigninPageForm - remove setNativeProps

1295ebb

TextInput - replace setNativeProps with setAttribute

27020f5

Revert "SigninPageForm - remove setNativeProps"

72bbc0b

This reverts commit 1295ebb.

SignInPageForm - replace setNativeProps with setAttribute

ddfabdf

TextInputLabel - replace setNativeProps with setAttribute

12fa935

BaseOptionsSelector - move setNativeProps to state

e45288e

Form - move inputValues to state to remove setNativeProps

e2b07d2

InvertedFlatList - remove setNativeProps, pass style as a prop instead.

1cce3d5

Make Picker controlled only and remove setNativeProps

4c90e34

fix picker state

1bfc4e2

Merge remote-tracking branch 'origin/fix-storybook' into remove-setNa…

a0ff498

…tiveProps

Merge remote-tracking branch 'origin/fix-storybook' into remove-setNa…

8528c0d

…tiveProps

Merge remote-tracking branch 'origin/fix-storybook' into remove-setNa…

deeaef9

…tiveProps

Revert "Make Picker controlled only and remove setNativeProps"

b77ab39

This reverts commit 4c90e34.

Revert "fix picker state"

3298fa7

This reverts commit 1bfc4e2.

fix stale Form values validation

28fdeac

remove setNativeProps from Picker

38fd5ad

remove unnecessary value from profile page

e23f51c

remove setNativeProps from reportSettingsPage

103e340

fix eslint errors

1192191

fix eslint errors

237d0dd

rushatgabhane marked this pull request as ready for review September 10, 2022 13:10

rushatgabhane requested a review from a team as a code owner September 10, 2022 13:10

melvin-bot bot requested review from stitesExpensify and removed request for a team September 10, 2022 13:10

rushatgabhane commented Sep 10, 2022

View reviewed changes

rushatgabhane added 3 commits September 11, 2022 05:05

fix setAttribute

7dd836f

make picker work with Form

0644e1d

fix lint errors

e0fb68b

rushatgabhane commented Sep 11, 2022

View reviewed changes

rushatgabhane added 2 commits September 11, 2022 07:51

pass value to Form inputs

a8b8e7b

Merge branch 'Expensify:main' into remove-setNativeProps

97ebb65

rushatgabhane mentioned this pull request Sep 13, 2022

[HOLD for payment 2024-04-25] [Tracking] [Performance] Enable the new Fabric architecture #8503

Closed

38 tasks

rushatgabhane commented Sep 13, 2022

View reviewed changes

parasharrajat suggested changes Sep 13, 2022

View reviewed changes

cleaner styleee

5f1ecfe

Co-authored-by: Rajat Parashar <parasharrajat@users.noreply.github.com>

rushatgabhane mentioned this pull request Sep 13, 2022

[No QA] Fix storybook #10933

Merged

93 tasks

luacmartins self-requested a review September 14, 2022 14:43

Merge branch 'Expensify:main' into remove-setNativeProps

b4d3bdd

luacmartins reviewed Sep 15, 2022

View reviewed changes

rushatgabhane added 5 commits September 16, 2022 03:38

init value in state

74cb3cb

fix picker ref

cbf0415

rm unnecessary picker prop

358a77f

profile page - use value prop for pickers

aaab06b

Merge branch 'Expensify:main' into remove-setNativeProps

2122178

rushatgabhane closed this Sep 16, 2022

rushatgabhane mentioned this pull request Sep 16, 2022

Form - Migrate setNativeProps to state. #11039

Merged

93 tasks

		"@storybook/builder-webpack5": "^6.5.10",
		"@storybook/manager-webpack5": "^6.5.10",

		value={this.props.value \|\| this.pickerValue}
		value={this.props.value \|\| this.props.defaultValue}

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

	transform: 'translate3d(0,0,0) scaleY(-1)',
	transform: [{translateX: 0}, {translateY: 0}, {scaleY: -1}],

Remove usage of setNativeProps #10934

Remove usage of setNativeProps #10934

Uh oh!

Conversation

rushatgabhane commented Sep 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why?

Details

Fixed Issues

Tests

Sign In page

New group page / OptionsSelector

Message Edit

Picker

Form component

Address search

Pages using Form.js

Chat / InvertedFlatList

Composer

This step is not applicable for QA - Room name input

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Sign In page

New group page / OptionsSelector

Message Edit

Picker

Form component

Pages using Form.js

Chat / InvertedFlatList

Composer

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Uh oh!

melvin-bot bot commented Sep 10, 2022

Uh oh!

rushatgabhane Sep 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rushatgabhane Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rushatgabhane left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rushatgabhane commented Sep 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rushatgabhane commented Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

parasharrajat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rushatgabhane Sep 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rushatgabhane commented Sep 10, 2022 •

edited

Loading

rushatgabhane Sep 10, 2022 •

edited

Loading

rushatgabhane Sep 13, 2022 •

edited

Loading

rushatgabhane left a comment •

edited

Loading

rushatgabhane commented Sep 11, 2022 •

edited

Loading

rushatgabhane commented Sep 13, 2022 •

edited

Loading

rushatgabhane Sep 13, 2022 •

edited

Loading

rushatgabhane Sep 13, 2022 •

edited

Loading

rushatgabhane Sep 13, 2022 •

edited

Loading

rushatgabhane Sep 13, 2022 •

edited

Loading

rushatgabhane Sep 13, 2022 •

edited

Loading

rushatgabhane Sep 13, 2022 •

edited

Loading