Form - Migrate setNativeProps to state. #11039

rushatgabhane · 2022-09-16T01:15:40Z

Details

setNativeProps will not be supported in the post-Fabric world (docs). Removing it's usage is a prerequisite to enable Fabric in #8503 . I'm creating multiple PRs to migrate it's usage. This is one of many.

Form is made controlled, and Picker's usage of setNativeProps has been removed.

Fixed Issues

$ #11049
$ #11646
$ #11654
$ #10437

Tests

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.

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)

Address search

Go to Settings - Payments -> Add payment method -> Add debit card page
Verify that you can search for an address.
Verify that you can select an address.
Verify that Street address, City, State and Zipcode are autofilled after selecting the 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.

Blueline around the fields

Go to Settings->Workspaсe->General settings
Change the workspace name and then change the currency
Verify that the two fields aren't highlighted at the same time

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

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.

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)

Address search

Go to Settings - Payments -> Add payment method -> Add debit card page
Verify that you can search for an address.
Verify that you can select an address.
Verify that Street address, City, State and Zipcode are autofilled after selecting the 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.

Blueline around the fields

Go to Settings->Workspaсe->General settings
Change the workspace name and then change the currency
Verify that the two fields aren't highlighted at the same time

Verify that no errors appear in the JS console

Screenshots

Web

Form - Profile Page

Screen.Recording.2022-10-05.at.2.15.17.AM.mov

Form - Debit card page

Screen.Recording.2022-10-05.at.2.18.38.AM.mov

Non Form Picker - Preferences page

Screen.Recording.2022-10-05.at.2.37.04.AM.mov

Mobile Web

Screen.Recording.2022-10-05.at.2.34.09.AM.mov

Desktop

Working of the Address search component

Screen.Recording.2022-10-05.at.2.24.43.AM.mov

iOS

Android

screen-20221005-030018.mp4

Storybook

Screen.Recording.2022-10-05.at.3.02.08.AM.mov

melvin-bot · 2022-09-16T01:15:43Z

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-16T01:17:14Z

src/components/Picker/index.js

+            return;
+        }
+
+        this.props.onInputChange(value, index);


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.

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.

@rushatgabhane what are your thoughts about my comment above?

just started reading this, will get back soon

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.

rushatgabhane · 2022-09-16T01:30:41Z

@luacmartins this is the PR you might be most interested in.

I've addressed your comments from #10934 (review)

parasharrajat · 2022-09-16T08:29:36Z

First, we should create a new issue instead of using #8503. This PR has nothing to do with Fabric architecture technically.

rushatgabhane · 2022-09-16T09:19:12Z

@parasharrajat this PR is a prerequisite step to enabling fabric right?
because we're removing setNativeProps

parasharrajat · 2022-09-16T10:06:35Z

I know but does it enable the Fabric architecture?

rushatgabhane · 2022-09-16T10:22:25Z

Fair enough

rushatgabhane · 2022-09-16T14:21:27Z

@parasharrajat @luacmartins are we going on the right track?

I feel like uncontrolled Form was so much cleaner. You didn't need to maintain values in state for pages that are using Form.

Is there a better approach?

parasharrajat · 2022-09-16T14:23:16Z

That was one of the reasons, we use uncontrolled inputs. 😄

luacmartins · 2022-09-16T14:48:48Z

I'm not sure there's much that we can do in this case. State seems to be the correct solution.

rushatgabhane · 2022-09-16T14:51:13Z

Cool, glad to hear we're on the right track.

P.S. Sorry but I'll add the screenshots to all the PRs within a week

luacmartins · 2022-09-19T16:18:02Z

@rushatgabhane is this ready for review?

rushatgabhane · 2022-09-19T16:43:22Z

ready for review

luacmartins

@rushatgabhane

Run npm run storybook and test picker functionalities. Make sure that we can edit the input and all values are updated accordinly.

NAB just noting that the callback provided in Picker's story is an empty lambda and the value is hardcoded, so selecting a different value and blurring the input resets the value to the previous one.

I did find an issue while testing though. In Profile > Timezone, manually selecting a different timezone and then checking Set my timezone automatically doesn't update the timezone to the correct one. The first value loaded also is incorrect for me, it should be Americas/Edmonton in my case.

Screen.Recording.2022-09-20.at.11.06.59.AM.mov

rushatgabhane · 2022-09-25T10:02:40Z

Thanks, I'm working on it

rushatgabhane · 2022-10-06T14:46:50Z

src/pages/settings/Profile/ProfilePage.js

+            return;
+        }
+
+        this.setState({hasSelfSelectedPronouns});


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

luacmartins

@rushatgabhane I did notice the following bugs while testing:

