[HOLD for payment 2024-01-05] [$500] Prohibit the use of defaultProps in ts functional components

[roryabraham]

Problem

Our TypeScript guidelines state that functional components should use prop destructuring instead of defaultProps, but it's still possible for developers who don't know that to try and use defaultProps. In the worst case, that means we'll have inconsistent patterns for default props in our TS functional components. In a more typical case, the development cycle is slowed because we have to request changes during review and wait for the developer to make those changes.

Solution

Set up an ESLint rule that prohibits the use of defaultProps in TypeScript functional components, and directs developers to use prop destructuring instead. To be clear, this means that defaultProps should be allowed:

• in JavaScript files
• in TS files that contain class components (there should not be many of these, but some may exist)
  • it's not ideal if we have to suppress the linter to make this case work, but it's an option.

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~01e2d42d1ab75ab97a
• Upwork Job ID: 1735845538188914688
• Last Price Increase: 2023-12-20
• Automatic offers:
• alitoshmatov | Reviewer | 28063449
• ishpaul777 | Contributor | 28063450

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~01e2d42d1ab75ab97a

[melvin-bot]

Triggered auto assignment to @abekkala (NewFeature), see https://stackoverflowteams.com/c/expensify/questions/14418#:~:text=BugZero%20process%20steps%20for%20feature%20requests for more details.

[melvin-bot]

Triggered auto assignment to Contributor-plus team member for initial proposal review - @alitoshmatov (External)

[melvin-bot]

Upwork job price has been updated to $250

[roryabraham]

Adjusting the bounty for this to $250

[samilabud]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Prohibit the use of defaultProps in ts functional components

What is the root cause of that problem?

Our TypeScript guidelines state that functional components should use prop destructuring instead of defaultProps.

What changes do you think we should make in order to solve the problem?

We should modify the file .eslintrc.js , adding a new rule like:

rules: {
    'no-default-props': ['error', { allowDefaultProps: true }],
  },

Then we can create a file/rule called eslint-rules/no-default-props.js with a content like:

module.exports = {
  meta: {
    type: 'problem',
    docs: {
      description: 'Prohibit the use of defaultProps in TypeScript/JS functional components.',
    },
    schema: [
      {
        type: 'object',
        properties: {
          allowDefaultProps: {
            type: 'boolean',
            default: false,
          },
        },
        additionalProperties: false,
      },
    ],
  },
  create: function (context) {
    const allowDefaultProps = context.options[0]?.allowDefaultProps || false;

    return {
      FunctionDeclaration(node) {
        if (!allowDefaultProps && node.returnType && node.returnType.typeAnnotation.type === 'TSFunctionType') {
          const defaultProps = node.body.body.find(
            (item) =>
              item.type === 'TSPropertySignature' &&
              item.key.name === 'defaultProps' &&
              item.typeAnnotation &&
              item.typeAnnotation.type === 'TSTypeAnnotation'
          );

          if (defaultProps) {
            context.report({
              node: defaultProps,
              message: 'Use prop destructuring instead of defaultProps in TypeScript functional components.',
            });
          }
        }
      },
    };
  },
};

Finally, we should include our rules in ESLint configuration file like:

overrides: [
  {
    files: ['*.ts', '*.tsx'],
    rules:{
     '@typescript-eslint/no-default-props': ['error', { allowDefaultProps: true }],
    'eslint-rules/no-default-props': ['error', { allowDefaultProps: true }],
    },
 }
  ],

[ishpaul777]

Proposal

Please re-state the problem that we are trying to solve in this issue.

Prohibit the use of defaultProps in ts functional components

What is the root cause of that problem?

N/a, not a bug more of dev experience improvement

What changes do you think we should make in order to solve the problem?

At present we suppressed the linter rule for default props 'react/require-default-props` for typescript files here, we just need to enable the rule and set option 'functions' to 'defaultArguments'.

'react/require-default-props': ['error', {functions: 'defaultArguments'}]`

Note: Enabling this would mean if there are optional props without defaultArguments linter will throw error, we need to provide "undefined" as default

[melvin-bot]

📣 @alitoshmatov 🎉 An offer has been automatically sent to your Upwork account for the Reviewer role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job

[melvin-bot]

📣 @ishpaul777 🎉 An offer has been automatically sent to your Upwork account for the Contributor role 🎉 Thanks for contributing to the Expensify app!

Offer link
Upwork job
Please accept the offer and leave a comment on the Github issue letting us know when we can expect a PR to be ready for review 🧑‍💻
Keep in mind: Code of Conduct | Contributing 📖

[ishpaul777]

Thanks for assigning, I am working on the PR now, will raise shortly

[roryabraham]

eslint-config-expensify PR merged, back to E/App PR now

[ishpaul777]

I'll open a Pr tomorrow (Monday).

[ishpaul777]

PR is reday for review @roryabraham

[roryabraham]

PR merged

[roryabraham]

I did not use any C+ services in the course of this issue, so I'm removing @alitoshmatov's assignment here

[melvin-bot]

Reviewing label has been removed, please complete the "BugZero Checklist".

[melvin-bot]

The solution for this issue has been 🚀 deployed to production 🚀 in version 1.4.19-2 and is now subject to a 7-day regression period 📆. Here is the list of pull requests that resolve this issue:

• bump the eslint-config-version #33565

If no regressions arise, payment will be issued on 2024-01-05. 🎊

After the hold period is over and BZ checklist items are completed, please complete any of the applicable payments for this issue, and check them off once done.

• External issue reporter
• Contributor that fixed the issue
• Contributor+ that helped on the issue and/or PR

For reference, here are some details about the assignees on this issue:

• @ishpaul777 requires payment automatic offer (Contributor)

[melvin-bot]

BugZero Checklist: The PR adding this new feature has been merged! The following checklist (instructions) will need to be completed before the issue can be closed:

• [@ishpaul777] Please propose regression test steps to ensure the new feature will work correctly on production in further releases.
• [@strepanier03 / @abekkala] Link the GH issue for creating/updating the regression test once above steps have been agreed upon.

[roryabraham]

No regression tests needed here

[abekkala]

PAYMENTS FOR JAN 05

• @ishpaul777 Fix $500 Offer link
• @alitoshmatov PR Review $500 Offer link

[alitoshmatov]

@abekkala no need payment for me since I did not review the PR

[abekkala]

@alitoshmatov ah yes, I do see that now. Thank you!

[abekkala]

@ishpaul777 $500 payment sent and contract ended - thank you! 🎉