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

fix(no-node-access): false positives with props.children #658

Merged

Conversation

sjarva
Copy link
Collaborator

@sjarva sjarva commented Oct 1, 2022

Checks

  • I have read the contributing guidelines.
  • If some rule is added/updated/removed, I've regenerated the rules list (npm run generate:rules-list)
  • If some rule meta info is changed, I've regenerated the plugin shared configs (npm run generate:configs)

Changes

  • checks if the node accessing property (e.g. children, firstChild or previousSibling) is called by props, so if there's a props.children, props.firstChild or props.previousSibling, then that code won't be reported as an error

Context

Closes #386 (at least the most common case, not all of the examples given in that issue).
This PR does not close/fix the deconstructed props case (mentioned by @Belco90 in #386):

function ComponentB({ children }) {
  // this should NOT be reported
  if (children) {
    // ...
  }

  // this should NOT be reported
  return <div>{children}</div>
}

But I am happy to make another PR about this case too (I just haven't figured out how to identify with AST).

P.S. If you feel that this PR is following the rules of Hactoberfest and this gets merged, it would be wonderful if this PR would be tagged with a hacktoberfest-accepted tag/label. Thanks!

If node's object name is "props" any of the node accessing properties is not reported.

Ref: testing-library#386
Copy link
Member

@Belco90 Belco90 left a comment

Choose a reason for hiding this comment

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

Just a small change in the docs and will be ready to merge.

docs/rules/no-node-access.md Outdated Show resolved Hide resolved
@sjarva sjarva requested a review from Belco90 October 2, 2022 16:53
@Belco90 Belco90 merged commit 5f4287f into testing-library:main Oct 2, 2022
@github-actions
Copy link

github-actions bot commented Oct 2, 2022

🎉 This PR is included in version 5.7.2 🎉

The release is available on:

Your semantic-release bot 📦🚀

@sjarva sjarva deleted the pr/no-node-access-props-children-bug branch October 2, 2022 17:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Library incorrectly report no-node-access when referencing props.children
2 participants