Propose removing this rule: prefer-screen-queries
OleDakotaJoe opened this issue · 9 comments
What rule do you want to change?
prefer-screen-queries
Does this change cause the rule to produce more or fewer warnings?
Fewer warnings
How will the change be implemented?
Remove the rule.
Example code
[Delete] + Raise PR.
How does the current rule affect the code?
This rule seems to be based on a kentcdodds post.
The only reason it exists is
From: https://kentcdodds.com/blog/common-mistakes-with-react-testing-library#not-using-screen
The benefit of using screen is you no longer need to keep the render call destructure up-to-date as you add/remove the queries you need. You only need to type screen. and let your editor's magic autocomplete take care of the rest.
Which, in my opinion, is some developer's personal preference in how he works in his workflow, and thus should not be a “rule”
It doesn’t matter.
Tbh – I don’t want to type ",screen" in my import, then “sc + tab +.” 6 times, when I can type “get + tab” then “que + tab” then “qu + tab” twice and “get + tab” 4 times.
IMO, 6 of one, half dozen of another and honestly a very silly rule for react/testing-library to have brought into their core library to begin with.
How will the new rule affect the code?
It will get rid of it, and developers would not have to follow some other developer's personal preference for no apparent reason other than "personal preference"
Anything else?
I just got asked to "fix" my tests in code review because of this rule.
There is absolutely 0 difference in the outcome of the code, and it is arguably more cluttered now because of it.
IMO, this should not be a rule, as it has no actual impact on the code, and it really doesn't make sense to enforce conformity to an expectation that one developer in the community has found to be useful in their own workflow.
We should not be writing ES-LINt rules based on an IDE's ability to autocomplete.
Do you want to submit a pull request to change the rule?
Yes
The rule is there because RTL is planning on removing these helper function from render's return & will only support screen.* in the future
Do you mind linking an issue where they have mentioned removing it?
That's unfortunate.
And a bit misleading that the rule itself explicitly says that it exists only to make it easier for importing, which IMO should be up to the developer (assuming it's supported).
If support is going away, that's perfectly understandable.
Though, I'm curious -- why is support going away for this?
Will be lots of broken tests.
This is what the docs say on that rule:
DOM Testing Library (and other Testing Library frameworks built on top of it) exports a screen object which has every query (and a debug method). This works better with autocomplete and makes each test a little simpler to write and maintain.
This rule aims to force writing tests using built-in queries directly from screen object rather than destructuring them from render result. Given the screen component does not expose utility methods such as rerender() or the container property, it is correct to use the render returned value in those scenarios.
Which, in all fairness, is a rule that is enforcing an opinion.
If the support is going away, then those docs should read to the effect of "support will go away in the future, and you should be using the screen object instead"
Do you mind linking an issue where they have mentioned removing it
Can't find it, but I think @kentcdodds can probably provide some more guidance towards reasoning about this.
If I'm correct there was even a PR opened at one point, but it was reverted because not everything was possible to remove (yet).
Will be lots of broken tests.
Whenever they release this, it would be a major release, so breaking changes are possible
If the support is going away
The PR I mentioned is from more than a year ago if I'm correct, so I get that the core RTL team wants to first figure out all quirks & bumps before mentioning removing them publicly
Well, thanks for letting me know!
Hey @OleDakotaJoe! Not sure if queries coming from the render method will disappear. However, using queries from screen is recommended by Testing Library docs. This is the motivation for the discussed rule.
If you don't want to follow this advice, just disable the rule in your codebase!
fair point
I don't expect the queries to ever be removed from the return of render. Feel free to disable the rule if you prefer. I don't have any more to add beyond the testing library docs or my blog post. And as I'm not an active maintainer of testing library, I can only recommend that this rule remain and default to enabled for people, but I can't make that decision.
Cheers!
All good comments and suggestions.
Thanks for taking the time to reply.