Picker does not work on Workspace > Connect bank account (Company step) > Company type and Workspace > Connect bank account (Company step) > Corporation state

error1.mov

Picker is selecting the value before user clicks done on iOS:

error2.mov

Additionally, the bugs below seem to already be on prod 🤦

luacmartins · 2022-10-06T18:40:27Z

Address autofill does not automatically fill street and zip code

This seems to be fixed with the changes in this PR 🎉

…ensify-App into rm-setNativeProps-form

rushatgabhane · 2022-10-06T20:43:45Z

Picker is selecting the value before user clicks done on iOS

This was introduced by #11120 when I merged with main.

Fixed the iOS issue and #10437 as well by removing they key prop from BasePicker.

Screen.Recording.2022-10-07.at.2.06.38.AM.mov

Picker does not work on Workspace > Connect bank account (Company step) > Company type and Workspace > Connect bank account (Company step) > Corporation state

Sorry about that, I missed Pickers in CompanyStep page. I went through all other pages to make sure it isn't happening elsewhere.

rushatgabhane · 2022-10-06T20:56:50Z

Picker focus outline persists after blurring the input

This is resolved by this PR as well 🎉
(key prop for BasePicker was the culprit)

Screen.Recording.2022-10-07.at.2.24.54.AM.mov

rushatgabhane · 2022-10-06T21:01:43Z

@luacmartins could you please give this another look? All 4 bugs you had mentioned are now fixed.

chiragsalian · 2022-10-06T23:37:38Z

Should we apply CP-staging label on this PR since it addresses a deploy blocker issue.

I think we should either CP this and then ask QA to regression test again since this changes a lot, OR ignore the deploy blocker for now since it's minor. Either way, I think regression testing should be done once this PR is on staging since its changes affect a number of components. Thoughts?

luacmartins · 2022-10-07T15:34:00Z

@chiragsalian I think that we should remove the blocker label from that issue since it's a minor UI bug and have this go through the regular QA process and have it tested with full regression tests.

luacmartins

LGTM and tested well. Thanks for squashing all those bugs! Awesome job!

rushatgabhane · 2022-10-07T16:33:01Z

Wohoo! 🎉
Thanks for the thorough review and helping me understanding form better

OSBotify · 2022-10-07T16:34:15Z

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

OSBotify · 2022-10-12T00:06:02Z

🚀 Deployed to staging by @luacmartins in version: 1.2.13-0 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

OSBotify · 2022-10-13T05:56:01Z

🚀 Deployed to production by @yuwenmemon in version: 1.2.13-5 🚀

platform	result
🤖 android 🤖	success ✅
🖥 desktop 🖥	success ✅
🍎 iOS 🍎	success ✅
🕸 web 🕸	success ✅

…ps-form Form - Migrate setNativeProps to state.

aldo-expensify · 2022-11-14T22:45:48Z

src/components/Form.js

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


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.

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

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

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:

React documentation says we shouldn't mutate state: https://reactjs.org/docs/react-component.html#state

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

Class component: member variable

Functional component: useRef

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.

App/src/components/Form.js

Line 176 in c72e7ac

