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

fix: allow modern moduleResolution in project's tsconfig #14739

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

akwodkiewicz
Copy link

@akwodkiewicz akwodkiewicz commented Dec 4, 2023

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

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.
Copy link

netlify bot commented Dec 4, 2023

Deploy Preview for jestjs ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 2a7a0cf
🔍 Latest deploy log https://app.netlify.com/sites/jestjs/deploys/663353fb340756000845ed85
😎 Deploy Preview https://deploy-preview-14739--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.

@akwodkiewicz akwodkiewicz changed the title fix: allow modern moduleResolution in project tsconfig fix: allow modern moduleResolution in project's tsconfig Dec 4, 2023
@SimenB
Copy link
Member

SimenB commented Dec 24, 2023

Thanks! Could you add a test as well? An integration test is probably the thing here

@akwodkiewicz
Copy link
Author

Sure thing, I've never done it before in this project, but I'll try to deliver it in the upcoming days.

@akwodkiewicz
Copy link
Author

akwodkiewicz commented Jan 9, 2024

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 runJest testing util, so I got false positives when running tests before the fix.

Then I realized we most likely have to place the test inside jest-config's test suites, but my test ends with the following error:

Error: Jest: Failed to parse the TypeScript config file __fixtures__/readConfigFileTsNodeCompatibility/bundler/jest.config.ts
  Error: You need to run with a version of node that supports ES Modules in the VM API. See https://jestjs.io/docs/ecmascript-modules
    at readConfigFileAndSetRootDir (/Users/andrzejwodkiewicz/Repos/jest/packages/jest-config/src/readConfigFileAndSetRootDir.ts:43:13)
    at processTicksAndRejections (node:internal/process/task_queues:95:5)

Node.js v18.18.2
 FAIL  packages/jest-config/src/__tests__/readConfigFileTsNodeCompatibility.test.ts
  ● Test suite failed to run

    Jest worker encountered 4 child process exceptions, exceeding retry limit

      at ChildProcessWorker.initialize (packages/jest-worker/build/index.js:804:21)

The idea was to create the actual folders representing various flavours of projects that trigger the bug and place them inside a new __fixtures__ folder:

pwd
/Users/andrzejwodkiewicz/Repos/jest/packages/jest-config/src/__tests__

tree
.
├── Defaults.test.ts
├── __fixtures__
│   └── readConfigFileTsNodeCompatibility
│       ├── bundler                             // <- case 1
│       │   ├── jest.config.ts
│       │   ├── package.json
│       │   ├── src
│       │   │   └── test.spec.ts
│       │   └── tsconfig.json
│       ├── node-16                             // <- case 2
│       │   ├── jest.config.ts
│       │   ├── package.json
│       │   ├── src
│       │   │   └── test.spec.ts
│       │   └── tsconfig.json
│       └── node-next                           // <- case 3
│           ├── jest.config.ts
│           ├── package.json
│           ├── src
│           │   └── test.spec.ts
│           └── tsconfig.json
├── __snapshots__
│   └── normalize.test.ts.snap
├── getMaxWorkers.test.ts
├── jest-preset.json
├── normalize.test.ts
├── parseShardPair.test.ts
├── readConfig.test.ts
├── readConfigFileAndSetRootDir.test.ts
├── readConfigFileTsNodeCompatibility.test.ts    // <- my test
├── readConfigs.test.ts
├── readInitialOptions.test.ts
├── resolveConfigPath.test.ts
├── setFromArgv.test.ts
├── stringToBytes.test.ts
└── tsconfig.json

9 directories, 27 files

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();
  });
});

@akwodkiewicz
Copy link
Author

akwodkiewicz commented Jan 19, 2024

It is the await import('ts-node') line that throws the ESM error

const tsNode = await import(/* webpackIgnore: true */ 'ts-node');

In all the readConfigFileAndSetRootDir tests there is not a single test that imports a jest.config.ts file. There are mocks of requireOrImportModule, but surely I don't want to mock a ts-node import. I'm stuck.

@akwodkiewicz
Copy link
Author

@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?

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.

@github-actions github-actions bot added the Stale label Apr 23, 2024
@akwodkiewicz
Copy link
Author

@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 module: commonjs setting and move to those that cause the #14740 issue

@@ -116,6 +116,7 @@ async function registerTsNode(): Promise<Service> {
return tsNode.register({
compilerOptions: {
module: 'CommonJS',
moduleResolution: 'Node',

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.

Copy link
Author

Choose a reason for hiding this comment

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

Good idea!

Copy link
Author

Choose a reason for hiding this comment

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

Addressed in 2a7a0cf

@michaelfaith
Copy link

It is the await import('ts-node') line that throws the ESM error

const tsNode = await import(/* webpackIgnore: true */ 'ts-node');

In all the readConfigFileAndSetRootDir tests there is not a single test that imports a jest.config.ts file. There are mocks of requireOrImportModule, but surely I don't want to mock a ts-node import. I'm stuck.

Could you mock it and use require to get ts-node instead, so the tests can be in commonjs?

@akwodkiewicz
Copy link
Author

Could you mock it and use require to get ts-node instead, so the tests can be in commonjs?

If by "it" you mean the function that contains the await import(...) -- registerTsNode() -- then by mocking it I'd mock my changes so I don't think it would make a good test. The tsconfig options are defined 1 line below the await import():

async function registerTsNode(): Promise<Service> {
try {
// Register TypeScript compiler instance
const tsNode = await import(/* webpackIgnore: true */ 'ts-node');
return tsNode.register({
compilerOptions: {
module: 'CommonJS',
moduleResolution: 'Node',
},
moduleTypes: {
'**': 'cjs',
},
});

Unless you had something else in mind. Like, mocking just the await import() so it means require()? Is that possible?

This is the new preferred name of the old "Node" option.

Co-Authored-By: Michael Faith <8071845+michaelfaith@users.noreply.github.com>
@akwodkiewicz akwodkiewicz force-pushed the fix/module-resolution-mismatch branch from 6091dab to 2a7a0cf Compare May 2, 2024 08:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants