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.

Sign up for GitHub

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

Jump to bottom

Fail no-container on use of innerHTML #883

Open
CodingItWrong opened this issue Feb 29, 2024 · 6 comments · May be fixed by #889
Open

Fail no-container on use of innerHTML #883

CodingItWrong opened this issue Feb 29, 2024 · 6 comments · May be fixed by #889
Labels
enhancement New feature or request

Comments

@CodingItWrong
Copy link
Contributor

What rule do you want to change?

no-container

Does this change cause the rule to produce more or fewer warnings?

More warnings

How will the change be implemented?

The rule will look for usages of container.innerHTML in addition to looking for usages of methods like container.querySelector()

Example code

const { container } = render(<Greeting />);
expect(container.innerHTML).toContain("Hello");

How does the current rule affect the code?

The current rule allows this code. It only errors if a method like container.querySelector is accessed.

How will the new rule affect the code?

The updated rule would fail on usages of container.innerHTML just as it does with the methods.

Anything else?

Descriptions of the rule would need to be updated from saying "disallow the user of container methods" to "container methods and properties" (or something)

If we found that anyone has use cases where using innerHTML is commonly needed but container methods are not, we could either make it configurable whether it's allowed, or make it a separate rule very similar to no-container

Do you want to submit a pull request to change the rule?

Yes
@CodingItWrong CodingItWrong added enhancement New feature or request triage Pending to be triaged by a maintainer labels Feb 29, 2024
@CodingItWrong
Copy link
Contributor Author

This feels like this'll be as easy to add as the previous rule I added, or easier if it's OK to add this functionality into the existing rule. Happy to do the PR for it 👍

@Belco90 Belco90 removed the triage Pending to be triaged by a maintainer label Mar 4, 2024
@Belco90
Copy link
Member

Belco90 commented Mar 4, 2024 •
edited

Hi @CodingItWrong. This is a great suggestion! We would appreciate a PR for implementing this, so definitely go for it by adding the functionality into the existing rule.

@CodingItWrong
Copy link
Contributor Author

Sounds good, @obsoke and I plan to pair on it again over the next few weeks. Thanks for your input!

@obsoke
Copy link
Contributor

obsoke commented Mar 7, 2024

@Belco90 We were curious how restrictive we want to be when accessing properties on container. The issue proposes disallowing accessing innerHTML. Do we want to focus on solely this property for now? What about other properties such as firstChild, outerHTML, localName, etc.?

The rule currently disallows calling any methods on container so there is precedent for disallowing properties broadly as well. However, there might be valid use cases for some properties such as firstChild.

@CodingItWrong CodingItWrong linked a pull request Mar 18, 2024 that will close this issue
Draft
1 task
@Belco90
Copy link
Member

Belco90 commented Mar 20, 2024

@obsoke I'd say disallowing any method from the container property would be ideal. I think in general properties like firstChild should be avoided.

@CodingItWrong
Copy link
Contributor Author

Thanks @Belco90. Just to make sure we follow, we will plan on disallowing any property on the container. This rule already disallows all methods, and since you noted that properties like firstChild should be avoided, I follow that your intent is that now all properties should be disallowed in addition to all methods.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Disallow usages of innerHTML for no-container rule testdouble/eslint-plugin-testing-library
3 participants
@obsoke @Belco90 @CodingItWrong