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

Support @jest/globals #556

Closed
SimenB opened this issue Apr 15, 2020 · 15 comments · Fixed by #1094
Closed

Support @jest/globals #556

SimenB opened this issue Apr 15, 2020 · 15 comments · Fixed by #1094

Comments

@SimenB
Copy link
Member

SimenB commented Apr 15, 2020

See jestjs/jest#9801.

We need to support finding expect, test, it etc when imported from @jest/globals. Should be relatively simple for ESM syntax, possibly a bit more pain for require. We need to support aliasing as well import { test as testFunction } from '@jest/globals'

Note that the feature hasn't been released yet (blocked by jestjs/jest#9806), but we can add support before it lands.

/cc @G-Rath

@G-Rath
Copy link
Collaborator

G-Rath commented Apr 15, 2020

Hmmm this should mainly be a matter of inverting scopeHasLocalReference to test if a reference is an import from @jest/globals.

I'll have a toy around sometime over the weekend.

In saying that...

On the one hand arguably we don't have to do much to support them because as long as the globals are around (and doubly so if they're the default) then we always have to assume what could be the globals are the globals (aka what we do now).

On the other hand we could have the rules check if the env or globals are specified in the config Nope, near as I can tell eslint doesn't provide us a way to check what envs are enabled, nor what globals (because why would that ever be useful right? 🙄). In fact globals work inversely: if they're in globals they're not included in the global scope.

So we'd have to have a plugin setting for configuring if the global export is used 🤷

@SimenB
Copy link
Member Author

SimenB commented Apr 15, 2020

I think we should check for the globals regardless - if the user don't want the globals to be used they should remove the global config. Only reason for introducing a plugin config would be for perf optimization - skipping searching the scope for the global. I doubt that'd make a big difference, though.

But yeah, in theory this should "just" be tweaking scopeHasLocalReference and seeing where what's referenced comes from

@G-Rath
Copy link
Collaborator

G-Rath commented Apr 15, 2020

I think we should check for the globals regardless... Only reason for introducing a plugin config would be for perf optimization

I don't get what you mean - the problem is that we can't check if globals have been configured from within rules, so we have no way to magically detect if the user is wanting to use @jest/globals or not; hence we need a setting property either way.

i.e we figure out that expect is a global - then we need to know if export is defined as "our" jest global, or if we should be looking for @jest/globals.

I guess maybe we might be able to the globalScope.through to see if the identifier we've found is missing but I don't know how reliable that is :/


actually a better way of framing it:

if the user don't want the globals to be used they should remove the global config

We can't detect that, as we don't have access to the global config at lint-time from within the rules.

We only get passed the settings property.

@SimenB
Copy link
Member Author

SimenB commented Apr 15, 2020

We shouldn't care what globals are configured for ESLint, we should always search for the globals Jest puts into the environment regardless of what ESLint considers as globals.

@G-Rath
Copy link
Collaborator

G-Rath commented Apr 15, 2020

Oh are you saying that we should implementing that behaviour, "and also btw" we'll need amend it to support when importing from @types/jest as that will break our "is it a global" checks?

@SimenB
Copy link
Member Author

SimenB commented Apr 15, 2020

Yup! I think that's reasonable

@G-Rath G-Rath self-assigned this Apr 17, 2020
@SimenB
Copy link
Member Author

SimenB commented May 3, 2020

This has shipped in 25.5. Messaging in https://github.com/jest-community/eslint-plugin-jest/blob/master/docs/rules/no-jest-import.md should probably account for this new package

@G-Rath
Copy link
Collaborator

G-Rath commented May 3, 2020

Apologies I havn't had the time to do much on this, but agree updating that documentation would be a good first step.

@SimenB
Copy link
Member Author

SimenB commented May 3, 2020

Nothing to apologize for! That's just the way of FOSS 🙂

@tettoffensive
Copy link

I'm not sure if this is related, (if not, i can start a new issue) but I'm finding that eslint isn't finding the globals defined in my setupFiles

./test/setup.js

import Enzyme, { configure, shallow, mount, render } from 'enzyme';
import Adapter from 'enzyme-adapter-react-16';

configure({ adapter: new Adapter() });

// Make Enzyme functions available in all test files without importing
global.shallow = shallow;
global.render = render;
global.mount = mount;

export { shallow, mount, render };
export default Enzyme;

But when I try to use shallow in a test file:

Cannot find name 'shallow'.ts(2304)
'shallow' is not defined. eslint(no-undef)

I've tried the following with no change:

env: {
      "jest/globals": true,
    },

Is there some setting to configure this?

@G-Rath
Copy link
Collaborator

G-Rath commented May 20, 2020

shallow isn't a Jest global :)

You can tell ESLint about additional globals using the globals property.

@SimenB
Copy link
Member Author

SimenB commented Sep 6, 2020

When this lands we should also have some way of not sticking jest/globals into the recommended config:

env: { 'jest/globals': true },

@G-Rath
Copy link
Collaborator

G-Rath commented Oct 9, 2021

@SimenB just re-reviewing this issue - could you help me clarify what changes we are actually wanting to make?

I know we'll need to adjust no-jest-import in some way - I think adjust from maybe adjusting the message, the main change to make is if we continue to recommend it based on what Jest itself prefers as the default.

I think my main question I'd like to confirm is do you think we should be thinking about adjusting our isDescribeCall & co methods to check if we're either an global or an import from @jest/globals?

I'm wondering if it's actually worth it, because it'd mean it would be harder to alias any of the methods (which arguably @jest/globals makes it easier to do, since you can import the methods with an actual alias and assign to one of the expected method names, e.g. import { describe as realDescribe } from '@types/jest'; const describe = (...) => { ... };) and currently I think we might be able to replace no-hooks with no-restricted-syntax which wouldn't be possible if we tighten our checks 🤔

I think it would also make things more complicated when we try to have more support for custom names & settings (which I'm trying to do but am blocked somewhat by the current really poor way ESLint merges arrays, which has been fixed in the new config system).

@SimenB
Copy link
Member Author

SimenB commented Oct 10, 2021

I know we'll need to adjust no-jest-import in some way - I think adjust from maybe adjusting the message, the main change to make is if we continue to recommend it based on what Jest itself prefers as the default.

Jest will not (in the foreseeable future) change our defaults here (or even recommendations). Way too much code is written in the wild for that churn to be worth it.

The big wrinkle here is native ESM - that has to use @jest/globals at least for jest as there is no other way of getting that. The other globals (which are actual globals, unlike jest) will work fine in ESM.

I think my main question I'd like to confirm is do you think we should be thinking about adjusting our isDescribeCall & co methods to check if we're either an global or an import from @jest/globals?

Yes, I think so. import { describe as realDescribe } from '@jest/globals'; should hopefully be no issue as the AST will tell you that the imported identifier is describe, not realDescribe.

Looking at the logic in Jest's babel plugin might help: https://github.com/facebook/jest/blob/03f4a6967b42ca390f036a8264ca65284acb27d3/packages/babel-plugin-jest-hoist/src/index.ts#L202-L234

Different AST and maybe helpers, but still

@github-actions
Copy link

🎉 This issue has been resolved in version 26.2.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

@SimenB SimenB unpinned this issue Sep 10, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants