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] checkContextObjects option in display-name #3529

Merged
merged 1 commit into from
Feb 25, 2023

Conversation

JulesBlm
Copy link
Contributor

I'm proposing adding a new rule that checks if the displayName property is set for context objects, see Context.displayName in the React docs.

With multiple Context providers in an app, consistently setting a displayName makes it easy to see which is which in the React dev tools.

Some things to consider

  • Should it perhaps be part of the display-name rule instead of a separate rule?
  • Does it need more tests?
  • Does this checking approach make sense? (This is my first time writing an ESLint rule, so I wouldn't be surprised if this is not the best way)
  • Does it need to check more cases?

@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Merging #3529 (725d4c7) into master (abb4871) will increase coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 725d4c7 differs from pull request most recent head a684ae1. Consider uploading reports for the commit a684ae1 to get more accurate results

@@            Coverage Diff             @@
##           master    #3529      +/-   ##
==========================================
+ Coverage   97.58%   97.59%   +0.01%     
==========================================
  Files         131      132       +1     
  Lines        9236     9289      +53     
  Branches     3358     3394      +36     
==========================================
+ Hits         9013     9066      +53     
  Misses        223      223              
Impacted Files Coverage Δ
lib/rules/display-name.js 98.57% <100.00%> (+0.23%) ⬆️
lib/util/isCreateContext.js 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

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.

Overall, this seems reasonable. Should this maybe be an option on the display-name rule?

tests/lib/rules/context-display-name.js Outdated Show resolved Hide resolved
@JulesBlm
Copy link
Contributor Author

Overall, this seems reasonable. Should this maybe be an option on the display-name rule?

I think that makes sense, the intention is the same. Main difference is that createContext does not create a component directly but a context object containing two components.
I looked at the implementation of display-name and it makes heavy use of the Components utility, which can't be used for createContext. I thought it would be easier and cleaner to make a separate rule for it but I'm not married to the idea of it being a separate rule. What would your preference be?

@ljharb
Copy link
Member

ljharb commented Feb 13, 2023

while that's a fair point, since the rule is "display name" and not "component display name" i think it's worth figuring out the complexity inside the rule, rather than adding complexity for users.

@JulesBlm
Copy link
Contributor Author

while that's a fair point, since the rule is "display name" and not "component display name" i think it's worth figuring out the complexity inside the rule, rather than adding complexity for users.

Alright makes sense, I turned it into a checkContextObjects option in display-name. As far as I can tell, the logic for detecting component isn't of much use for detecting context definitions so I chose not make use of the Components utility for it.

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.

There's a few lines missing test coverage; otherwise it looks pretty great!

lib/rules/display-name.js Outdated Show resolved Hide resolved
@JulesBlm
Copy link
Contributor Author

There's a few lines missing test coverage; otherwise it looks pretty great!

Thanks! I noticed I forgot to enable the new flag for the new valid tests so I added that. I added few more tests too, some formatting fixes and moved the React version check.

I'm a little confused how to increase the test coverage. What would need to be done to meet the threshold? Test the isCreateContext util?

@JulesBlm JulesBlm changed the title [New] context-display-name [New] checkContextObjects option in display-name Feb 14, 2023
lib/rules/display-name.js Outdated Show resolved Hide resolved
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.

Looks good overall

lib/rules/display-name.js Outdated Show resolved Hide resolved
@JulesBlm
Copy link
Contributor Author

Hey, is there anything I can do to move this PR forward?

@ljharb
Copy link
Member

ljharb commented Feb 24, 2023

@JulesBlm sorry for the delayed reply; i replied to your comment above and will review today.

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.

This looks great - I'll land this later tonight.

@ljharb ljharb merged commit a684ae1 into jsx-eslint:master Feb 25, 2023
@JulesBlm
Copy link
Contributor Author

Awesome thanks for your review @ljharb!

@christianvuerings
Copy link

@ljharb Would you be able to push out a release with this feature?

@ljharb
Copy link
Member

ljharb commented Jul 19, 2023

v7.33.0 is released.

@christianvuerings
Copy link

@ljharb https://www.npmjs.com/package/eslint-plugin-react still shows 7.32.2 as the latest version. Did anything go wrong during the npm publish process?
image

@ljharb
Copy link
Member

ljharb commented Jul 20, 2023

huh, maybe so, let me check

@ljharb
Copy link
Member

ljharb commented Jul 20, 2023

ah thanks, published it manualy

@christianvuerings
Copy link

@ljharb that worked, thank you!

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