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: allow passing TS config loader options #15234

Merged
merged 5 commits into from
Aug 4, 2024

Conversation

k-rajat19
Copy link
Contributor

@k-rajat19 k-rajat19 commented Aug 2, 2024

Summary

Follow up #15190
this will allow us to pass config loader options in a docblock comment.
Currently, valid options are swc and transpileOnly, these options are useful if someone is using ts-node for loading ts config files.

Test plan

Added e2e test

Copy link

netlify bot commented Aug 2, 2024

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 4223489
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/66af17c6a366a700080e9d2c
😎 Deploy Preview https://deploy-preview-15234--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 ready for review August 3, 2024 06:27
Comment on lines 153 to 154
swc: extraTSLoaderOptions?.swc,
transpileOnly: extraTSLoaderOptions?.transpileOnly,
Copy link
Member

Choose a reason for hiding this comment

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

Instead of setting these two, can we just spread the config passed?

i.e.

tsLoader.register({
  compilerOptions: {
    module: 'CommonJS',
  },
  moduleTypes: {
    '**': 'cjs',
  },
  ...extraTSLoaderOptions,
});

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 thought the same but not sure if it's a good idea to expose all options or not, done 4223489

Copy link
Member

Choose a reason for hiding this comment

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

if people misconfigure, that's on them. I think exposing the API is better than having to expose more in the future


if (typeof docblockTSLoaderOptions === 'string') {
extraTSLoaderOptions = JSON.parse(docblockTSLoaderOptions);
Copy link
Member

Choose a reason for hiding this comment

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

these should be passed to esbuild-register as well

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.

thanks! I like this approach 👍

@@ -89,13 +89,19 @@ export default async function readConfigFileAndSetRootDir(
}

// Load the TypeScript configuration
let extraTSLoaderOptions: Record<string, unknown>;

const loadTSConfigFile = async (
configPath: string,
): Promise<Config.InitialOptions> => {
// Get registered TypeScript compiler instance
const docblockPragmas = parse(extract(fs.readFileSync(configPath, 'utf8')));
const tsLoader = docblockPragmas['jest-config-loader'] || 'ts-node';
Copy link
Contributor Author

@k-rajat19 k-rajat19 Aug 4, 2024

Choose a reason for hiding this comment

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

should we also change this default behavior of using ts-node if no loader is passed in a separate PR? it will be a breaking change though

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 a separate thing, yeah. happy to take a PR

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.

thanks!

@SimenB SimenB merged commit ab15575 into jestjs:main Aug 4, 2024
73 checks passed
@k-rajat19 k-rajat19 deleted the loader-options branch August 4, 2024 13:33
@SimenB
Copy link
Member

SimenB commented Aug 8, 2024

Copy link

github-actions bot commented Sep 8, 2024

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 8, 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