-
-
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(jest-config): Support using esbuild-register for loading TS configs #13742
Conversation
Hi @MasterOdin! Thank you for your pull request and welcome to our community. Action RequiredIn order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you. ProcessIn order for us to review and merge your suggested changes, 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. Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with If you have received this in error or have any questions, please contact us at [email protected]. Thanks! |
Signed-off-by: Matthew Peveler <[email protected]>
402b669
to
688005c
Compare
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.
Exciting!
@@ -18,12 +18,16 @@ | |||
}, | |||
"peerDependencies": { | |||
"@types/node": "*", | |||
"esbuild-register": ">=3.1.0", |
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.
I wonder if we could resolve from the context if the config file and avoid the peer dep?
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.
I'm not sure I follow what you mean here. My view was that this was documenting optional dependencies which can add additional functionality for jest-config, but I think could just totally remove it without it affecting the code at all.
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.
Taking a step back, we need the peer dep so that we can do import(moduleSpecified)
and it resolves correctly (especially when using pnp or pnpm). However, if we resolve the module specified from the context of the config, we wouldn't need to specify the peer dep.
However, since I wanna change this in a future major so that we don't have to hard code support for modules in Jest, I think the current approach is fine for now 👍
* @jest-config-loader esbuild-register | ||
*/ | ||
export default { | ||
jestConfig: 'jest.config.ts', |
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.
Add a type annotation? Currently this file is valid 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.
Makes sense, done in 66e69a1.
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.
Hm, not sure why using @jest/types
worked locally and failed in CI, but replaced with a dummy interface in a42b463 which doesn't capture real-world usage as well, but does have TS specific syntax at least and doesn't require any sort of change to the CI.
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.
I can’t find the exact failure, but this note could be helpful. Only one job in CI builds types, all other tests run without types being build. So if you import some type from @jest/types
, type check will fail.
Integration tests for ts-node
used to read Jest config with type checks are ignored in jest.config.msj
and run through jest.config.ts.mjs
(with types build):
https://github.com/facebook/jest/blob/d2420aaf42055097dfe1e6a54bf3701214b06402/jest.config.mjs#L67
https://github.com/facebook/jest/blob/d2420aaf42055097dfe1e6a54bf3701214b06402/.github/workflows/nodejs.yml#L49-L50
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, interesting.
Should I add testcases to e2e/__tests__/tsIntegration.test.ts
for the new functionality? The testcases that check for a TS type failure won't fail the same way under esbuild-register
since it doesn't do type/typescript checking, so would need to do different error checking.
For reference:
● using esbuild-register › when `Config` type is imported from "@jest/types" › throws if type errors are encountered
expect(received).toMatch(expected)
Expected substring: "jest.config.ts(3,40): error TS2322: Type 'string' is not assignable to type 'number'."
Received string: "● Validation Error:·
Option \"testTimeout\" must be of type:
number
but instead received:
string·
Example:
{
\"testTimeout\": 5000
}·
Configuration Documentation:
https://jestjs.io/docs/configuration
"
77 | const {stderr, exitCode} = runJest(DIR);
78 |
> 79 | expect(stderr).toMatch(
| ^
80 | "jest.config.ts(3,40): error TS2322: Type 'string' is not assignable to type 'number'.",
81 | );
82 | expect(exitCode).toBe(1);
at Object.toMatch (e2e/__tests__/tsIntegration.test.ts:79:22)
● using esbuild-register › when `Config` type is imported from "@jest/types" › throws if syntax errors are encountered
expect(received).toMatch(expected)
Expected substring: "jest.config.ts(4,16): error TS2304: Cannot find name 'get'."
Received string: "Error: Jest: Failed to parse the TypeScript config file /Users/mpeveler/code/github/jest/e2e/ts-node-integration/jest.config.ts
Error: Transform failed with 1 error:
/Users/mpeveler/code/github/jest/e2e/ts-node-integration/jest.config.ts:4:19: ERROR: Expected \";\" but found \"config\"
at readConfigFileAndSetRootDir (/Users/mpeveler/code/github/jest/packages/jest-config/build/readConfigFileAndSetRootDir.js:123:13)
at async readInitialOptions (/Users/mpeveler/code/github/jest/packages/jest-config/build/index.js:396:13)
at async readConfig (/Users/mpeveler/code/github/jest/packages/jest-config/build/index.js:147:48)
at async readConfigs (/Users/mpeveler/code/github/jest/packages/jest-config/build/index.js:417:26)
at async runCLI (/Users/mpeveler/code/github/jest/packages/jest-core/build/cli/index.js:144:59)
at async Object.run (/Users/mpeveler/code/github/jest/packages/jest-cli/build/run.js:124:37)"
98 | const {stderr, exitCode} = runJest(DIR);
99 |
> 100 | expect(stderr).toMatch(
| ^
101 | "jest.config.ts(4,16): error TS2304: Cannot find name 'get'.",
102 | );
103 | expect(exitCode).toBe(1);
at Object.toMatch (e2e/__tests__/tsIntegration.test.ts:100:22)
● using esbuild-register › when `Config` type is imported from "@jest/types" › throws if type errors are encountered when package.json#type=module
expect(received).toMatch(expected)
Expected substring: "jest.config.ts(3,42): error TS2322: Type 'string' is not assignable to type 'number'."
Received string: "● Validation Error:·
Option \"testTimeout\" must be of type:
number
but instead received:
string·
Example:
{
\"testTimeout\": 5000
}·
Configuration Documentation:
https://jestjs.io/docs/configuration
"
159 | const {stderr, exitCode} = runJest(DIR);
160 |
> 161 | expect(stderr).toMatch(
| ^
162 | "jest.config.ts(3,42): error TS2322: Type 'string' is not assignable to type 'number'.",
163 | );
164 | expect(exitCode).toBe(1);
at Object.toMatch (e2e/__tests__/tsIntegration.test.ts:161:22)
● using esbuild-register › when `Config` type is imported from "@jest/types" › throws if syntax errors are encountered when package.json#type=module
expect(received).toMatch(expected)
Expected substring: "jest.config.ts(4,16): error TS2304: Cannot find name 'get'."
Received string: "Error: Jest: Failed to parse the TypeScript config file /Users/mpeveler/code/github/jest/e2e/ts-node-integration/jest.config.ts
Error: Transform failed with 1 error:
/Users/mpeveler/code/github/jest/e2e/ts-node-integration/jest.config.ts:4:21: ERROR: Expected \";\" but found \"config\"
at readConfigFileAndSetRootDir (/Users/mpeveler/code/github/jest/packages/jest-config/build/readConfigFileAndSetRootDir.js:123:13)
at async readInitialOptions (/Users/mpeveler/code/github/jest/packages/jest-config/build/index.js:396:13)
at async readConfig (/Users/mpeveler/code/github/jest/packages/jest-config/build/index.js:147:48)
at async readConfigs (/Users/mpeveler/code/github/jest/packages/jest-config/build/index.js:417:26)
at async runCLI (/Users/mpeveler/code/github/jest/packages/jest-core/build/cli/index.js:144:59)
at async Object.run (/Users/mpeveler/code/github/jest/packages/jest-cli/build/run.js:124:37)"
180 | const {stderr, exitCode} = runJest(DIR);
181 |
> 182 | expect(stderr).toMatch(
| ^
183 | "jest.config.ts(4,16): error TS2304: Cannot find name 'get'.",
184 | );
185 | expect(exitCode).toBe(1);
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 I add testcases to
e2e/__tests__/tsIntegration.test.ts
for the new functionality?
yes please 👍
Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks! |
Signed-off-by: Matthew Peveler <[email protected]>
Signed-off-by: Matthew Peveler <[email protected]>
@@ -57,7 +57,7 @@ export default async (): Promise<Config> => { | |||
|
|||
:::tip | |||
|
|||
To read TypeScript configuration files Jest requires [`ts-node`](https://npmjs.com/package/ts-node). Make sure it is installed in your project. | |||
To read TypeScript configuration files Jest by default requires [`ts-node`](https://npmjs.com/package/ts-node). You can override this behavior by adding a `@jest-config-loader` docblock at the top of the file. Currently, [`ts-node`](https://npmjs.com/package/ts-node) and [`esbuild-register`](https://npmjs.com/package/esbuild-register) is supported. Make sure `ts-node` or the loader you specify is installed. |
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 we have an example of a docblock? in case folks don't know what it is
@MasterOdin ping 🙂 |
This PR is stale because it has been open 90 days with no activity. Remove stale label or comment or this will be closed in 30 days. |
This PR was closed because it has been stalled for 30 days with no activity. Please open a new PR if the issue is still relevant, linking to this one. |
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
PR adds support for using
esbuild-register
as a support loader for thejest.config.ts
file. The user can opt into using it by using a docblock (@jest-config-loader
) and suggested as part of #13143.This PR would supercede the work done in #12041.
The motivation for the PR is that I do not use
ts-node
in my projects, rather I useesbuild
(throughtsx
) and thatesbuild-register
allows for me to reuse existing modules I've installed for my project, rather than installingts-node
just to read the jest.config.ts file.The eventual goal would be to add some additional modules (e.g.
tsx
), but that'll require a bit of upstream work first to support how jest enables and then disables the module after it's done parsing thejest.config.ts
file.Test plan
I've added a new e2e test that should pass: