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

Jump to bottom

[New] require-default-props: add option functions #3249

Merged
merged 2 commits into from Apr 27, 2022
Merged

[New] require-default-props: add option functions #3249

merged 2 commits into from Apr 27, 2022

Conversation

nix6839
Copy link
Contributor

@nix6839 nix6839 commented Mar 20, 2022 •
edited

Closes #2396

Todo

  • Update function comments.
  • Add more tests.
  • Update docs.
  • Update changelog.

@nix6839 nix6839 changed the title [New] requireDefaultProps: add option initArgForFnComponents [New] require-default-props: add option initArgForFnComponents Mar 20, 2022
ljharb
ljharb requested changes Mar 20, 2022
View reviewed changes
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
lib/util/ast.js Outdated Show resolved Hide resolved
lib/util/ast.js Outdated Show resolved Hide resolved
tests/lib/rules/require-default-props.js Show resolved Hide resolved
@nix6839 nix6839 marked this pull request as ready for review March 21, 2022 02:52
@nix6839 nix6839 marked this pull request as draft March 21, 2022 02:52
@nix6839 nix6839 changed the title [New] require-default-props: add option initArgForFnComponents [New] require-default-props: add option assignDefaultToObjectForFc Mar 21, 2022
@nix6839 nix6839 requested a review from ljharb March 21, 2022 10:52
@nix6839 nix6839 marked this pull request as ready for review March 21, 2022 10:55
@nix6839
Copy link
Contributor Author

nix6839 commented Mar 21, 2022

Should we also handle forbidDefaultForRequired && assignDefaultToObjectForFc?

ljharb
ljharb reviewed Mar 21, 2022
View reviewed changes
Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point of this option is to allow default arguments on props to "count" alongside defaultProps, right? It wouldn't force using default arguments, it would just let them satisfy the requirement.

What about allowDefaultPropArguments? It can't apply except in an SFC, so i don't think that needs calling out, and the = isn't assignment, and it's not "to an object" either.

docs/rules/require-default-props.md Outdated Show resolved Hide resolved
@nix6839
Copy link
Contributor Author

nix6839 commented Mar 22, 2022 •
edited

The point of this option is to allow default arguments on props to "count" alongside defaultProps, right? It wouldn't force using default arguments, it would just let them satisfy the requirement.

What about allowDefaultPropArguments? It can't apply except in an SFC, so i don't think that needs calling out, and the = isn't assignment, and it's not "to an object" either.

No alongside, only use default arguments (this means that forbid to defaultProps). Other than that, I have the same thoughts as you.

Therefore, what about forceDefaultPropArguments, which means to force more allowDefaultPropArguments?

@ljharb
Copy link
Member

ljharb commented Mar 22, 2022

If that’s what it does, then that’s a better name for the option.

@nix6839 nix6839 changed the title [New] require-default-props: add option assignDefaultToObjectForFc [New] require-default-props: add option forceDefaultPropArguments Mar 22, 2022
@nix6839
Copy link
Contributor Author

nix6839 commented Mar 22, 2022

Is there anything I can do?

@nix6839 nix6839 requested a review from ljharb March 22, 2022 07:52
@nix6839
Copy link
Contributor Author

nix6839 commented Mar 23, 2022 •
edited

There is an error when ignorePropsValidation and ignoreUnusedPropTypesValidation are false but I don't know what they do.

ljharb
ljharb reviewed Mar 26, 2022
View reviewed changes
docs/rules/require-default-props.md Outdated Show resolved Hide resolved
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
lib/util/ast.js Show resolved Hide resolved
tests/lib/rules/require-default-props.js Outdated Show resolved Hide resolved
tests/lib/rules/require-default-props.js Outdated Show resolved Hide resolved
tests/lib/rules/require-default-props.js Outdated Show resolved Hide resolved
@nix6839 nix6839 changed the title [New] require-default-props: add option forceDefaultPropArguments [New] require-default-props: add option functions Mar 27, 2022
@nix6839 nix6839 requested a review from ljharb March 27, 2022 05:48
@codecov
Copy link

codecov bot commented Apr 23, 2022 •
edited

Codecov Report

Merging #3249 (e3af018) into master (ef733fd) will increase coverage by 0.00%.
The diff coverage is 97.77%.

❗ Current head e3af018 differs from pull request most recent head 24621e4. Consider uploading reports for the commit 24621e4 to get more accurate results

@@           Coverage Diff           @@
##           master    #3249   +/-   ##
=======================================
  Coverage   97.70%   97.71%           
=======================================
  Files         123      123           
  Lines        8716     8750   +34     
  Branches     3160     3173   +13     
=======================================
+ Hits         8516     8550   +34     
  Misses        200      200
Impacted Files Coverage Δ
lib/rules/require-default-props.js 98.46% <97.67%> (-1.54%) ⬇️
lib/util/ast.js 96.39% <100.00%> (+0.55%) ⬆️

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 ef733fd...24621e4. Read the comment docs.

@nix6839

This comment was marked as resolved.

Sign in to view
@nix6839
Copy link
Contributor Author

nix6839 commented Apr 24, 2022

Two failed coverages appear to need no testing. Should I guard it?

ljharb
ljharb requested changes Apr 26, 2022
View reviewed changes
docs/rules/require-default-props.md Outdated Show resolved Hide resolved
docs/rules/require-default-props.md Outdated Show resolved Hide resolved
docs/rules/require-default-props.md Outdated Show resolved Hide resolved
docs/rules/require-default-props.md Outdated Show resolved Hide resolved
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
lib/rules/require-default-props.js Outdated Show resolved Hide resolved
@nix6839 nix6839 requested a review from ljharb April 27, 2022 00:03
@ljharb ljharb merged commit 24621e4 into jsx-eslint:master Apr 27, 2022
@sheiman-pavlo
Copy link

When we can expect this to be tagged and released?

@ljharb
Copy link
Member

ljharb commented May 18, 2022

v7.30.0 is released.

@ljharb ljharb mentioned this pull request Jul 20, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ljharb ljharb ljharb approved these changes

Assignees
No one assigned
Labels
enhancement
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

defaultProps rule to be deprecated on function components
3 participants
@nix6839 @ljharb @sheiman-pavlo