Lint-rule/prefer-default-argument-for-function-components #33202
Conversation
|
Hey! I see that you made changes to our Form component. Make sure to update the docs in FORMS.md accordingly. Cheers! |
|
@alitoshmatov Please copy/paste the Reviewer Checklist from here into a new comment on this PR and complete it. If you have the K2 extension, you can simply click: [this button] |
| imageStyles, | ||
| iconAdditionalStyles, | ||
| containerStyles, | ||
| source = undefined, |
There was a problem hiding this comment.
Does this rule enforce setting a default values for every optional prop? That's a bummer imo 😒 cc @ishpaul777 @roryabraham
There was a problem hiding this comment.
Indeed seems a little extra work! but, we're applying the same approach for default props in .js components, so I believe it's acceptable.
There was a problem hiding this comment.
That is something we would like to avoid with Typescript, that's why this rule wasn't applied to Ts files before, wdyt @fabioh8010?
There was a problem hiding this comment.
Yes, is unnecessary to set undefined for every optional prop without default values because both JS and TS will already consider that the variable can be undefined, let's not enforce this 🙂
|
Hm ok I see your point @blazejkustra @fabioh8010. It looks like something more like the other proposal on the issue (a custom lint rule) might be needed to disallow the use of @ishpaul777 how do you want to move forward? |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
|
Hi @roryabraham @blazejkustra @fabioh8010 , could you please review the code below and let me know if it makes sense? I've attempted to clarify with comments, but if anything is unclear, please let me know. If the code appears satisfactory, I'll proceed to create a pull request in https://github.com/Expensify/eslint-config-expensify. const _ = require('underscore');
const lodashGet = require('lodash/get');
const message = require('./CONST').MESSAGE.NO_DEFAULT_PROPS;
const {isReactViewFile} = require('./utils');
module.exports = {
create: context => ({
Program(node) {
// Only looking at react files
if (!isReactViewFile(context.getFilename())) {
return;
}
// Infer the component name from the filename
const filenameParts = context.getFilename().split('/');
let componentName = filenameParts[filenameParts.length - 1].split('.')[0];
if (componentName === 'index') {
componentName = filenameParts[filenameParts.length - 2];
}
// Find a root level variable delcaration that matches the component name
const functionComponent = _.find(node.body, n => n.type === 'FunctionDeclaration' && n.id.name === componentName);
if (_.isUndefined(functionComponent)) {
return;
}
// Find the defaultProps assignment
const defaultPropsAssignment = _.find(node.body, n => n.type === 'ExpressionStatement' && lodashGet(n, 'expression.type') === 'AssignmentExpression' && lodashGet(n, 'expression.left.type') === 'MemberExpression' && lodashGet(n, 'expression.left.object.name') === componentName && lodashGet(n, 'expression.left.property.name') === 'defaultProps');
// If we don't have a defaultProps assignment, we're good
if (_.isUndefined(defaultPropsAssignment)) {
return;
}
// report the error
context.report({
node: defaultPropsAssignment,
message,
});
},
}),
};Tests
const RuleTester = require('eslint').RuleTester;
const rule = require('../no-default-props');
const message = require('../CONST').MESSAGE.NO_DEFAULT_PROPS;
const ruleTester = new RuleTester({
parserOptions: {
ecmaVersion: 6,
sourceType: 'module',
},
});
const functionalComponentWithDefaultProps = `
const Test = () => 'Test';
Test.defaultProps = {
test: 'test',
};
`;
const classComponentWithoutDefaultProps = `
class Test extends Component {
render() {
return 'Test';
}
}
`;
const functionalComponentWithoutDefaultProps = `
const Test = () => 'Test';
Test.displayName = 'Test';
`;
const classComponentWithoutDefaultPropsWithDisplayName = `
class Test extends Component {
render() {
return 'Test';
}
}
Test.displayName = 'Test';
`;
ruleTester.run('no-default-props', rule, {
valid: [
{
code: functionalComponentWithoutDefaultProps,
filename: 'src/pages/Test.js',
},
{
code: classComponentWithoutDefaultPropsWithDisplayName,
filename: 'src/pages/Test.js',
},
{
code: functionalComponentWithoutDefaultProps,
filename: 'src/pages/Test/index.js',
},
{
code: classComponentWithoutDefaultPropsWithDisplayName,
filename: 'src/pages/Test.js/index.js',
},
{
code: functionalComponentWithoutDefaultProps,
filename: 'src/pages/Test/index.native.js',
},
{
code: classComponentWithoutDefaultPropsWithDisplayName,
filename: 'src/pages/Test.js/index.android.js',
},
],
invalid: [
{
code: functionalComponentWithDefaultProps,
filename: 'src/pages/Test.js',
errors: [{message}],
},
{
code: classComponentWithoutDefaultProps,
filename: 'src/pages/Test.js',
errors: [{message}],
},
{
code: functionalComponentWithDefaultProps,
filename: 'src/pages/Test/index.js',
errors: [{message}],
},
{
code: classComponentWithoutDefaultProps,
filename: 'src/pages/Test.js/index.js',
errors: [{message}],
},
{
code: functionalComponentWithDefaultProps,
filename: 'src/pages/Test/index.native.js',
errors: [{message}],
},
{
code: classComponentWithoutDefaultProps,
filename: 'src/pages/Test.js/index.android.js',
errors: [{message}],
},
],
}); |
|
Seems like some work is already done, should I review this PR? cc: @roryabraham |
|
@ishpaul777 I'm not experienced in linter rules development, but the code looks good to me! One thing a noticed, here you don't have to check for if (_.isUndefined(defaultPropsAssignment) || _.isUndefined(functionComponent)) { |
|
@roryabraham Can you take a final look at #33202 (comment) and clarify next steps please? |
|
@ishpaul777 that looks good, let's proceed with a eslint-config-expensify PR. Some feedback though – I think we shouldn't rely on furthermore, I'd encourage you to include tests for some additional edge cases:
Thanks for your work on this. I think we may need to adjust the bounty up from $250 back to $500 since the scope changed and we're indeed making a custom lint rule. |
|
@alitoshmatov sit tight for now, but once the E/App PR is ready for review I will want a C+ review on it. Thanks! |
|
Thanks for feedback @roryabraham, i'll modify the rule based on received feedback, closing this PR as the changes are stale now. |
Details
This PR enables ESLint rule that prohibits the use of defaultProps in TypeScript functional components.
General pattern followed for providing default arguments in Compoents.
{}(empty object)() => {}(void function)Fixed Issues
$ #33197
PROPOSAL: #33197 (comment)
Tests
Offline tests
QA Steps
PR Author Checklist
### Fixed Issuessection aboveTestssectionOffline stepssectionQA stepssectiontoggleReportand notonIconClick)myBool && <MyComponent />.src/languages/*files and using the translation methodWaiting for Copylabel for a copy review on the original GH to get the correct copy.STYLE.md) were followedAvatar, I verified the components usingAvatarare working as expected)StyleUtils.getBackgroundAndBorderStyle(theme.componentBG))Avataris modified, I verified thatAvataris working as expected in all cases)ScrollViewcomponent to make it scrollable when more elements are added to the page.mainbranch was merged into this PR after a review, I tested again and verified the outcome was still expected according to theTeststeps.Screenshots/Videos
Android: Native
Android: mWeb Chrome
iOS: Native
iOS: mWeb Safari
MacOS: Chrome / Safari
MacOS: Desktop