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: allow modern moduleResolution in project's tsconfig #14739
base: main
Are you sure you want to change the base?
fix: allow modern moduleResolution in project's tsconfig #14739
Conversation
This small change fixes an issue that makes using jest impossible with inside a TypeScript project which is working on NodeNext, Node16 or Bundler moduleResolution setting. The default behaviour of ts-node is to read the default tsconfig.json file from the project. With a hardcoded option of "module: CommonJs" the TypeScript compiler will throw an error, because the modern moduleResolution options used in the project are not compatible with the hardcoded module value. The only way to use jest in such a repo is to change the jest.config.ts file into a JS one (or pass the options in a different way), so that the code responsible of instantiating ts-node is not invoked. This commit fixes the issue by providing a missing complementary option, moduleResolution: Node, which will work perfectly with module: CommonJs and will not be overridden by any project-specific value in tsconfig.json.
✅ Deploy Preview for jestjs ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
Thanks! Could you add a test as well? An integration test is probably the thing here |
Sure thing, I've never done it before in this project, but I'll try to deliver it in the upcoming days. |
I have a hard time writing those integration tests. I spent two days already, without any success. First I tried to create a e2e scenario, but it turned out that the config was not even read when the test invoked the Then I realized we most likely have to place the test inside
The idea was to create the actual folders representing various flavours of projects that trigger the bug and place them inside a new
The test itself: import path = require('node:path');
import readConfigFileAndSetRootDir from '../readConfigFileAndSetRootDir';
const rootDir = path.join('__fixtures__', 'readConfigFileTsNodeCompatibility');
const moduleResolutionOptions = ['bundler', 'node-16', 'node-next'] as const;
describe('jest is correctly started from using a jest.config.ts file for a project using module/moduleResolution set to', () => {
test.each(moduleResolutionOptions)('%s', async opt => {
const readConfig = async () =>
readConfigFileAndSetRootDir(path.join(rootDir, opt, 'jest.config.ts'));
expect(readConfig).not.toThrow();
});
}); |
It is the
In all the |
@SimenB, it seems that I'd need to run the tests in the ESM mode, but that is not how all the test suites are configured to run. Can we merge the PR without an appropriate test suite? |
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, would it be possible for you to take a look and suggest how to write an integration test here? Alternatively, make the decision to skip the tests here? This PR has a solution to an issue that will affect more and more people since TS is now more openly suggesting to stay away from the |
@@ -116,6 +116,7 @@ async function registerTsNode(): Promise<Service> { | |||
return tsNode.register({ | |||
compilerOptions: { | |||
module: 'CommonJS', | |||
moduleResolution: 'Node', |
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.
Would recommend going with Node10
, which is the "new" name for this value.
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.
Good idea!
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.
Addressed in 2a7a0cf
Could you mock it and use |
If by "it" you mean the function that contains the jest/packages/jest-config/src/readConfigFileAndSetRootDir.ts Lines 112 to 124 in 3ed62f0
Unless you had something else in mind. Like, mocking just the |
This is the new preferred name of the old "Node" option. Co-Authored-By: Michael Faith <8071845+michaelfaith@users.noreply.github.com>
6091dab
to
2a7a0cf
Compare
This small change fixes an issue that makes using Jest impossible inside a TypeScript project configured with NodeNext, Node16 or Bundler moduleResolution setting if a Jest configuration file is written in TS.
The default behaviour of ts-node is to read the default tsconfig.json file from the project.
With a hardcoded option of "module: CommonJs" that is used to read the jest.config.ts file, the TypeScript compiler will throw an error, because the modern moduleResolution options are not compatible with
the hardcoded "CommonJs" module value.
The only way to use Jest in such a repo is to change the jest.config.ts file into a JS one (or pass the options in a different way), so that the code responsible of instantiating ts-node is not invoked.
This commit fixes the issue by providing a missing complementary option, moduleResolution: Node, which will work perfectly with module: CommonJs and will not be overridden by any project-specific value in tsconfig.json.
Closes #14740
Closes #13350