New Rule: Disallow DOM exploration properties

[jsphstls]

This is not a great way to select elements, but is allowed:
parentElement.parentElement.parentElement

It would be great if there were an optional rule that discouraged the use of properties such as parentElement, previousSibling, and the rest.

The rule could be highly configurable by allowing each property key to be enabled or disabled. It could also allow a certain level of traversing so that one use of a property is okay, but a chain is not.

[Belco90]

This is a really good idea! I guess this rule should prevent some of Node properties from being used but we need to agree which ones (I guess those returning another Node).

I love the idea of restricting level of traversing them, but I think we need to think about how we want to config this: for all the properties? by individual properties? both?

The rule could be named like no-node-access or something similar, what you think?

[jsphstls]

That's a good name. I'm not familiar with rule creation but I have seen that rule configuration can become problematic. What if the rule begins with no configuration and then configuration is added based on input after release?

I think starting with a default list of properties that return another node is safe. I don't think traverse level should be set by property.

[Belco90]

I'm happy with restricting all the rules returning another node for first approach (without enabling the rule by default in any shareable config) and the we see what we do. How should the traverse level work then?

[jsphstls]

The traverse level can be an integer property, similar to complexity. This could help with adoption as the level is gradually decreased while making fixes in a codebase.

Maybe we are looking at two different rules? One that handles traverse level at a minimum of one and another that disallows all properties but also allows whitelisting?

[Belco90]

Mmm I like the idea about splitting this in two different rules. What do you think @emmenko @thomlom ?

[emmenko]

Sure we can give it a try. I'm still trying to wrap my head around the workings of the rule but maybe when I see some concrete examples things will be clearer.

[thomaslombart]

That sounds good to me too but I would go with one rule only. Having two rules would be more confusing as they're closely related.

Configuration-wise, we could have something like:

"no-node-access": ["error", { allowedProperties: [...], max: 2 }]

[Belco90]

@thomlom Fair enough, your proposal seems best approach for now. @jsphstls do you want to give this rule a try?

[jsphstls]

I cannot promise enough time or expertise, so it is open to anyone really. This got more momentum than I expected 😅

[thomaslombart]

Anyone for this issue? 🙂

[thebinaryfelix]

Hey guys! So, if I've understood correctly, this rule should throw an error message when the user tries to select a DOM Node by traversing through multiple Nodes.

The config would be set as "no-node-access": ["error", { allowedProperties: [...], max: 2 }] where:

• allowedProperties: is a list of properties that return another Node set by the user, with default being empty [].
• max: integer that represents the number of levels which can be traversed, similar to complexity rule

Some examples

incorrect

/* no-node-access: ['error'] */

const node = container.firstChild

/* no-node-access: ['error', { max: 1 }] */

const node = container.firstChild.firstChild

/* no-node-access: ['error', { max: 2 }] */

const node = container.firstChild.firstChild.nextSibling

/* no-node-access: ['error', { allowedProperties: ['nextSibling'], max: 1 }] */

const node = container.firstChild.nextSibling.firstChild

correct

/* no-node-access: ['error', { max: 1 }] */

const node = container.firstChild.nodeName // as nodeName returns a DOMString

/* no-node-access: ['error', { allowedProperties: ['firstChild'] }] */

const node = container.firstChild.firstChild // as firstChild is listed in allowedProperties

/* no-node-access: ['error', { allowedProperties: ['firstChild'], max: 1 }] */

const node = container.firstChild.firstChild.nextSibling

/* no-node-access: ['error', { allowedProperties: ['nextSibling'], max: 2 }] */

const node = container.firstChild.nextSibling.firstChild

Did I get it right? Are there any other considerations regarding the implementation of this rule or anyone already working on it? If not, I'd like to give it a try :)

[timdeschryver]

Wouldn't this rule be caught with the no-container rule (#177)?
Thinking out loud, if the user allows accessing the container it's probably for doing stuff like this?

Besides this, I think you nailed it @thebinaryfelix

[Belco90]

@thebinaryfelix you are right, but keep in mind this applies to every element returned from a Testing Library query, not just the container. This is basically the difference from no-container @timdeschryver.

The idea here then is to report things like:

const buttonText = screen.getByText('submit');
const button = buttonText.closest('button');

That's why we discussed about how many chained levels we should allow.

[Belco90]

This one is probably the trickiest and complex one in the plugin, as the are tons of things to keep in mind:

• different ways of accessing node properties:

const buttonText = screen.getByText('submit');
const button = buttonText.closest('button');

 // or

const button = screen.getByText('submit').closest('button');

// or many others you probably can think of now

• elements within lists from Testing Library queries:

const buttons = screen.getAllByRole('button');

// this should be reported
buttons[0].firstChild;

// this should be reported too
const buttonA = button[1];
expect(buttonA.lastChild).toBeInTheDocument();

• check amount of chained calls properly

[thebinaryfelix]

keep in mind this applies to every element returned from a Testing Library query, not just the container.

Yes! The "container" I mentioned was a poor example name, sorry about that, but what I meant was a Node in general.

Basically we would have to:

• check whether a variable contains a Node and if somewhere else on the code any methods or properties that return another Node are being called by that same variable (and there are also the cases without variable declaration) or by the result of calling another of it's chained methods/properties.

I think that with this concept in mind we can get to those variations of ways of accessing a Node.

[Belco90]

@thebinaryfelix That sounds perfect, I've assigned it to you then.

In the original discussion we decided to:

• keep it simple and avoid adding any kind of rule config for now, so you don't need to worry the chained calls now and just check the first call
• only report those methods returning another node(s), like childNodes or firstChild. You can find more info in MDN - Node section.

I think these 2 details will simplify the first approach. Let me know if you want to clarify something else or write down the specific methods the rule should report.

[thebinaryfelix]

@Belco90 great, thanks!

I think it's all well explained now and these two details will definitely simplify the approach. If I have any questions after starting the implementation I'll let you know.

[Belco90]

This will be released together with v4

[github-actions]

🎉 This issue has been resolved in version 4.0.0-beta.1 🎉

The release is available on:

• npm package (@beta dist-tag)
• GitHub release

Your semantic-release bot 📦🚀

[github-actions]

🎉 This issue has been resolved in version 4.0.0 🎉

The release is available on:

• npm package (@latest dist-tag)
• GitHub release

Your semantic-release bot 📦🚀