feat: add no-container rule

[thebinaryfelix]

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):

[gndelia]

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 🚀

[thebinaryfelix]

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!

[gndelia]

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)

[thebinaryfelix]

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]

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]

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.

[Belco90]

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

[gndelia]

LGTM 🚀

[gndelia]

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

[allcontributors]

@gndelia

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

[Belco90]

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]

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]

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]

🎉 This PR is included 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 PR is included in version 4.0.0 🎉

The release is available on:

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

Your semantic-release bot 📦🚀