Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: prefer importing jest globals [new rule] #1490

Merged
merged 16 commits into from Apr 6, 2024

Conversation

MadeinFrance
Copy link
Contributor

@MadeinFrance MadeinFrance commented Jan 16, 2024

Issue: #1101

Prefer importing Jest globals (prefer-importing-jest-globals)

🔧 This rule is automatically fixable by the
--fix CLI option.

This rule aims to enforce explicit imports from @jest/globals.

  1. This is useful for ensuring that the Jest APIs are imported the same way in
    the codebase.
  2. When you can't modify Jest's
    injectGlobals
    configuration property, this rule can help to ensure that the Jest globals
    are imported explicitly and facilitate a migration to @jest/globals.

Rule details

Examples of incorrect code for this rule

/* eslint jest/prefer-importing-jest-globals: "error" */

describe('foo', () => {
  it('accepts this input', () => {
    // ...
  });
});

Examples of correct code for this rule

/* eslint jest/prefer-importing-jest-globals: "error" */

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

describe('foo', () => {
  it('accepts this input', () => {
    // ...
  });
});

Further Reading

image

src/rules/prefer-jest-globals.ts Outdated Show resolved Hide resolved
src/rules/__tests__/prefer-jest-globals.test.ts Outdated Show resolved Hide resolved
yarn.lock Outdated Show resolved Hide resolved
src/rules/prefer-jest-globals.ts Outdated Show resolved Hide resolved
src/rules/prefer-jest-globals.ts Outdated Show resolved Hide resolved
@MadeinFrance MadeinFrance force-pushed the prefer-jest-globals branch 2 times, most recently from 29020c4 to 93157b0 Compare January 17, 2024 07:51
@SimenB
Copy link
Member

SimenB commented Jan 17, 2024

Is it possible to add an autofix for this? I.e. detecting what globals are used, and add either an import or require of them?

I think just flagging global usage can be done by not having jest/globals in the env config of ESLint, so if we add this rule, I think it should provide value above and beyond just marking them.

@MadeinFrance MadeinFrance force-pushed the prefer-jest-globals branch 3 times, most recently from 2398844 to 3b4a038 Compare January 18, 2024 09:43
src/rules/prefer-importing-jest-globals.ts Outdated Show resolved Hide resolved
src/rules/prefer-importing-jest-globals.ts Outdated Show resolved Hide resolved
@MadeinFrance
Copy link
Contributor Author

Is it possible to add an autofix for this? I.e. detecting what globals are used, and add either an import or require of them?

I think just flagging global usage can be done by not having jest/globals in the env config of ESLint, so if we add this rule, I think it should provide value above and beyond just marking them.

Hi @SimenB, I've added an auto fixer in the logic 👨🏻‍🔧

@MadeinFrance MadeinFrance force-pushed the prefer-jest-globals branch 4 times, most recently from 7843eb7 to 15c1f71 Compare January 23, 2024 08:59
src/rules/prefer-importing-jest-globals.ts Outdated Show resolved Hide resolved
src/rules/prefer-importing-jest-globals.ts Outdated Show resolved Hide resolved
src/rules/prefer-importing-jest-globals.ts Outdated Show resolved Hide resolved
@MadeinFrance MadeinFrance force-pushed the prefer-jest-globals branch 3 times, most recently from 13adf46 to 3c9c4e5 Compare January 30, 2024 08:33
@MadeinFrance MadeinFrance marked this pull request as ready for review January 30, 2024 08:36
src/rules/__tests__/prefer-importing-jest-globals.test.ts Outdated Show resolved Hide resolved
src/rules/prefer-importing-jest-globals.ts Outdated Show resolved Hide resolved
src/rules/prefer-importing-jest-globals.ts Outdated Show resolved Hide resolved
@MadeinFrance MadeinFrance force-pushed the prefer-jest-globals branch 2 times, most recently from c86e12d to 65c50fe Compare February 2, 2024 10:30
src/rules/prefer-importing-jest-globals.ts Outdated Show resolved Hide resolved
src/rules/prefer-importing-jest-globals.ts Outdated Show resolved Hide resolved
src/rules/prefer-importing-jest-globals.ts Outdated Show resolved Hide resolved
src/rules/prefer-importing-jest-globals.ts Outdated Show resolved Hide resolved
src/rules/prefer-importing-jest-globals.ts Outdated Show resolved Hide resolved
Copy link
Collaborator

@G-Rath G-Rath left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

awesome stuff getting complete coverage - I think we're almost there, just need to restore our valid test cases; I'm happy to take care of any further cleanup in post :)

src/rules/__tests__/prefer-importing-jest-globals.test.ts Outdated Show resolved Hide resolved
src/rules/__tests__/prefer-importing-jest-globals.test.ts Outdated Show resolved Hide resolved
src/rules/prefer-importing-jest-globals.ts Outdated Show resolved Hide resolved
src/rules/__tests__/prefer-importing-jest-globals.test.ts Outdated Show resolved Hide resolved
@MadeinFrance
Copy link
Contributor Author

@G-Rath can we move forward to Main with this PR? Most of the clean up is done.

@G-Rath
Copy link
Collaborator

G-Rath commented Apr 2, 2024

@MadeinFrance I'm waiting to see what's happening with our upcoming major whichll probably get released this week.

This PR looks good to me so you can consider it off your plate - I'll handle landing it and doing any revising that might be needed :)

@G-Rath
Copy link
Collaborator

G-Rath commented Apr 6, 2024

Thanks again @MadeinFrance! I have done some minor refactoring that I recommend checking out if you're interested but don't feel bad about any of it - you did the grunt work which I've just built on and I've been working with these rules for years so never expected a first-time contributor to have gotten my changes on their first rule.

The fixer is still a bit rough but I'm happy to improve that in follow-up PRs since you've already gone through a lot here and I think the roughness is just with very unlikely edge cases e.g.

  • import jest from '@jest/globals' should give import jest, { ... } rather than import { jest, ... }
  • we should retain template literals if they're used in the require
  • we shouldn't end up with the next line being dedented after adding the import
  • ideally we should include data in the test errors
  • ideally we could probably support settings.jest.globalAliases too (this is super low priority though)

@G-Rath G-Rath merged commit 37478d8 into jest-community:main Apr 6, 2024
33 checks passed
github-actions bot pushed a commit that referenced this pull request Apr 6, 2024
# [28.1.0](v28.0.0...v28.1.0) (2024-04-06)

### Features

* add `prefer-importing-jest-globals` rule ([#1490](#1490)) ([37478d8](37478d8)), closes [#1101](#1101)
Copy link

github-actions bot commented Apr 6, 2024

🎉 This PR is included in version 28.1.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@MadeinFrance
Copy link
Contributor Author

@G-Rath I'm thrilled to see this landing on the release line. Thank you for your review and patience 🙏
Seeing how this PR has grown since the initial commit is amazing.

I will use this in our corporate repos and improve the edge cases if required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants