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(jest-config):Support loading TS config files via docblock loader #15190

Merged
merged 17 commits into from
Jul 23, 2024

Conversation

k-rajat19
Copy link
Contributor

@k-rajat19 k-rajat19 commented Jul 15, 2024

Summary

Unblocks #13143
Unblocks #11989

Added support for loading Typescript configuration files by specifying a loader in a docblock comment in config file.
this is a continuation of work done in #13742
one can define a loader in a docblock comment like this:

/**@jest-config-loader ts-node(or esbuild-register)*/

import type {Config} from 'jest';

const config: Config = {
  verbose: true,
};

export default config;

if there is no loader defined it uses ts-node by default for loading a config file.
currently there is support for esbuild-register and ts-node.

Test plan

Added e2e tests

Copy link

netlify bot commented Jul 15, 2024

Deploy Preview for jestjs ready!

Name Link
🔨 Latest commit 4d71390
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/669f622d772cf40008556d1b
😎 Deploy Preview https://deploy-preview-15190--jestjs.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@k-rajat19 k-rajat19 marked this pull request as draft July 15, 2024 06:39
@SimenB
Copy link
Member

SimenB commented Jul 15, 2024

Thanks for sending a PR!

this PR also opt-out using ts-node by default which was initially set as a default loader for loading ts
configs (#13143 (comment))
So Jest will not do anything by default for loading ts configs, one has to define a loader in a docblock comment like this:

I think we should change default behaviour in a separate PR, just to keep the scope per PR down a bit

@k-rajat19
Copy link
Contributor Author

I think we should change default behaviour in a separate PR, just to keep the scope per PR down a bit

Should I update docs in a separate PR ? after current one and PR which changes default behaviour lands
or in this one according to current changes :)

@k-rajat19 k-rajat19 marked this pull request as ready for review July 15, 2024 09:55
@SimenB
Copy link
Member

SimenB commented Jul 15, 2024

Updating the docs here with info about the docblock makes sense. Even though we might delete parts of the new docs when changing default behaviour 🙂

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

looks good! just some nits 🙂

docs/Configuration.md Outdated Show resolved Hide resolved
e2e/__tests__/readInitialOptions.test.ts Show resolved Hide resolved
packages/jest-config/package.json Outdated Show resolved Hide resolved
e2e/__tests__/readInitialOptions.test.ts Outdated Show resolved Hide resolved
@@ -105,36 +121,57 @@ const loadTSConfigFile = async (
return configObject;
};

let registeredCompilerPromise: Promise<Service>;
let registeredCompilerPromise: Promise<TsLoader>;
Copy link
Member

@SimenB SimenB Jul 17, 2024

Choose a reason for hiding this comment

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

I wonder if this needs to be a Map<TsLoaderModule, Promise<TsLoader>>? I'm thinking in cases where multi projects are used, and some project uses a different TS loader. That might already not work if the loaders get in the way of each other, tho.

Maybe a better thing would just be to save what module was loaded, and then throw an error if later another one is attempted to be used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added integration tests for different loaders in different projects and they seem to be working fine d2c3a8d
do we still need to save the loader and doesn't allow using a different one?

export default config;
```

If you are using `ts-node`, you can set `JEST_CONFIG_TRANSPILE_ONLY` environment variable to `true` (case insensitive) to read configuration files without typechecking.
Copy link
Member

Choose a reason for hiding this comment

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

not for this PR, but maybe in a follow-up - should we drop this new env variable and use docblock config instead (like we support for test environment: https://jestjs.io/blog/2022/04/25/jest-28#inline-testenvironmentoptions)? Then people could also e.g. opt into using swc (https://typestrong.org/ts-node/docs/swc/)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that would be better, will send PR for that
we might also need to add support for swc based loader at some point #12156

const tsLoader = docblockPragmas['jest-config-loader'] || 'ts-node';

if (Array.isArray(tsLoader)) {
throw new TypeError(
Copy link
Contributor Author

@k-rajat19 k-rajat19 Jul 18, 2024

Choose a reason for hiding this comment

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

This is not working when passing multiple loaders like this
/**@jest-config-loader ts-node esbuild-register*/ it simply returns a string 'ts-node esbuild-register' instead of returning an array of strings ['ts-node','esbuild-register']
is this an expected behavior or a bug in jest-docblock?

Copy link
Member

Choose a reason for hiding this comment

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

that's expected - for it to be an array you'd need to do

/**
 * @jest-config-loader ts-node
 * @jest-config-loader esbuild-register
 */

Copy link
Member

@SimenB SimenB left a comment

Choose a reason for hiding this comment

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

great job, thanks!

@SimenB SimenB enabled auto-merge (squash) July 23, 2024 07:56
@SimenB SimenB merged commit 798d6c0 into jestjs:main Jul 23, 2024
73 checks passed
@k-rajat19 k-rajat19 deleted the jest-config-ts branch July 23, 2024 08:33
@SimenB
Copy link
Member

SimenB commented Aug 8, 2024

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 Sep 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants