feat: add no-container rule #177

thebinaryfelix · 2020-06-19T16:42:57Z

Closes #171

This PR adds a new no-container rule for disallowing the use of container methods like querySelector, following best practices.

A refactor of the no-debug rule was made in order to extract two helper functions to node-utils.ts so they could be used by no-container.

Preview of how the message will appear (in VS Code):

to node-utils, to be used by no-container

for containerIndex | clean up method declaration

functions

…testing-library into no-container

gndelia · 2020-06-19T17:06:46Z

Thanks for your PR! There are some conflicts with the files. Could you rebase/update your PR ? Thanks!

gndelia

I added some comments. I Will continue reviewing it later. Thank you very much for this PR 🚀

docs/rules/no-container.md

lib/rules/no-container.ts

tests/lib/rules/no-container.test.ts

thebinaryfelix · 2020-06-19T17:20:15Z

I added some comments. I Will continue reviewing it later. Thank you very much for this PR 🚀

I'll take a look and work on them later, thanks for the review!

Belco90

Hi Mateus! Thank you very much for this PR, specially about the custom render function refactor.

I added some comments, basically for adding more test cases (it could imply updating the rule implementation tho) and removing the rule from recommended config.

Let me know if you need help to address the feedback here or want to discuss something!

README.md

docs/rules/no-container.md

lib/index.ts

lib/rules/no-container.ts

tests/lib/rules/no-container.test.ts

| add description about testing library frameworks | add link to container docs | remove recommended badge

| update error message

docs/rules/no-container.md

tests/lib/rules/no-container.test.ts

gndelia · 2020-06-20T00:32:46Z

I think that by using destructuring, it'd be also possible to bypass this rule

const { container: { querySelector } } = render(<Example />);
querySelector('foo');

I don't generally use that double-nested destructuring because I find it really hard to read 😆 but it's valid js code. The rule should probably cover it. Can we add it to the docs as invalid, and in the tests? I followed the code and I think it does not cover this scenario (but I might be wrong)

lib/rules/no-container.ts

scenario

| destructure method from container

and change its scope

thebinaryfelix · 2020-06-20T19:45:47Z

I think that by using destructuring, it'd be also possible to bypass this rule
const { container: { querySelector } } = render(<Example />);
querySelector('foo');
I don't generally use that double-nested destructuring because I find it really hard to read laughing but it's valid js code. The rule should probably cover it. Can we add it to the docs as invalid, and in the tests? I followed the code and I think it does not cover this scenario (but I might be wrong)

You're right! It didn't cover this scenario, I've adjusted the code now. If you think of any other case, please let me know.

thebinaryfelix · 2020-06-20T19:51:53Z

I think that by using destructuring, it'd be also possible to bypass this rule
const { container: { querySelector } } = render(<Example />);
querySelector('foo');
I don't generally use that double-nested destructuring because I find it really hard to read laughing but it's valid js code. The rule should probably cover it. Can we add it to the docs as invalid, and in the tests? I followed the code and I think it does not cover this scenario (but I might be wrong)
You're right! It didn't cover this scenario, I've adjusted the code now. If you think of any other case, please let me know.

Actually there's another case where the user could rename the method by doing something like:

const { container: { querySelector: alias } } = render(<Example />);
alias("foo");

And we could verify that recursively, but do we really wanna go that far to validate it? Also, if anyone has another approach for how to check that, feel free to share it.

Belco90 · 2020-06-20T20:04:46Z

Actually there's another case where the user could rename the method by doing something like:
const { container: { querySelector: alias } } = render(<Example />);
alias("foo");
And we could verify that recursively, but do we really wanna go that far to validate it? Also, if anyone has another approach for how to check that, feel free to share it.

I'm against going that far in general. We need to cover different cases which are pretty common, but this is a really weird edge case I would say. So I'd go ahead with a basic coverage of different usages of this method (which is actually really nice) and leave that complex destructuring case as a future improvement.

lib/rules/no-container.ts

and incorrect use case

Belco90

Looking good now. Amazing job here mate, thank you very much for your work and patience 😅

gndelia

LGTM 🚀

gndelia · 2020-06-21T20:06:12Z

@all-contributors please add @thebinaryfelix for code, tests and doc

allcontributors · 2020-06-21T20:06:22Z

@gndelia

I've put up a pull request to add @thebinaryfelix! 🎉

Belco90 · 2020-06-22T08:36:00Z

I need to update contributing guidelines with a section for maintainers. Meanwhile, please let's try to squash merge PRs to make the commit messages fit with the semantic release changelogs (except when merging v4 to master for example, so we don't ruin the original commits as I did when merging v3 😓 ).

gndelia · 2020-06-22T13:23:17Z

Hey @Belco90 I don't have access, but I think there should be a way in the settings to only allow squash merges in PR

(this is from another project I work for)

Perhaps that could help to avoid missing squashes. (but yeah, we'd need to be careful when merging v4 to master - we'd need to disable it there)

I reviewed the commit message so it fit the semantic release, but I missed the squash. Sorry for that 😓

Belco90 · 2020-06-22T13:26:45Z

No problem, not a big deal as I can update the release notes after the release itself. I don't want to force always squash merging tho, as it's useful to allow merge commits too (for example when merging v4 to master).

github-actions · 2021-03-28T18:18:54Z

🎉 This PR is included in version 4.0.0-beta.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

github-actions · 2021-04-11T18:19:00Z

🎉 This PR is included in version 4.0.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Mateus Felix added 9 commits June 18, 2020 20:51

feat(no-container): setup new rule on index

094f53c

refactor(no-container): set rules

da88a9d

test(no-container): define scenarios

ee369e5

refactor(no-debug): extract auxiliary functions

5d02426

to node-utils, to be used by no-container

refactor(no-container): add conditional

0205374

for containerIndex | clean up method declaration

docs(no-container): add new and update README

d8e555e

refactor(no-container): allow custom render

497a5b6

functions

test(no-container): add custom render function

4ab2305

docs(no-container): update further reading topics

c11fcd6

Belco90 added this to the v4 milestone Jun 19, 2020

Belco90 changed the base branch from master to v4 June 19, 2020 16:49

Belco90 changed the title ~~no-container rule~~ feat: add no-container rule Jun 19, 2020

Belco90 added the new rule New rule to be included in the plugin label Jun 19, 2020

Merge branch 'v4' of /~https://github.com/thebinaryfelix/eslint-plugin-…

1d8077c

…testing-library into no-container

Merge branch 'v4' into no-container

d624bfe

gndelia requested changes Jun 19, 2020

View reviewed changes

docs/rules/no-container.md Show resolved Hide resolved

lib/rules/no-container.ts Outdated Show resolved Hide resolved

tests/lib/rules/no-container.test.ts Show resolved Hide resolved

Belco90 requested changes Jun 19, 2020

View reviewed changes

Mateus Felix added 4 commits June 19, 2020 16:23

docs(no-container): update

1c59122

| add description about testing library frameworks | add link to container docs | remove recommended badge

test(no-container): add scenarios

52f5995

refactor(no-container): adjust to new scenarios

4eb5381

| update error message

docs(no-container): add incorrect use cases

3943f45

gndelia requested changes Jun 20, 2020

View reviewed changes

docs/rules/no-container.md Show resolved Hide resolved

tests/lib/rules/no-container.test.ts Outdated Show resolved Hide resolved

gndelia requested changes Jun 20, 2020

View reviewed changes

lib/rules/no-container.ts Outdated Show resolved Hide resolved

thebinaryfelix added 3 commits June 20, 2020 13:05

refactor(no-container): remove wrong use case

8e2cdcc

scenario

refactor(no-container): add scenario

78fdfe7

| destructure method from container

refactor(no-container): rename function

9812cf4

and change its scope

nickserv linked an issue Jun 20, 2020 that may be closed by this pull request

new rule: no-container #171

Closed

nickserv removed this from the v4 milestone Jun 20, 2020

refactor(no-container): remove condition

e63968f

Belco90 mentioned this pull request Jun 20, 2020

Rename recommended config #182

Closed

Belco90 reviewed Jun 20, 2020

View reviewed changes

lib/rules/no-container.ts Outdated Show resolved Hide resolved

refactor(no-container): update error message

5aa3a80

and incorrect use case

Belco90 approved these changes Jun 21, 2020

View reviewed changes

Belco90 requested a review from gndelia June 21, 2020 10:33

Merge branch 'v4' into no-container

9d44911

gndelia approved these changes Jun 21, 2020

View reviewed changes

gndelia merged commit 86f9c84 into testing-library:v4 Jun 21, 2020

allcontributors bot mentioned this pull request Jun 21, 2020

docs: add thebinaryfelix as a contributor #185

Merged

timdeschryver mentioned this pull request Jun 22, 2020

New Rule: Disallow DOM exploration properties #62

Closed

Belco90 mentioned this pull request Jun 22, 2020

ESLint plugin Testing Library v4 #186

Closed

19 tasks

gndelia mentioned this pull request Jul 20, 2020

New Rule: dont-use-query-selector #199

Closed

github-actions bot added the released on @beta label Mar 28, 2021

github-actions bot added the released label Apr 11, 2021

MichaelDeBoey added this to the v4 milestone Apr 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add no-container rule #177

feat: add no-container rule #177

thebinaryfelix commented Jun 19, 2020 •

edited

Loading

gndelia commented Jun 19, 2020

gndelia left a comment

thebinaryfelix commented Jun 19, 2020

Belco90 left a comment

gndelia commented Jun 20, 2020 •

edited

Loading

thebinaryfelix commented Jun 20, 2020

thebinaryfelix commented Jun 20, 2020 •

edited

Loading

Belco90 commented Jun 20, 2020

Belco90 left a comment

gndelia left a comment

gndelia commented Jun 21, 2020

allcontributors bot commented Jun 21, 2020

Belco90 commented Jun 22, 2020

gndelia commented Jun 22, 2020 •

edited

Loading

Belco90 commented Jun 22, 2020

github-actions bot commented Mar 28, 2021

github-actions bot commented Apr 11, 2021

feat: add no-container rule #177

feat: add no-container rule #177

Conversation

thebinaryfelix commented Jun 19, 2020 • edited Loading

gndelia commented Jun 19, 2020

gndelia left a comment

Choose a reason for hiding this comment

thebinaryfelix commented Jun 19, 2020

Belco90 left a comment

Choose a reason for hiding this comment

gndelia commented Jun 20, 2020 • edited Loading

thebinaryfelix commented Jun 20, 2020

thebinaryfelix commented Jun 20, 2020 • edited Loading

Belco90 commented Jun 20, 2020

Belco90 left a comment

Choose a reason for hiding this comment

gndelia left a comment

Choose a reason for hiding this comment

gndelia commented Jun 21, 2020

allcontributors bot commented Jun 21, 2020

Belco90 commented Jun 22, 2020

gndelia commented Jun 22, 2020 • edited Loading

Belco90 commented Jun 22, 2020

github-actions bot commented Mar 28, 2021

github-actions bot commented Apr 11, 2021

thebinaryfelix commented Jun 19, 2020 •

edited

Loading

gndelia commented Jun 20, 2020 •

edited

Loading

thebinaryfelix commented Jun 20, 2020 •

edited

Loading

gndelia commented Jun 22, 2020 •

edited

Loading