this.validate(this.state.inputValues);

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: {

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?

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.

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

Awesome, thanks guys!

rushatgabhane added 3 commits September 16, 2022 05:49

make form controlled by moving inputValues to state

1e1765a

Make Picker controlled only and remove setNativeProps

58505b4

make picker work with Form

50373ab

rushatgabhane requested a review from a team as a code owner September 16, 2022 01:15

melvin-bot bot requested review from stitesExpensify and removed request for a team September 16, 2022 01:15

rushatgabhane commented Sep 16, 2022

View reviewed changes

remove defaultValue prop from Picker

a7f4f88

This was referenced Sep 16, 2022

Remove usage of setNativeProps #10934

Closed

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

Closed

rushatgabhane changed the title ~~Rm set native props form~~ Form - Migrate setNativeProps to state. Sep 16, 2022

fix eslint error

3fb74b2

This comment was marked as outdated.

Sign in to view

luacmartins self-requested a review September 16, 2022 13:18

Merge branch 'Expensify:main' into rm-setNativeProps-form

d3e630b

stitesExpensify assigned rushatgabhane Sep 19, 2022

luacmartins requested changes Sep 20, 2022

View reviewed changes

fix typo

ff83af2

rushatgabhane commented Oct 6, 2022

View reviewed changes

luacmartins requested changes Oct 6, 2022

View reviewed changes

rushatgabhane added 3 commits October 7, 2022 00:44

fix: pickers on company info page

ddc4050

Merge branch 'rm-setNativeProps-form' of github.com:rushatgabhane/Exp…

1273efc

…ensify-App into rm-setNativeProps-form

fix: picker closing before clicking done on ios

aab173b

rushatgabhane requested a review from luacmartins October 6, 2022 20:57

This was referenced Oct 6, 2022

Prevent picker onInputChange firing on re-render #11120

Merged

[$250] BUG - Create room - Workspace is not selected by default if one workspace only available in the account #11650

Closed

chiragsalian mentioned this pull request Oct 6, 2022

Workspace - Blueline around the fields does not disappear moving to another field #11646

Closed

Merge branch 'Expensify:main' into rm-setNativeProps-form

c72e7ac

luacmartins approved these changes Oct 7, 2022

View reviewed changes

luacmartins merged commit a4be883 into Expensify:main Oct 7, 2022

rushatgabhane mentioned this pull request Oct 9, 2022

BUG:Pay with debit card - Doesn't auto-complete the zip and state correctly #11654

Closed

NikkiWines mentioned this pull request Oct 11, 2022

[Refactor] Use new API Command: UpdateBeneficialOwnersForBankAccount #10994

Merged

90 tasks

luacmartins mentioned this pull request Oct 11, 2022

[Form Refactor] Add custom input callback feature to Form #10267

Closed

OSBotify mentioned this pull request Oct 11, 2022

Deploy Checklist: New Expensify 2022-10-11 #11741

Closed

48 tasks

smrutiparida pushed a commit to autosave-app/App that referenced this pull request Oct 13, 2022

Merge pull request Expensify#11039 from rushatgabhane/rm-setNativePro…

8c6dab8

…ps-form Form - Migrate setNativeProps to state.

michaelhaxhiu mentioned this pull request Nov 8, 2022

Various fabric improvements and migrations completed by Rushat #12568

Closed

aldo-expensify reviewed Nov 14, 2022

View reviewed changes

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

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

Form - Migrate setNativeProps to state. #11039

Form - Migrate setNativeProps to state. #11039

Conversation

rushatgabhane commented Sep 16, 2022 • edited Loading

Details

Fixed Issues

Tests

Form component

Picker

Address search

Pages using Form.js

Blueline around the fields

PR Review Checklist

Contributor (PR Author) Checklist

PR Reviewer Checklist

QA Steps

Form component

Picker

Address search

Pages using Form.js

Blueline around the fields

Screenshots

Web

Mobile Web

Desktop

iOS

Android

Storybook

melvin-bot bot commented Sep 16, 2022

rushatgabhane Sep 16, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rushatgabhane commented Sep 16, 2022

This comment was marked as outdated.

parasharrajat commented Sep 16, 2022

rushatgabhane commented Sep 16, 2022

parasharrajat commented Sep 16, 2022

rushatgabhane commented Sep 16, 2022

rushatgabhane commented Sep 16, 2022 • edited Loading

parasharrajat commented Sep 16, 2022

luacmartins commented Sep 16, 2022

rushatgabhane commented Sep 16, 2022 • edited Loading

luacmartins commented Sep 19, 2022

rushatgabhane commented Sep 19, 2022

luacmartins left a comment

Choose a reason for hiding this comment

rushatgabhane commented Sep 25, 2022

rushatgabhane Oct 6, 2022 • edited Loading

Choose a reason for hiding this comment

luacmartins left a comment • edited Loading

Choose a reason for hiding this comment

luacmartins commented Oct 6, 2022

rushatgabhane commented Oct 6, 2022 • edited Loading

rushatgabhane commented Oct 6, 2022 • edited Loading

rushatgabhane commented Oct 6, 2022 • edited Loading

chiragsalian commented Oct 6, 2022

luacmartins commented Oct 7, 2022

luacmartins left a comment

Choose a reason for hiding this comment

rushatgabhane commented Oct 7, 2022 • edited Loading

OSBotify commented Oct 7, 2022

OSBotify commented Oct 12, 2022

OSBotify commented Oct 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aldo-expensify Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rushatgabhane Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rushatgabhane Nov 28, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rushatgabhane commented Sep 16, 2022 •

edited

Loading

rushatgabhane Sep 16, 2022 •

edited

Loading

rushatgabhane commented Sep 16, 2022 •

edited

Loading

rushatgabhane commented Sep 16, 2022 •

edited

Loading

rushatgabhane Oct 6, 2022 •

edited

Loading

luacmartins left a comment •

edited

Loading

rushatgabhane commented Oct 6, 2022 •

edited

Loading

rushatgabhane commented Oct 6, 2022 •

edited

Loading

rushatgabhane commented Oct 6, 2022 •

edited

Loading

rushatgabhane commented Oct 7, 2022 •

edited

Loading

aldo-expensify Nov 28, 2022 •

edited

Loading

rushatgabhane Nov 28, 2022 •

edited

Loading

rushatgabhane Nov 28, 2022 •

edited

Loading