[HOLD for payment 2023-04-03] [$4000] New Lint Rule: onyx-props-must-have default

[roryabraham]

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!

---

Action Performed:

1. Wrap a component with the withOnyx HOC
2. Make the associated prop on the component required in propTypes – do not provide a default.

Expected Result:

The lint rule should catch this and insist than any props originating from withOnyx be optional and provide a default.

Actual Result:

n/a – nothing happens right now

Workaround:

n/a

Issue reported by: @roryabraham
Slack conversation: https://expensify.slack.com/archives/C01GTK53T8Q/p1673645106127709?thread_ts=1673641691.365339&cid=C01GTK53T8Q

View all open jobs on GitHub

Upwork Automation - Do Not Edit

• Upwork Job URL: https://www.upwork.com/jobs/~0188c3bb2e4ac48bf8
• Upwork Job ID: 1614039228269498368
• Last Price Increase: 2023-02-03

[melvin-bot]

@roryabraham the AutoAssignerTriage label has been deprecated. Please use Bug for issue triage. See https://stackoverflowteams.com/c/expensify/questions/14418 for more details.

[melvin-bot]

Job added to Upwork: https://www.upwork.com/jobs/~0188c3bb2e4ac48bf8

[melvin-bot]

Triggered auto assignment to @tjferriss (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

[melvin-bot]

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

[melvin-bot]

Triggered auto assignment to @grgia (External), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

[ahmdshrif]

Proposal:

We should create a custom linter to check the following:

1. The current node and make sure it's using the withOnyx HOC.
2. Get the props from both class and function components in the WrappedComponent.
3. Loop through every prop passed to withOnyx and make sure that it is optional in the WrappedComponent's propTypes and has a default value.

To accomplish this, we will:

1. Create a new ESLint rule, withOnyx-props. in eslint-config-expensify/eslint-plugin-expensify
2. add // 'rulesdir/with-onyx-props': 'error', to eslint-config-expensify/rules/expensify.js

Linter code :

const PropTypes = require('prop-types');

module.exports = {
  create: context => ({
    CallExpression(node) {
      if (node.callee.name !== 'withOnyx') {
        return;
      }

      const wrappedComponent = node.arguments[0];
      let propTypes, defaultProps, componentName;

      if (wrappedComponent.type === "ClassExpression") {
        propTypes = wrappedComponent.body.body.find(
          (m) => m.key.name === "propTypes"
        );
        defaultProps = wrappedComponent.body.body.find(
          (m) => m.key.name === "defaultProps"
        );
        componentName = wrappedComponent.id.name;
      } else {
        propTypes = wrappedComponent.propTypes;
        defaultProps = wrappedComponent.defaultProps;
        componentName = wrappedComponent.displayName || wrappedComponent.name;
      }

      if (node.arguments.length === 0) {
        context.report({
          node,
          message: `withOnyx must be passed a component`
        });
      }

      node.arguments[1].properties.forEach(({ key, value }) => {
        if (!propTypes[key.name]) {
          context.report({
            node,
            message: `Prop '${key.name}' is not defined in propTypes for component ${componentName}`
          });
          return;
        }

        if (!defaultProps[key.name]) {
          context.report({
            node,
            message: `Prop '${key.name}' has no default value for component ${componentName}`
          });
        }

 if (propTypes[key.name].isRequired ) {
          context.report({
            node,
            message: `Prop '${key.name}' is required in propTypes for component ${componentName} `
          });
      });
    },
  }),
};

[Prince-Mendiratta]

Proposal

Background

It is not safe to assume that props provided by withOnyx can be required. Thus, we need to create a eslint rule that will report if any of the props passed to withOnyx either:

• is required in propTypes
• does not have a default value declared in defaultProps.

Solution

We need to create a rule in eslint-config-expensify . The rule I have created will check and report the following things that will be checked if the component is wrapped with the withOnyx HOC.

• Prop passed to withOnyx and has the .isRequired property set.
• Prop passed to withOnyx and does not have a default value set in defaultProps.
• Prop passed to withOnyx and has not been declared in propTypes.
• Component does not have propTypes declared.
• Component does not have defaultProps declared.

My solution covers for all the occurrences of this issue in the repository, including the ones mentioned in the Slack thread

Workflow

The steps with ensures in bold signify that the check is reported as well.

1. The rule checks for existence of the withOnyx call.
2. The rule checks and ensures that the withOnyx HOC is passed a parameter.
3. The rule obtains the top level Program node from the AST.
4. The rule checks, ensures and obtains the top level propTypes declaration node from the Program AST.
5. The rule checks, ensures and obtains the top level defaultProps node from the Program AST.
6. The rule starts iterating through all the props passed to withOnyx and for each prop, does the following:
   • Check and ensure if the prop has been declared in propTypes.
   • Check and ensure that the prop in propTypes does not have .isRequired.
   • Check and ensure that the prop has a default value in defaultProps.

Rule Impact on current codebase

To implement this standard in the current codebase, once the PR for the fix has been merged, the following needs to be done-

1. Update the package eslint-config-expensify to latest version.
2. Run npm lint to get the list where this rule is violated.
3. Depending on the error reported, customise the prop values.

Code

For better visibility and readability, have commited the code to my fork.
Prince-Mendiratta/eslint-config-expensify@58baa29

Tests

To test for the various cases covered by this rule, I've added the test rules to
https://github.com/Prince-Mendiratta/eslint-config-expensify/blob/props/eslint-plugin-expensify/tests/onyx-props-must-have-default.test.js

[bernhardoj]

I don't think we can cover cases where the value is imported from another file.

1. Prop types/default props imported from other files
2. Object (from imported file) destructuring inside default props/prop types

[mollfpr]

@ahmdshrif @Prince-Mendiratta How do you guys test the local eslint config? My npm is doesn't install the package that required from eslint-config-expensify.

[Prince-Mendiratta]

@mollfpr Hey! To test out the rule,

1. Navigate to cloned Expensify/App repository.
2. Run npm install git+https://github.com/Prince-Mendiratta/eslint-config-expensify.git#props.
3. Run npm run lint

If you personally want to make any changes and see their effect, you can do so by directly modifying the source code in node_modules/eslint-config-expensify. This will reflect the changes the next time you run lint.

[mollfpr]

Thanks a lot @Prince-Mendiratta!

[Prince-Mendiratta]

After checking out all the reported rules, a couple of edge cases I'd like to mention -

Imports prop types and default props from an external file.

Something like this -
https://github.com/Expensify/App/blob/main/src/components/KYCWall/BaseKYCWall.js#L12
Violating components -

• https://github.com/Expensify/App/blob/main/src/components/KYCWall/BaseKYCWall.js
• https://github.com/Expensify/App/blob/main/src/components/ScreenWrapper/index.js
• https://github.com/Expensify/App/blob/main/src/components/WalletStatementModal/index.js
• https://github.com/Expensify/App/blob/main/src/pages/settings/Payments/PaymentsPage/BasePaymentsPage.js

withOnyx() call has a key as an array

App/src/components/createOnyxContext.js

Lines 23 to 27 in 7ca2979

	const ProviderWithOnyx = withOnyx({
	[onyxKeyName]: {
	key: onyxKeyName,
	},
	})(Provider);

This is the only occurrence of this issue.

Prop types and Default props have been declared but have a different variable name

App/src/components/withCurrentUserPersonalDetails.js

Lines 8 to 14 in 7ca2979

	const withCurrentUserPersonalDetailsPropTypes = {
	currentUserPersonalDetails: personalDetailsPropType,
	};
	const withCurrentUserPersonalDetailsDefaultProps = {
	currentUserPersonalDetails: {},
	};

The rule specifically triggers on the variable name propTypes and defaultProps.
Other occurrences
- https://github.com/Expensify/App/blob/main/src/pages/workspace/withPolicy.js

Prop types declared inside a function

https://github.com/Expensify/App/blob/main/src/pages/home/report/withReportOrNavigateHome.js#L10-L26
The rule triggers on the prop types and default props declared at the top level. If they are inside a function, they won't trigger and report false errors.
Other occurrences
- https://github.com/Expensify/App/blob/main/src/pages/workspace/withPolicy.js

Solution

These are the few false negatives I was able to find by initially combing through the reported errors by the config. To fix these, we can take either of the following approaches:

Ignore the rule for specific components

This can be used for Problem 2 and Problem 4.

Standardize coding style according to expectations set by rule.

This can be done for Problem 1 and Problem 3

Read prop types from external file

The rule can be modified to get the prop variables from Imported files, if they are not declared inside the component itself. This can be a nested condition such that the rule first checks for prop variables declared inside the component and if not found, check for externally imported declaration of prop variables. However, we should decide on a fix pattern for this.

[mollfpr]

Thanks for the proposal @ahmdshrif @Prince-Mendiratta!

---

@ahmdshrif I can't run your code, I got the above error after creating the rule suggested. Can you update your solution?

[mollfpr]

Ignore the rule for specific components

@Prince-Mendiratta I think the rule should be applied too for HOC that use withOnyx. Is this something that we can't do?

Read prop types from external file

Could you update your solution to work with this? We have a lot of components that use imported propTypes/defaultProps for DRY's sake.

Standardize coding style according to expectations set by rule.

I do not understand what we should do here.

[Prince-Mendiratta]

Ignore the rule for specific components

@Prince-Mendiratta I think the rule should be applied too for HOC that use withOnyx. Is this something that we can't do?

These are the only HOCs that make use of withOnyx, correct? I'll check if I can modify the same rule to take care of these as well or if we would need a separate rule for this.

• https://github.com/Expensify/App/blob/main/src/pages/workspace/withPolicy.js
• https://github.com/Expensify/App/blob/main/src/pages/home/report/withReportOrNavigateHome.js
• https://github.com/Expensify/App/blob/main/src/components/withCurrentUserPersonalDetails.js

Read prop types from external file

Could you update your solution to work with this? We have a lot of components that use imported propTypes/defaultProps for DRY's sake.

Sure! I'll create update and ping you here once I'm done.

Standardize coding style according to expectations set by rule.

I do not understand what we should do here.

Had something else in mind, doesn't look relevant anymore. please ignore this haha

[mollfpr]

Just checking in, do you have an update on your proposal? @Prince-Mendiratta

[Prince-Mendiratta]

Hi, @mollfpr. I've been working on it, trying to work with externally declared propTypes turned out to be more complex than I expected, I'm exploring all the possible ways to go ahead with this to get the maximum coverage of components in an efficient manner. I'm trying to create a single rule that'll work for all components instead of a different rule for HOCs. Rest assured, I've made much progress and will share an updated proposal soon!

[mollfpr]

Thanks for the update @Prince-Mendiratta!

[grgia]

Not overdue, I believe PR is in the works

[mollfpr]

Heyy @Prince-Mendiratta any update on the work in progress PR?

[Prince-Mendiratta]

PR is ready for review!

Sorry for the delay and thank you for your patience.

cc @mollfpr @grgia @roryabraham

[mollfpr]

Well done @Prince-Mendiratta! I'll review it tonight!

[MelvinBot]

@tjferriss, @mollfpr, @grgia, @Prince-Mendiratta Whoops! This issue is 2 days overdue. Let's get this updated quick!

[tjferriss]

@mollfpr have you had a chance to review the PR?

[mollfpr]

@tjferriss Yup, just need to do the final review!

[MelvinBot]

@tjferriss, @mollfpr, @grgia, @Prince-Mendiratta Whoops! This issue is 2 days overdue. Let's get this updated quick!

[Prince-Mendiratta]

PR is in under testing phase.

[Prince-Mendiratta]

With the rule merged, sent in a disclaimer about the update on slack!

[MelvinBot]

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

[MelvinBot]

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

• [PropTypes / Default Props] Modify components to be compatible with new Onyx Lint rule #15590

If no regressions arise, payment will be issued on 2023-04-03. 🎊

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 Not Applicable
• Contributor that fixed the issue @Prince-Mendiratta
• Contributor+ that helped on the issue and/or PR @mollfpr

As a reminder, here are the bonuses/penalties that should be applied for any External issue:

• Merged PR within 3 business days of assignment - 50% bonus
• Merged PR more than 9 business days after assignment - 50% penalty

[Prince-Mendiratta]

Hello, while this is under the regression phase, I'd like to request a bounty reconsidering, considering the scope and complexity of the issue. Can you please discuss this internally and let me know if it'd be possible? Thanks.

cc @grgia @mollfpr @roryabraham

[tjferriss]

@Prince-Mendiratta thanks for sharing the feedback. I shared your request internally and ultimately we feel the current $4k bounty is fair for the work completed.

[Prince-Mendiratta]

Noted, thank you @tjferriss!

[tjferriss]

Payments have been made!