Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[New] function-component-definition: replace var by const in certain situations #3248

Merged
merged 1 commit into from Apr 20, 2022

Conversation

JohnBerd
Copy link
Contributor

@JohnBerd JohnBerd commented Mar 20, 2022

Thank you for your amazing job, this rule is almost perfect but something is missing to me.

var is almost not used since ES6 was introduced and I am not able to find any React documentation showing a functional component using var instead of const.

fix #3152

…rtain situations

Signed-off-by: Xavier Le Cunff <xavier@MacBook-Pro-de-Xavier.local>
Co-authored-by: Xavier Le Cunff <xavier@MacBook-Pro-de-Xavier.local>
Co-authored-by: Simeon Cheeseman <simeon@tablecheck.com>
@JohnBerd JohnBerd changed the title Master [FIX] function-component-definition: replace var by const Mar 20, 2022
Copy link
Member

@ljharb ljharb left a comment

var is still quite used.

docs/rules/function-component-definition.md Outdated Show resolved Hide resolved
lib/rules/function-component-definition.js Outdated Show resolved Hide resolved
@ljharb
Copy link
Member

ljharb commented Mar 20, 2022

The linked issue specifically says:

if the component is using jsx, or if the file is already using const.

In only those cases, should it autofix to const. In any other case, it must autofix to var, otherwise it’s a breaking change.

@JohnBerd
Copy link
Contributor Author

JohnBerd commented Mar 20, 2022

The linked issue specifically says:

if the component is using jsx, or if the file is already using const.

In only those cases, should it autofix to const. In any other case, it must autofix to var, otherwise it’s a breaking change.

Thank you for the review and your time, I agree with you about using jsx.
How can we check if the file is already using const if it is a new one?

@ljharb
Copy link
Member

ljharb commented Mar 20, 2022

We could add a visitor for const/let declarations/jsx elements, and save a state flag; and then on program exit, we could do the autofixes as either var or const depending.

@JohnBerd
Copy link
Contributor Author

JohnBerd commented Mar 20, 2022

We could add a visitor for const/let declarations/jsx elements, and save a state flag; and then on program exit, we could do the autofixes as either var or const depending.

Unfortunately I have not enough skills for implementing this :(
I tried implementing the tool isJSX but it doesn't seem to recognize which functions return JSX in the unit tests.

Should I close this PR?

@ljharb
Copy link
Member

ljharb commented Mar 20, 2022

I’m suggesting the rule add visitors for const, let, and jsx nodes - then eslint finds them for us.

@SimeonC
Copy link
Contributor

SimeonC commented Mar 23, 2022

As a different option - could we add a config option to use var or const or let rather than hard code it? That would pass the problem onto the user of the rule rather than the library and wouldn't be breaking.

@JohnBerd I've just done something similar in another project so I'll give you a code snippet here. (I could make another PR if you'd rather I take it over?)

Replace this;

return {
      FunctionDeclaration(node) { validate(node, 'function-declaration'); },
      ArrowFunctionExpression(node) { validate(node, 'arrow-function'); },
      FunctionExpression(node) { validate(node, 'function-expression'); },
    };

with something like this;

const validatePairs = [];
let hasEs6OrJsx = false;
return {
      FunctionDeclaration(node) { validatePairs.push([node, 'function-declaration']); },
      ArrowFunctionExpression(node) { validatePairs.push([node, 'arrow-function']); },
      FunctionExpression(node) { validatePairs.push([node, 'function-expression']); },
      VariableDeclaration(node) {
        hasEs6OrJsx = hasEs6OrJsx || node.kind === "const" || node.kind === "let" 
      },
      JSXElement(node) {
        hasEs6OrJsx = true;
      },
      'Program:exit'() {
         validatePairs.forEach(pair => validate(pair[0], pair[1], hasEs6OrJsx));
      }
    };

@JohnBerd
Copy link
Contributor Author

JohnBerd commented Mar 23, 2022

Thank you so much @SimeonC

I would be pleased If you can create another PR for this.
I really need this feature but I don't have too much programming skills

@ljharb
Copy link
Member

ljharb commented Mar 23, 2022

@SimeonC no, we can’t. Rule options can’t ever just affect autofixing.

@JohnBerd please don’t suggest making another PR; if someone has changes they want to make, they can post a link to a branch here and I’ll pull them into this one.

@SimeonC
Copy link
Contributor

SimeonC commented Mar 23, 2022

@JohnBerd please don’t suggest making another PR; if someone has changes they want to make, they can post a link to a branch here and I’ll pull them into this one.

OK, I'll create a branch with the suggested changes and link it here tomorrow.

@SimeonC no, we can’t. Rule options can’t ever just affect autofixing.

This will be my first time contributing to an eslint plugin so I'm a bit ignorant of any conventions. What's the reasoning for this restriction?

@ljharb
Copy link
Member

ljharb commented Mar 23, 2022

Rule options that do nothing by default (since autofixing isn’t the default) are confusing.

@SimeonC
Copy link
Contributor

SimeonC commented Mar 24, 2022

@ljharb I've implemented the changes on the master branch of my fork here - https://github.com/SimeonC/eslint-plugin-react/tree/master, could you pull the changes into this branch and let me know if there are any other changes needed.

I also updated the tests and prevented the fixer from changing existing var/let/const declarations if they already exist.

@codecov
Copy link

codecov bot commented Apr 15, 2022

Codecov Report

Merging #3248 (fe84e80) into master (18de0a6) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

Current head fe84e80 differs from pull request most recent head 4b4bba9. Consider uploading reports for the commit 4b4bba9 to get more accurate results

@@            Coverage Diff             @@
##           master    #3248      +/-   ##
==========================================
- Coverage   97.69%   97.68%   -0.01%     
==========================================
  Files         122      121       -1     
  Lines        8634     8610      -24     
  Branches     3135     3130       -5     
==========================================
- Hits         8435     8411      -24     
  Misses        199      199              
Impacted Files Coverage Δ
lib/rules/function-component-definition.js 98.83% <100.00%> (+0.30%) ⬆️
lib/rules/prefer-stateless-function.js 86.89% <0.00%> (-0.18%) ⬇️
lib/util/ast.js 95.50% <0.00%> (-0.18%) ⬇️
lib/rules/no-redundant-should-component-update.js 95.83% <0.00%> (-0.17%) ⬇️
lib/rules/no-access-state-in-setstate.js 93.67% <0.00%> (-0.08%) ⬇️
lib/util/defaultProps.js 92.63% <0.00%> (-0.08%) ⬇️
lib/rules/no-deprecated.js 97.97% <0.00%> (-0.06%) ⬇️
lib/util/jsx.js 95.55% <0.00%> (-0.05%) ⬇️
lib/rules/require-optimization.js 98.90% <0.00%> (-0.05%) ⬇️
lib/rules/no-direct-mutation-state.js 98.07% <0.00%> (-0.04%) ⬇️
... and 16 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 18de0a6...4b4bba9. Read the comment docs.

Copy link
Member

@ljharb ljharb left a comment

Looks pretty good - can we include some tests for a var going to a const?

In particular, can we include tests covering these scenarios:

  • a var, assigned to somewhere in the same block, to confirm it's changed to a let
  • a let arrow function, assigned to somewhere in the same block, to confirm it's not changed to a const
  • a var, that's assigned to outside the block in which it's declared but inside the same function as it's declared, to confirm it's not change from a var

lib/rules/function-component-definition.js Show resolved Hide resolved
@SimeonC
Copy link
Contributor

SimeonC commented Apr 18, 2022

Looks pretty good - can we include some tests for a var going to a const?

In particular, can we include tests covering these scenarios:

  • a var, assigned to somewhere in the same block, to confirm it's changed to a let
  • a let arrow function, assigned to somewhere in the same block, to confirm it's not changed to a const
  • a var, that's assigned to outside the block in which it's declared but inside the same function as it's declared, to confirm it's not change from a var

I added some test cases for let in particular, but the code here I wrote specifically ignores any let - var - const conversion. It keeps it the same as what it was. The only time that we set const or var is if we are changing a Function Declaration to a Variable Declaration. When changing function expression to arrow expression or vice versa, we just persist what was already used.

if ((node.type === 'FunctionExpression' || node.type === 'ArrowFunctionExpression') && node.parent.type === 'VariableDeclarator') {
varType = node.parent.parent.kind;
}

@ljharb
Copy link
Member

ljharb commented Apr 19, 2022

That seems like a good strategy - I just want some tests to guarantee it :-)

lib/rules/function-component-definition.js Outdated Show resolved Hide resolved
lib/rules/function-component-definition.js Outdated Show resolved Hide resolved
@SimeonC
Copy link
Contributor

SimeonC commented Apr 20, 2022

Thanks for the review notes - I've added another commit to fix up the issues SimeonC@edccc0f (Also realised I hadn't checked eslint either so there's a bunch of formatter fixes in that commit as well)

@ljharb ljharb changed the title [FIX] function-component-definition: replace var by const [Fix] function-component-definition: replace var by const in certain situations Apr 20, 2022
ljharb
ljharb approved these changes Apr 20, 2022
@ljharb ljharb changed the title [Fix] function-component-definition: replace var by const in certain situations [New] function-component-definition: replace var by const in certain situations Apr 20, 2022
@ljharb ljharb merged commit 4b4bba9 into jsx-eslint:master Apr 20, 2022
243 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants