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

Fix type ellision of jest-runtime imports #9717

Merged
merged 3 commits into from Mar 27, 2020
Merged

Conversation

jimmydief
Copy link
Contributor

@jimmydief jimmydief commented Mar 27, 2020

Fixes #9600.

Summary

After refreshing my project's lockfile, I noticed an issue when running Jest with jest-circus as the runner due to a missing jest-runtime dependency. Making it a non-dev dependency should fix this.

Test plan

Installing the dependency at the top level fixes the issue in my project, npm seems to install it in a nested node_modules folder otherwise.

@facebook-github-bot
Copy link
Contributor

Hi @jimmydief!

Thank you for your pull request and welcome to our community.We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file.

In order for us to review and merge your code, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

If you have received this in error or have any questions, please contact us at cla@fb.com. Thanks!

@facebook-github-bot
Copy link
Contributor

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Facebook open source project. Thanks!

@SimenB
Copy link
Member

SimenB commented Mar 27, 2020

Hey, thanks for the PR! Unfortunately, this isn't the correct solution - it's a "bug" in the babel typescript transform where it isn't removed even though it's only used as a type. This is the correct fix:

diff --git i/packages/jest-circus/src/legacy-code-todo-rewrite/jestAdapter.ts w/packages/jest-circus/src/legacy-code-todo-rewrite/jestAdapter.ts
index 34d214f38..2a9561d3a 100644
--- i/packages/jest-circus/src/legacy-code-todo-rewrite/jestAdapter.ts
+++ w/packages/jest-circus/src/legacy-code-todo-rewrite/jestAdapter.ts
@@ -9,7 +9,7 @@ import * as path from 'path';
 import type {Config} from '@jest/types';
 import type {JestEnvironment} from '@jest/environment';
 import type {TestResult} from '@jest/test-result';
-import Runtime = require('jest-runtime');
+import type {RuntimeType as Runtime} from 'jest-runtime';
 import type {SnapshotStateType} from 'jest-snapshot';
 
 const FRAMEWORK_INITIALIZER = require.resolve('./jestAdapterInit');
diff --git i/packages/jest-runtime/src/index.ts w/packages/jest-runtime/src/index.ts
index 11bb29aa2..7c1390ebc 100644
--- i/packages/jest-runtime/src/index.ts
+++ w/packages/jest-runtime/src/index.ts
@@ -64,6 +64,8 @@ type CacheFS = {[path: string]: string};
 
 namespace Runtime {
   export type Context = JestContext;
+  // ditch this export when moving to esm - for now we need it for to avoid faulty type elision
+  export type RuntimeType = Runtime;
 }
 
 const testTimeoutSymbol = Symbol.for('TEST_TIMEOUT_SYMBOL');

Wanna update your PR to have this instead?

EDIT: actually, your change is probably also needed, or the typings file is wrong. But it's not required at runtime, so the diff above should get in as well 🙂

EDIT2: Same issue in jasmine actually, but there it's already declared in dependencies. Fix:

diff --git i/packages/jest-jasmine2/src/index.ts w/packages/jest-jasmine2/src/index.ts
index d87e3451f..f8ab81713 100644
--- i/packages/jest-jasmine2/src/index.ts
+++ w/packages/jest-jasmine2/src/index.ts
@@ -10,7 +10,7 @@ import type {Config, Global} from '@jest/types';
 import type {AssertionResult, TestResult} from '@jest/test-result';
 import type {JestEnvironment} from '@jest/environment';
 import type {SnapshotStateType} from 'jest-snapshot';
-import Runtime = require('jest-runtime');
+import type {RuntimeType as Runtime} from 'jest-runtime';
 
 import {getCallsite} from '@jest/source-map';
 import installEach from './each';

@jimmydief
Copy link
Contributor Author

Thanks, makes sense!

@jimmydief jimmydief changed the title Make jest-runtime a non-dev dependency of jest-circus Fix type ellision of jest-runtime imports Mar 27, 2020
@SimenB SimenB merged commit db055c2 into jestjs:master Mar 27, 2020
@SimenB
Copy link
Member

SimenB commented Mar 27, 2020

Thanks!

@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.

jest-circus is missing dependency on jest-runtime
3 participants