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

[brettz9]

There is a requirement specific to prefer-node-remove which expects the parent to be named "parentNode" or "parentElement".

There is no such requirement in prefer-modern-dom-apis with replaceChild or insertBefore, and I think there need be no such requirement.

[fisker]

Can you point me the line ? Can't find

[brettz9]

Sure. https://github.com/sindresorhus/eslint-plugin-unicorn/blob/master/rules/prefer-node-remove.js#L63-L64

[fisker]

Sorry , I thought you are talking about prefer-modern-dom-apis, so missed it, haha.

Indeed, it seem unnessary, I think the old one mean to fix only foo.parentNode.removeChild(foo) but accidentally also allow parentNode.removeChild(foo).

Question is can bar.removeChild(foo) fix to foo.remove(), I think its safe.

[brettz9]

Yes, I think it would be. And sometimes I even use parentNode as the variable name, e.g., const {parentNode} = node; so end up with parentNode.removeChild(foo) or, as in your example, bar.removeChild(foo).

[fisker]

Also I think old one allow foo.parentNode.removeChild(bar) fix to bar.remove() since argument and caller is not compared. so this check is meaningless. Can you confirm this for me? Not on my laptop

[brettz9]

foo.parentNode.removeChild('bar') is a valid test case (i.e., it is not reported/fixed).

I think it should be reported though since foo could be a sibling of bar.

[fisker]

No this case is not what I mean , Literal cant pass argument check bar without quotes

[brettz9]

You're right, foo.parentNode.removeChild(bar) is reported as invalid and fixed to bar.remove().

[fisker]

Ok, accepted. remove that check, PR welcome

[fisker]

Other things to be done,

1. check arguments length, as we do in other similar cases
2. fix the argument check, should allow other types like removeChild(foo.bar) removeChild(foo[0]) , it should allow any type except some type that cant be a Node like literal type. be careful might need () to argument, maybe even ;()

[brettz9]

Ok, I've done #1 (including for prefer-node-append, where I also changed to include the callee property name inside the selector).

I'm not sure what you mean about the parentheses in #2.

[brettz9]

And I've added the callee type within the selector for prefer-modern-dom-apis.

[brettz9]

I don't know if you want to crowd any more than one condition into the selectors, even though it appears possible per https://eslint.org/docs/developer-guide/selectors#restricting-syntax-with-selectors .

[fisker]

for 2, foo.removeChild(foo.bar) is not fixed, because arguments[0] is not Identifier or ThisExpression.

About parentheses, I mean , foo.removeChild(await bar()) can't fix to await bar().remove() should be (await bar()).remove()

And

const array = []
foo.removeChild(await bar())

to

const array = []
(await bar()).removeChild()

still not right, this is right. (but we can forget about this, too hard)

const array = []
;(await bar()).removeChild()

[fisker]

I don't know if you want to crowd any more than one condition into the selectors, even though it appears possible per eslint.org/docs/developer-guide/selectors#restricting-syntax-with-selectors .

I think only check callee.property.name in selector is good enough, you can add more if you want.

[fisker]

@brettz9 I thought about this today again. The fix maybe more complicate, say we have this

someDomLib('div')
	.one()
	.two()
	.three()
	.four()
	.getParent() // do a lot of stuff , and return the parent at last
	.removeChild(bar);

I think we just fix it to

bar.remove();

actually, we should do

someDomLib('div')
	.one()
	.two()
	.three()
	.four()
	.getParent(); // do a lot of stuff , and return the parent at last
bar.remove();