[new-rule] prefer importing from @jest/globals

[ecraig12345]

It would be nice to have a rule (with fixer) enforcing that APIs such as expect, describe, it, jest, etc should be imported from @jest/globals. If there's interest in this rule, I'd probably be willing to implement it.

Motivation:

• Reduce global type pollution by not needing to put "types": ["jest"] in a tsconfig
• Allow removing dependency on @types/jest which can be outdated, mismatched, and/or pull in extra versions of packages (this is an issue as of writing--jest 28 has released but the types are still on 27 and pull in some outdated deps from 27)
• Unblock adoption of future mode with globals unavailable once that's implemented (I'm not sure where that is on their roadmap)

Here's an example of code with the rule enabled, and an error it might catch:

import { describe, it, expect, jest } from '@jest/globals';
import { foo } from './foo';

describe('foo', () => {
  // ERROR - you forgot to import beforeAll
  beforeAll(() => {
    jest.useFakeTimers();
  });

  it('does stuff', () => {
    expect(foo()).toBe(0);
  });
});

(I assume this would be blocked by #556, but looks like there's an approved PR #1094 addressing that.)

[G-Rath]

I think you should be able to handle this with the existing tooling and education - e.g. so long as you don't have @types/jest installed, Typescript will error (you just have to ignore it's suggestion to install @types/jest).

Having said that the core of the work for this is now already done via the PR you mentioned, so a rule shouldn't be expensive (though I don't know if we could have a autofix to add the import without adding a bunch of complexity).

What is missing first is the follow up work I'm going to do: moving from a conditional check to returning information about the reference (which is needed to handle import { xit as skip } from ... sort of stuff) - once that's done this rule should be a matter of adding a isImported boolean prop to that return and then have a rule which checks for that...

Im planning to merge that PR and then do this follow up work over the next couple of weeks :) (I've had a few things pop up at work and need to do some stuff on my osv-detector project, which is why I've not merged yet)

[SimenB]

Note that you can run jest with injectGlobals: false and at least get runtime errors today. I doubt we'll ever change the default, but who knows 🙂 It's at least only a config option away

[ecraig12345]

My main interest was actually in easing migration to not using globals by having an automatically fixable rule. Possibly also enabling gradual migration (where @types/jest is still referenced, but the lint rule is enabled), which might be desirable for larger repos depending on their preferred approach to changes like this. Granted there are other possible approaches to doing migrations, but taking advantage of tooling (eslint) that people already have installed could be a convenient way to do it.

@G-Rath What kind of complexity/issues were you thinking auto-fix might run into? I've written lint rules before but I'm less familiar with writing fixers, and not familiar yet with this repo's codebase.

[G-Rath]

@ecraig12345 definitely - overall I think it makes sense for us to have the rule given how small it should be and it will be a lot easier to use than the alternatives you can do today.

In terms of complexity of auto-fixing I thought it through as I was replying and realised I don't think it's as initially complex as I thought - we should be able to safely find the top level of the whole file (we're doing that in other rules) and insert the import.

We should also be able to handle adding to an existing import, as that information should also already be being captured by way of the scope reference checking (it just needs to be exposed).

I think for now the biggest question is if we can actually make this an auto fix or if it has to be a suggestion - the trouble is the risk of conflicting with existing definitions. While we'll already know if a reference exists in the current scope, we won't know if there's another scope that has a reference with the same name that'd then be shadowed, e.g.

describe('x', () => {
  const it = createScopedIt('runs');

  it('fine', () => { ... });
});

describe('y', () => {
  it('is the jest method', () => { ... });
});

Remember the general expectation for auto fixers is that they're not breaking - from what I've seen the community tends to lean on the side of caution even when there's just weird probably-nonsense edge cases like this.

I'm not worried about it right now though, as the first thing is to just create the rule (technically the second thing 😅) then we can figure out the finer details :)

[ecraig12345]

@G-Rath I started working on this and had a few questions. (Also noticed the scope issue that you already fixed, nice!)

• What do you think the rule name should be? I was calling it prefer-jest-globals, but that's potentially ambiguous: it could be understood as either "prefer importing from @jest/globals" or "prefer using ambient jest globals."

• isExpectCall doesn't currently include scope checking--should that be added, and all the usage updated? (separate PR?)

• Should there be a similar helper added for detecting jest member calls, or implement that within the rule itself for now?

• collectReferences is repeatedly called within multiple helper functions while checking the same call expression--do you have any sense of whether that's expensive enough that pre-calculating and passing it around would be worthwhile? (This would have to be done either per call expression or somehow per scope.)

[G-Rath]

@ecraig12345 I love your enthusiasm but it sounds like you're we've doubling up as I've been implementing the same thing via #1106 - I think I'm almost at the point of doing the next PR as of this morning, so hoping to have it landed by early next week, at which point implementing this rule should be a pretty quick job.

I'm effectively re-writing all of these utils to be a core parseJestFn which is a expanded version of parseExpectCall (who'll end up being the expect branch of this function), and which all rules will probably end up calling as one of their first actions.

Landing #1106 should take care of your second point (as it moves expect to also be scope-based), and I'm hoping it should make it easy to support member checking off jest (as I think it should just be a matter of extending the parser to handle the jest. member expression as a special case).

Regarding performance: it should be fine - we've never had any complaints on performance and what we're doing is all in memory so should be pretty fast; having said that I do plan to do a second-pass over the codebase (mixed with some light performance testing for peace of mind) once all this has been done as I know we definitely are doing a bunch of the same work over and over which we might be able to optimise with a little restructuring and maybe introduce some caching. I'm hesitant to add it initially though because it makes testing a lot harder vs there not currently being a noticeable problem (javascript is typically really performant with these kind of in-memory calls).

re the rule name, what about prefer-importing-jest? We've already got no-jest-import which currently checks for jest itself but I think it would be valid to have it also cover @jest/globals (@SimenB would you say that's a breaking change? if so we can put it behind an option for now)

[ecraig12345]

I'm fine with waiting on the work you have in progress.

I had noticed the no-jest-import rule as well. Expanding it to cover @jest/globals seems like a breaking change, especially with that rule being part of the recommended config. The intent is also different: importing from jest does literally nothing and is never desirable that I'm aware of, whereas whether to import from @jest/globals vs. using ambient globals (and referencing @types/jest for typescript) is a style preference.

[G-Rath]

Ah I'd forgotten we had it as a recommended - I think it would still make sense to use that rule for this since it's a stylistic choice so we can just put it behind an option.

Having said that, I am actually now wondering if we should be creating a new singular rule all together because:

• no-jest-import was created "back in the day" when @jest/globals wasn't a thing at all (i.e it is now valid to do import { jest } from '@jest/globals')
• I think in practice the options are mutually exclusive (and if you "mixed" then just don't enable the result 🤷) which you can enforce with one rule (with two you can enable both and then watch them fight)
  • I typically think it's best if we also default to "do nothing" for things that are very stylistic like this, meaning we could keep the current recommended behaviour
• I think it's reasonable to have such a rule also checking if people try and import from jest as a side thing

I'll have a think on this for future.

[piotrpalek]

did this move forward somehow? is there an eslint rule to enforce importing from @jest/globals now?

[G-Rath]

@piotrpalek no we've not yet got a rule for this, though you can enforce it at runtime via config.