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

Expand reporting for prefer-node-remove #507

Merged

Conversation

brettz9
Copy link
Contributor

@brettz9 brettz9 commented Feb 2, 2020

  • prefer-node-remove: Expand reporting to cover cases where parent of removeChild is other than parentNode or parentElement.

Fixes #506.

…of `removeChild` is other than `parentNode` or `parentElement`.
@fisker

This comment has been minimized.

rules/prefer-node-remove.js Outdated Show resolved Hide resolved
:

1. insist on a single argument
2. Put callee property name in selector
@brettz9 brettz9 force-pushed the prefer-node-remove-remove-parent-check branch from ca96301 to 5888f36 Compare February 2, 2020 08:58
rules/prefer-node-remove.js Outdated Show resolved Hide resolved
@fisker
Copy link
Collaborator

fisker commented Feb 2, 2020

@brettz9 Let's focus on prefer-node-remove, do the selector thing on other rule in another PR, shall we?

We have a lot to do with this rule

rules/prefer-node-remove.js Outdated Show resolved Hide resolved
rules/prefer-node-remove.js Outdated Show resolved Hide resolved
@fisker fisker self-assigned this Feb 10, 2020
@fisker fisker force-pushed the prefer-node-remove-remove-parent-check branch from 04b37b0 to 9637cc4 Compare February 12, 2020 10:37
@fisker
Copy link
Collaborator

fisker commented Feb 12, 2020

More detail: #506

@brettz9 already did:

  • prefer-node-remove: Expand reporting to cover cases where parent of removeChild is other than parentNode or parentElement.

I've done:

  • replace error message with a general message Prefer `childNode.remove()` over `parentNode.removeChild(childNode)`., old one is more specific, but could be Prefer `foo.remove()` over `null.removeChild(foo)`. in some case
  • since we are not checking parentNode/parentElement anymore, I removed most of them from tests
  • simplify tests that can't be fixed.
  • exclude parentNode.removeChild(undefined) -> undefined.remove()

Need to do (will raise another issue after this merge):

  • allow more childNode, not only Identifier and ThisExpression, what I most want support is:

parentNode.removeChild(some.node) / parentNode.removeChild(get.child())

  • investigate what should we do with complicated callee

eg:

doSomething1().doSomething2().doSomething3().doSomething4().getParent()
	.removeChild(childNode)

we can't just fix to childNode.remove().

/cc @brettz9 @sindresorhus

@fisker fisker force-pushed the prefer-node-remove-remove-parent-check branch from 5d1e92e to efd573e Compare February 12, 2020 11:15
@fisker fisker force-pushed the prefer-node-remove-remove-parent-check branch from efd573e to 8b85bc6 Compare February 12, 2020 11:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

prefer-node-remove: Remove requirement for removeChild to be on parentElement or parentNode
3 participants