Allow destructuring from screen with prefer-screen-queries

[joe-reed]

It would be great if prefer-screen-queries could allow destructuring from screen, for example:

it("renders foo", () => {
  const { getByText } = screen;

  render(<App />);

  // currently flagged by rule
  expect(getByText("foo")).toBeTruthy();
});

or

const { getByText } = screen;

it("renders foo", () => {
  render(<App />);

  // currently flagged by rule
  expect(getByText("foo")).toBeTruthy();
});

[Belco90]

Hey Joe! Thanks for opening this issue. Interesting one. The rule shouldn't complain in that case, so I'm adding the label "bug" here.

[gndelia]

not sure this is a bug. The point of the rule is using screen imported to avoid destructuring- not the origin of the methods. If the methods come from screen or the returned value from render, it's the same

the only exception that applies is within because within changes the element that is bound.

[Belco90]

I didn't think about it in that way. That makes sense, I think you are completely right @gndelia

[joe-reed]

This is a good point, and I think the second example in my original comment is a better example - i.e. destructuring all the needed queries from screen once at the top of the test file rather than in each individual test.

The way the rule is currently worded/implemented does imply that the origin of the methods is important and that the point of the rule is not just avoiding destructuring - otherwise wouldn't the following be acceptable?

it("renders foo", () => {
  const renderResult = render(<App />);

  // currently flagged by rule
  expect(renderResult.getByText("foo")).toBeTruthy();
});

[gndelia]

from what I understand in kent c dodds post about 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.

as I see it, your second example should be triggered by the linter as "incorrect", according to the rule.

The only allowed scenarios to query should be screen.{{query}} calls, being the only exception any queries related to within (as it changes the bounding of the queries). There, nothing is said about allowing destructuring or not, so I assume it should be allowed...

[joe-reed]

I understand, that makes sense.

There are benefits to using queries from screen over queries from render - it provides a single set of queries that can be used in all of your tests rather than obtaining a new query function every test, and also allows for easier extraction of helper/util functions for common assertions or interactions that you want to reuse between tests, as they can use screen globally.

Additionally, there are benefits to preferring not destructuring queries out, as you are able to change the queries you are using without having to keep the destructure up to date - but this can be achieved by both screen.getByText() and renderResult.getByText() (where renderResult is the return value of render())

It feels like two separate concerns to me - perhaps we should have two rules, e.g. prefer-screen-queries and prefer-no-query-destructuring

[Belco90]

Ok so based on "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" I think it's clear one of the main points of using screen is not having to destructure anything for getting the queries.

Having said that, I understand @joe-reed's point. It's completely fine to destructure screen itself. Though I'm not sure if it should be a concern for this rule.

The main point of the rule is to encourage using screen because of its advantages. If you don't want to be forced to use screen in that particular way (which is the one making the code more maintainable), I don't see the point of adapting the rule. Instead, I think you should disable the rule so you are free to destructure global screen or utils from render (which will make you lose that benefit from screen).

To wrap up my thoughts around this: the goal of this rule is use screen because of its benefits. So if you don't want/need them, I think the rule should be disabled rather than making the rule so flexible that could not make sense anymore.

Happy to clarify this in the rule doc tho. We can link that section from KCD's common mistake post and include some conclusions from this discussion.

[joe-reed]

Ok so based on "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" I think it's clear one of the main points of using screen is not having to destructure anything for getting the queries.

It's clearly one of the benefits as mentioned in the blog post, but not the only benefit, otherwise why use screen over doing

  const renderResult = render(<App />);
  expect(renderResult.getByText("foo")).toBeTruthy();

In my opinion it would be useful to be able to only allow developers to use queries from screen, while still allowing them to destructure. If we also wanted to disallow destructuring, a new rule could handle that.

It seems a shame to say that if you do want to destructure from screen at the top of your test file, the solution is to disable the rule entirely, which has the downside of allowing devs to use queries from the result of render too.

[Belco90]

I understand your point, and it's a fair point to be honest. Let me think about it again, but I'm not sure about relaxing this rule for allowing destructuring screen for now.

[Belco90]

@joe-reed just exploring other possibilities. What do you think about making this rule configurable? So by default it works like it is but you could relax it by passing { allowDestructuring: true } or something similar? I'm just thinking out loud here.

[timdeschryver]

How I see it it's still invalid while destructuring screen, and the point made by Kent is still valid (alltough it's not as big as a pain as with render).

you no longer need to keep the render call destructure up-to-date as you add/remove the queries you need

So every time a new query is used, you will still need to add it to the destructure.

[joe-reed]

@joe-reed just exploring other possibilities. What do you think about making this rule configurable? So by default it works like it is but you could relax it by passing { allowDestructuring: true } or something similar? I'm just thinking out loud here.

That's certainly an option. Having an additional rule for disallowing destructuring does have the benefit that if a team prefers to use queries from render they could still disallow destructuring in that case.

How I see it it's still invalid while destructuring screen, and the point made by Kent is still valid

I agree that the best option is to have the combination of using preferring queries from screen and disallowing destructuring of queries, and I'd propose that the combination of these two should be the "recommended" set of rules, but allowing developers to opt in or out of these independently would allow the most flexibility.

Then developers would be free to choose from

// prefer-no-query-destructuring: false, prefer-screen-queries: false
it("renders foo", () => {
  const { getByText } = render(<App />);
  expect(getByText("foo")).toBeTruthy();
});

// prefer-no-query-destructuring: true, prefer-screen-queries: false
it("renders foo", () => {
  const renderResult = render(<App />);
  expect(renderResult.getByText("foo")).toBeTruthy();
});

// prefer-no-query-destructuring: false, prefer-screen-queries: true
const { getByText } = screen;

it("renders foo", () => {
  render(<App />);
  expect(getByText("foo")).toBeTruthy();
});

// prefer-no-query-destructuring: true, prefer-screen-queries: true (recommended)
it("renders foo", () => {
  render(<App />);
  expect(screen.getByText("foo")).toBeTruthy();
});

[Belco90]

Ok so after thinking about this, my final position it's to not implementing this option.

The main point of this ESLint plugin is helping users "to follow best practices and anticipate common mistakes". Using getting queries from screen without destructuring is one of those best practices. Implementing the ability to skip some of the checks could mislead people about what those best practices are. Additionally, it seems like a really particular and specific use case. So I'm sorry but I don't think we need to cover this case, specially when is a rule you can enable/disable as you wish.

[Belco90]

Having said that, I'm happy to update the rule description to cover why it won't allow you to destructure queries from screen.