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

Allow jest.config.cts #14070

Merged
merged 12 commits into from
Jan 5, 2024
Merged

Allow jest.config.cts #14070

merged 12 commits into from
Jan 5, 2024

Conversation

DerTimonius
Copy link
Contributor

@DerTimonius DerTimonius commented Apr 13, 2023

Summary

This PR will allow loading jest.config.cts files for ESM projects like proposed in PR #14064 and issue #13118
Additionally, this PR will add fixtures and snapshots that should fix the tests.

Closes #14064

Test plan

@mrazauskas
Copy link
Contributor

Good idea. These are the missing parts:

@DerTimonius
Copy link
Contributor Author

@mrazauskas Thanks for the hints, I added some things now, please tell me if something is missing 🙂

@mrazauskas
Copy link
Contributor

Thanks!

Did you try running these new e2e tests? To do so, you build the repo and run yarn test-ts --selectProjects ts-integration. It is passing on main, but if I checkout your branch few tests fail. They all have "CTS config" in their names.

CHANGELOG.md Outdated Show resolved Hide resolved
@DerTimonius
Copy link
Contributor Author

Interesting, I ran yarn test after writing the tests and did not get any errors. Will get into this later today!

@DerTimonius
Copy link
Contributor Author

I investigated the failing tests and was able to recognize two separate issues:

  1. I can't use imports
  2. TS is not really working in those files. I tried a different strategy to import the Config type and got the following error:
"const config: Config.InitialOptions = {displayName: 'ts-object-config', verbose: true};\n" +
        '      ^^^^^^\n' +
        '\n' +
        'SyntaxError: Missing initializer in const declaration\n' +

I then tried to move it to the back using as Config.InitialOptions but this also did not work.

I don't have any idea how to solve this...

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 29, 2023

CLA Signed

The committers listed above are authorized under a signed CLA.

@netlify
Copy link

netlify bot commented Apr 29, 2023

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit a71fab1
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/6597ab59675f0c00087d55b7
😎 Deploy Preview https://deploy-preview-14070--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.

@DerTimonius
Copy link
Contributor Author

Still trying, but still really have no idea how to fix the issue that the .cts files appear not to be recognized as valid TypeScript...
Maybe some hints?

@karlhorky
Copy link
Contributor

karlhorky commented Jun 17, 2023

@DerTimonius can you post a new short summary about what the problem is?

From the error message and your conclusions above, it seems like the tests are not running .cts files as TypeScript - maybe as part of your summary, check into how the tests are running Jest? Maybe also useful would be to check if Jest works with .cts files outside of the testing environment.

Maybe @mrazauskas can assist...?

@karlhorky
Copy link
Contributor

One thing that I can see without looking too deeply into it is that you have an export in your .cts file:

Screenshot 2023-06-17 at 19 46 53

Since .cts is compiled to CommonJS, maybe you need to use the module.exports = {} format as you see in the other jest.config.cjs fixture:

/**
* Copyright (c) Meta Platforms, Inc. and affiliates.
*
* This source code is licensed under the MIT license found in the
* LICENSE file in the root directory of this source tree.
*/
module.exports = {};

@karlhorky
Copy link
Contributor

@DerTimonius
Copy link
Contributor Author

DerTimonius commented Jun 17, 2023

@karlhorky Sure. The issues I'm currently facing are solely in the new integration tests that were necessary to write.

Initially I got an error, telling me that I can't use import in the .cts files. I changed the type import, but I'm pretty sure that this is the first issue.
If I'm running the tests now I get the following error

"const config: Config.InitialOptions = {displayName: 'ts-object-config', verbose: true};\n" +
        '      ^^^^^^\n' +
        '\n' +
        'SyntaxError: Missing initializer in const declaration\n' +

It appears to me that the normal TypeScript code is not recognized as such - if I change the code to this:

const config = {displayName: 'ts-object-config', verbose: true} as Config.InitialOptions

I get almost a different error, telling me that as is an unknown token.

What I tried so far

  • I looked a bit in the TS docs regarding .cts files and found that maybe it is necessary to add "module": "NodeNext" to the tsconfig.json file. I then put it in the tsconfig of the e2e directory, but still got the same errors.
  • I then tried to adapt the tsconfig.json file in the root dir by adding "module": "NodeNext". The errors remained the same.
  • I then looked if maybe somewhere in the code the possibility for .cts files is missing and need to be added, but I'm not sure where this should be the case.

@github-actions
Copy link

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.

@SimenB
Copy link
Member

SimenB commented Oct 5, 2023

Hiya, sorry about the radio silence. Let's get this in 🙂

I tried to merge in main - the only conflict was in the changelog, but it seems like there might have been other changes causing test failures?

@SimenB SimenB removed the Stale label Oct 5, 2023
@DerTimonius
Copy link
Contributor Author

Hi @SimenB
thanks for your response!
I had issues with the tests before, because the .cts files were not recognized as TS files - so typical TS syntax did throw errors. I will take a look into this later today, maybe there has been an update somewhere that solved this in some way?

@DerTimonius
Copy link
Contributor Author

Still getting the same errors that I mentioned above (#14070 (comment))

I really don't get this error: writing TS code in a .cts file and it's not recognized...

Copy link

github-actions bot commented Jan 3, 2024

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.

@github-actions github-actions bot added the Stale label Jan 3, 2024
@@ -26,7 +27,7 @@ import {
export default async function readConfigFileAndSetRootDir(
configPath: string,
): Promise<Config.InitialOptions> {
const isTS = configPath.endsWith(JEST_CONFIG_EXT_TS);
const isTS = configPath.endsWith(JEST_CONFIG_EXT_TS || JEST_CONFIG_EXT_CTS);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
const isTS = configPath.endsWith(JEST_CONFIG_EXT_TS || JEST_CONFIG_EXT_CTS);
const isTS =
configPath.endsWith(JEST_CONFIG_EXT_TS) ||
configPath.endsWith(JEST_CONFIG_EXT_CTS);

@DerTimonius I think this is your issue - cts was never considered

Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh man. you're right 🙈
implemented this change with the last commit and fixed the tests (all running locally! they did not before, so this is a huge improvement)

@SimenB
Copy link
Member

SimenB commented Jan 4, 2024

nice! seems like we just need to fix packages/create-jest/src/__tests__/init.test.ts and it's good to go

@DerTimonius
Copy link
Contributor Author

Hm, I don't know why the last check is failing. I did not remove the file in question: /home/runner/work/jest/jest/packages/jest-cli/src/init/__tests__/tsconfig.json

@SimenB
Copy link
Member

SimenB commented Jan 5, 2024

packages/jest-cli/src/init/ have been moved to packages/create-jest - this is probably some remnant of that?

#14490

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 enabled auto-merge (squash) January 5, 2024 07:13
@SimenB SimenB merged commit b6f27ab into jestjs:main Jan 5, 2024
73 checks passed
@DerTimonius DerTimonius deleted the patch branch January 5, 2024 07:29
@karlhorky
Copy link
Contributor

karlhorky commented Jan 13, 2024

@SimenB @DerTimonius thanks for the PR, review and merge!

Look forward to trying this out in a release! Will this be a part of jest@29.8.0 or jest@30.0.0?

@karlhorky
Copy link
Contributor

@SimenB if the next minor or major releases are still going to be pending for a while, what do you think of creating and publishing a new alpha release eg. jest@30.0.0-alpha.3?

@karlhorky
Copy link
Contributor

Nice, this has been released in jest@30.0.0-alpha.3 🎉

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 Mar 23, 2024
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.

5 participants