-
-
Notifications
You must be signed in to change notification settings - Fork 6.5k
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
Conversation
export declare type xdescribe = Global.GlobalAdditions['xdescribe']; | ||
export declare type fdescribe = Global.GlobalAdditions['fdescribe']; | ||
|
||
throw new Error( |
There was a problem hiding this comment.
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'); |
There was a problem hiding this comment.
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 => { |
There was a problem hiding this comment.
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
There was a problem hiding this 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. 😄
packages/jest-types/src/Circus.ts
Outdated
@@ -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 = ( |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, looks like it does matter https://www.typescriptlang.org/play?#code/MYewdgzgLgBAHgLhgNxASwCYwLwwCwBMA3AFABmArmMFGuDGQBQCUSqmMA3iTLzAE4BTKBX5h8xEgF8gA
There was a problem hiding this comment.
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())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, nevermind, this might be the problem https://www.typescriptlang.org/play?#code/MYewdgzgLgBAZgLhgCgJQwLwD4YDcQCWAJpiutjACwBMA3EA
There was a problem hiding this comment.
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) { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
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; |
There was a problem hiding this comment.
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
Is it now possible to use Jest with Flow without having to install the flow-types definitions? |
No, we don't ship Flow definitions, only TypeScript. |
Bit of a shame those are not exported from import/no-extraneous-dependencies would force a |
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 🙂 |
@silverwind main reason for not implementing it in the Those imports are pretty useless, so one thing we could do is make |
One limitation I thought of now is that |
True, should be easy to fix in Babel plugin jest hoist though |
I tried (and failed) in #9806, help appreciated 🙂 |
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! |
…ls explicitly (jestjs#9801)"" This reverts commit 75ab1ab.
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. |
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 thanenvironment.global
. I don't want it to beimport {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