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: add @jest/globals package for importing globals explicitly #9801

Merged
merged 6 commits into from Apr 12, 2020

Conversation

SimenB
Copy link
Member

@SimenB SimenB commented Apr 12, 2020

Summary

People have asked for it, so why not. We still add to the global env - we can remove that later if we want to (via some config flag I guess). If we want to do that it should be pretty straight forward, just add a runtime.registerGlobals function or some such and get the globals from that rather than environment.global. I don't want it to be import {test} from 'jest' as that's difficult to type correctly - a separate package can both ensure types are correct and that it's only used in a Jest environment.

Fixes #4473
Fixes #5192
Closes #7571
Closes #9306

Test plan

Integration test added

export declare type xdescribe = Global.GlobalAdditions['xdescribe'];
export declare type fdescribe = Global.GlobalAdditions['fdescribe'];

throw new Error(
Copy link
Member Author

@SimenB SimenB Apr 12, 2020

Choose a reason for hiding this comment

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

this file ensure types are correct, but also that it's not imported outside of jest. Compiled code is just the throw

@@ -12,7 +12,8 @@ import type {TestResult} from '@jest/test-result';
import type {RuntimeType as Runtime} from 'jest-runtime';
import type {SnapshotStateType} from 'jest-snapshot';

const FRAMEWORK_INITIALIZER = require.resolve('./jestAdapterInit');
const FRAMEWORK_INITIALIZER = path.resolve(__dirname, './jestAdapterInit');
const EXPECT_INITIALIZER = path.resolve(__dirname, './jestExpect.js');
Copy link
Member Author

@SimenB SimenB Apr 12, 2020

Choose a reason for hiding this comment

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

changes not related, just some local cleanup from looking into not adding the globals to the global env. Went with a simpler solution, but I liked these refactorings 🤷

@@ -23,6 +23,24 @@ const packagesWithTs = packages.filter(p =>
fs.existsSync(path.resolve(p, 'tsconfig.json'))
);

packagesWithTs.forEach(pkgDir => {
Copy link
Member Author

Choose a reason for hiding this comment

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

moved to verify before compiling

Copy link
Contributor

@jeysal jeysal left a comment

Choose a reason for hiding this comment

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

Nice workaround, although I kinda wish for a better world now where we inject things into people's test fns, and global APIs are accessed via gimmeSomeThing(__filename) etc. 😄

@@ -16,7 +16,9 @@ export type BlockMode = void | 'skip' | 'only' | 'todo';
export type TestMode = BlockMode;
export type TestName = Global.TestName;
export type TestFn = Global.TestFn;
export type HookFn = (done?: DoneFn) => Promise<any> | null | undefined;
export type HookFn = (
Copy link
Contributor

Choose a reason for hiding this comment

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

wWhy the asymmetry? Promise<unknown> and null instead of Promise<any> like in Global.TestFn, Global.HookFn, and Circus.TestFn

Copy link
Member Author

Choose a reason for hiding this comment

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

this was mostly me fixing a type error since it didn't accept void. Good point on the mismatch though - I'll push up exporting HookFn from Globals and reuse that here.

The type should ideally be Promise<void> | void, but then we don't get type errors if you return anything else. From my testing, void only matters to the caller, the callee doesn't have to fulfill that contract, it's just that the returned value cannot be used

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, nice! Can change then 👍 Should probably keep Promise<unknown> just so people can do test('t', () => promiseThing())

Copy link
Contributor

@jeysal jeysal Apr 12, 2020

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

ah, there you go! Adding |undefined makes it behave like we want, though

throw new Error(`Package ${pkg.name} is missing \`types\` field`);
}

if (!pkg.typesVersions) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also check the content? Just to make sure they are all the same since they do all use the same downleveling build process.

Copy link
Member Author

Choose a reason for hiding this comment

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

meh 😀 This is more "did you really mess up" than any thorough check

@SimenB
Copy link
Member Author

SimenB commented Apr 12, 2020

Nice workaround, although I kinda wish for a better world now where we inject things into people's test fns, and global APIs are accessed via

100% agreed, this is a first step 😀

@@ -16,7 +16,7 @@ export type BlockMode = void | 'skip' | 'only' | 'todo';
export type TestMode = BlockMode;
export type TestName = Global.TestName;
export type TestFn = Global.TestFn;
export type HookFn = (done?: DoneFn) => Promise<any> | null | undefined;
Copy link
Member Author

Choose a reason for hiding this comment

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

we don't actually accept null, we check for void 0 which is an non-strict mode way of saying undefined

@FezVrasta
Copy link

Is it now possible to use Jest with Flow without having to install the flow-types definitions?

@SimenB
Copy link
Member Author

SimenB commented Apr 12, 2020

No, we don't ship Flow definitions, only TypeScript.

@silverwind
Copy link
Contributor

Bit of a shame those are not exported from jest.

import/no-extraneous-dependencies would force a @jest/globals dependency that may get incompatible with jest if not updated in tandem. Not sure I like this over just using the globals.

@SimenB
Copy link
Member Author

SimenB commented Apr 13, 2020

They'll always get updated together, so shouldn't get out of sync in practice. We haven't changed our global API in years, so I doubt they'll drift anytime soon 🙂

@SimenB
Copy link
Member Author

SimenB commented Apr 13, 2020

@silverwind main reason for not implementing it in the jest package is types - we already have code in jest and making that play nice with types is hacky I think.

https://github.com/facebook/jest/blob/c83051789cc60789f2c575374270236e3a428879/packages/jest/src/jest.ts

Those imports are pretty useless, so one thing we could do is make jest just be a re-export of the globals in Jest 26.

@SimenB
Copy link
Member Author

SimenB commented Apr 13, 2020

One limitation I thought of now is that jest.mock calls where jest is imported from @jest/globals won't be hoisted, so mocking might silently not work... I'll give it a whack

@jeysal
Copy link
Contributor

jeysal commented Apr 13, 2020

True, should be easy to fix in Babel plugin jest hoist though

@SimenB
Copy link
Member Author

SimenB commented Apr 13, 2020

I tried (and failed) in #9806, help appreciated 🙂

@SimenB
Copy link
Member Author

SimenB commented Apr 19, 2020

Need to make a release, so I'll be reverting this until we're able to fix the Babel plugin (#9806). Will hopefully be included in the next release!

@github-actions
Copy link

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.
Please note this issue tracker is not a help forum. We recommend using StackOverflow or our discord channel for questions.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators May 11, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

import jest functions instead of having globals Add ability to explicitly import global variables
5 participants