[No QA] No Props/State Destructuring

[roryabraham]

Details

Implements a new ESLint rule that bans destructuring of props and/or state.

Fixed Issues

$ #6250
Coming from #5984 (comment)

Tests / QA Steps

Just regression testing. Try to display every view in the app. Keep the JS console open and verify that there are no errors.

Tested On

• Web
• Mobile Web
• Desktop
• iOS
• Android

[roryabraham]

Putting this on HOLD for Expensify/eslint-config-expensify#36 and #6174

[marcaaron]

Looks good. Went about 25% in and just wanted to ask about a few higher level things before continuing.

[marcaaron]

NAB, could maybe opt to be more explicit here vs. depending propTypes? We are creating a dependency where changing the propTypes will have a side effect of altering the pickerProps and not sure if that will be obvious to anyone modifying this (also might make zero difference so 🤷).

[roryabraham]

I feel like propTypes is meant to be the main source of truth for the prop types for a component so I personally feel like this pattern is fine. Open to other ideas too, but it feels odd/unnecessary to duplicate all the props in the _.omit as they're already declared in propTypes

[marcaaron]

Ok, I will just leave you with the thought where someone does not remember to add something to propTypes and unintentionally passes that prop in pickerProps. Again, might not be a problem, but the way to prevent it before was clearer i.e. anything not de-structured would be passed and we are changing this to anything not included in propTypes will be passed - which just feels less obvious.

[marcaaron]

Seeing this again and maybe it's better to pick out which ones we do want to include in inputProps?

[roryabraham]

So the biggest thing it seems we need to agree upon is the pattern to follow for components that use some props, and pass others via a prop spread to a child component. And here are the options as I see them:

1. Suppress the eslint rule and use the spread operator to omit the props we need to use in the parent

   // eslint-disable-next-line react/destructuring-assignment
   const {
       someProp,
       someOtherProp,
       ...propsToDrill,
   } = props;
   
   return (
       <InnerComponent
            // eslint-disable-next-line react/jsx-props-no-spreading
            {...propsToDrill}
            someValue={someProp ? 'a' : 'b'}
            someOtherValue={10 * someOtherProp}
       >
   );

2. Explicitly omit the props we need to use in the parent

   const propsToDrill = _.omit(props, ['someProp', 'someOtherProp']);
   return (
       <InnerComponent
            // eslint-disable-next-line react/jsx-props-no-spreading
            {...propsToDrill}
            someValue={props.someProp ? 'a' : 'b'}
            someOtherValue={10 * props.someOtherProp}
       >
   );

3. Explicitly declare the set of props we will drill to the child

   return (
       <InnerComponent
            someValue={props.someProp ? 'a' : 'b'}
            someOtherValue={10 * props.someOtherProp}
            someChildProp={props.someChildProp}
            someOtherChildProp={props.someOtherChildProp}
       >
   );

So each of these approaches have pros and cons. While 3 at first seems like it would be the most ideal, it's not very practical because it doesn't allow us to define wrapper components that take arbitrary other props and drill those to the wrapped child. So every prop that we could possibly want to pass through to a Button or Text would need to be declared in the ExpensiButton or ExpensiText, respectively, which is not great.

So between 1 and 2, I think I prefer 2 because it's a bit more explicit, and more importantly our props used in the parent component get to retain their props. prefix.

Thoughts?

[marcaaron]

Agree that 2 is the best option, but also was thinking we could use _.pick() instead of _.omit() so you still have to be a bit more explicit, but maybe it's fine to be passive here (I'm mostly worried about using the keys from propTypes). NAB for me really just came to mind :)

[roryabraham]

Taking this off hold, should be ready for review now. Thanks!

[OSBotify]

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

[MonilBhavsar]

Unable to load preferences page due to issue in LocalePicker

Please see #6267

[OSBotify]

🚀 Deployed to staging by @marcaaron in version: 1.1.14-5 🚀

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

[OSBotify]

🚀 Deployed to production by @AndrewGable in version: 1.1.15-15 🚀

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

[mananjadhav]

I am tagging this PR to highlight an issue fixed here. All conditions in ternary expressions or left-hand operands on conditional renders, should be boolean. This PR is one of the PRs that uses conditional render with string operands, hence I am tagging it here for the contributors to check.

We've also updated the item in the checklist with this PR to avoid this issue in the future.