-
-
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
fix: allow Node16/NodeNext/Bundler moduleResolution in project's tsconfig #14739
fix: allow Node16/NodeNext/Bundler moduleResolution in project's tsconfig #14739
Conversation
✅ 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 |
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 |
6091dab
to
2a7a0cf
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.
thanks!
would you mind adding an integration test (like the test added in #12397 - maybe a sibling?) and a changelog entry?
EDIT: sorry, I see you asked for help with the test - I'll take a look tomorrow. But I see you tried a unit test - that probably won't work. We need an integration test (i.e. spawn Jest and have it read the TS config file, not via a unit test)
I tried to apply this fix to the reproduction from #13350 (after running |
regardless, I pushed up a test based on #13350 that fails. If this PR is solving a different issue than that, feel free to tweak it to be for that 🙂 You can run just the test to get the full output by doing |
Ok, I see what's the issue with my PR. The problem I wanted to address here is that a really simple, self-contained But the #13350 is a variation that adds some complexity, because with a modern I'll first try to add a test case that addresses my original problem, and then I'll try to find out what exactly is responsible for the other issue. |
d2cacef
to
291df58
Compare
The issue is about ts-node not being able to properly load ESM files with those .js imports. I tried providing But then I found this: TypeStrong/ts-node#1514
I modified the test case by adding the following part to the "ts-node": {
"experimentalResolver": true
} This makes the test work -- the I don't know if we should bake this resolver into Jest, I honestly don't know how it will impact all the existing working cases. But I'm starting to wonder if it's even a good idea to hardcode any options in the ts-node registration: jest/packages/jest-config/src/readConfigFileAndSetRootDir.ts Lines 119 to 127 in 291df58
or should we pass nothing and rely on the EDIT: ...unless we want to bake the return tsNode.register({
experimentalResolver: true,
}); The idea would be that |
Happy to try that! The reason why we try to force CJS is that otherwise the |
Took a look at vite - seems they actually bundle the config file, then execute it. Something like that would work as well (and we'd drop the |
There's one more thing that bugs me -- I cannot make the solution with baked |
fae4275
to
66edbf0
Compare
@SimenB, I suppose all tests need to pass (no flaky suites you are aware of)? 😅 If that's the case then I'd suggest reverting your test case and I'll revert all the additional changes I made, so that we can at least make importless jest.config.ts work with the modern TS moduleResolution. Is that ok? |
When you were mentioning vite -- was the idea to pre-compile the config file with |
Either should work, but just using |
I'm happy to do that in a separate PR tho, if you wanna get the |
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.
This is the new preferred name of the old "Node" option. Co-Authored-By: Michael Faith <8071845 [email protected]>
66edbf0
to
d115bb8
Compare
I rewrote the history and pushed only the minimal changes to resolve #14740 on top of the current I did not keep your commit with the test case for #13350 (for obvious reasons), but it's available under hash cf7082e -- it will be useful for the other PR. I added the integration test in my last commit. I'm currently tinkering with TS programmatic API to transpile the config on-the-fly, but I'm a little bit worried this might be quite slow. Anyway, if I manage to do anything that looks like it could work, I'll submit a PR. |
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.
thanks!
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. |
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
EDIT
The PR does not close the issue #13350!
This means that the fix works only for
jest.config.ts
files that do not reference other files using.js
extensions. See #14739 (comment)