feat: enable type check when isolatedModules and diagnostics === true #4868
Conversation
WalkthroughThe changes refactor test setup and teardown for diagnostics and configuration logic, add new and more precise diagnostics-related test cases, and reorganize the configuration initialization sequence for clarity. The TypeScript transpile module now explicitly performs type-checking and includes type-check diagnostics in its output, improving diagnostic coverage. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant transpileWorker
participant ts.Program
Caller->>transpileWorker: Call with input, options, and diagnostics enabled
transpileWorker->>ts.Program: Create for syntactic analysis
ts.Program-->>transpileWorker: Return syntactic diagnostics
transpileWorker->>ts.Program: Create with noEmit: true for type-checking
ts.Program-->>transpileWorker: Return type-check diagnostics
transpileWorker->>Caller: Return output with combined diagnostics
Possibly related issues
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm warn config production Use ✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
| const findConfig = jest.spyOn(ts, 'findConfigFile') | ||
| const readConfig = jest.spyOn(ts, 'readConfigFile') | ||
| const parseConfig = jest.spyOn(ts, 'parseJsonConfigFileContent') |
There was a problem hiding this comment.
Addressing undesired side effects between tests.
| }) | ||
|
|
||
| afterEach(() => { | ||
| findConfig.mockClear() |
There was a problem hiding this comment.
Already restoring mocks above
| spyFindTsConfigFile.mockRestore() | ||
| }) | ||
|
|
||
| it('should show warning log when isolatedModules: true is used in transformer options when a tsconfig file path for tests exists', () => { |
There was a problem hiding this comment.
This test suffered from the side effect described above
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/transpilers/typescript/transpile-module.spec.ts (2)
352-359: Make the assertion resilient to future TypeScript wording changes
messageTextis compared for full string equality. TypeScript occasionally tweaks diagnostic strings (punctuation / wording / quotes) between patch releases which can lead to brittle test failures even though the behaviour is still correct.
A looserstringContaining(or checking onlycode) is usually sufficient.-expect(result.diagnostics).toContainEqual( - expect.objectContaining({ - code: 5109, - messageText: - "Option 'moduleResolution' must be set to 'Node16' (or left unspecified) when option 'module' is set to 'Node16'.", - }), -) +expect(result.diagnostics).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + code: 5109, + messageText: expect.stringContaining('moduleResolution'), + }), + ]), +)
388-393: Reduce brittleness of the type-error expectationSame concern as above: asserting the full diagnostic text ties the test to one TS version. Matching the error code (and maybe a substring) is enough to prove the feature works.
- expect(result.diagnostics).toContainEqual( - expect.objectContaining({ - code: 2322, - messageText: "Type 'number' is not assignable to type 'string'.", - }), - ) + expect(result.diagnostics).toEqual( + expect.arrayContaining([ + expect.objectContaining({ + code: 2322, + messageText: expect.stringContaining('not assignable'), + }), + ]), + )src/legacy/config/config-set.ts (1)
301-315: Simplify the ternary – remove the “useless” boolean literal branchStatic-analysis is right here:
this.parsedTsConfig.options.isolatedModules === true ? false : truecan be replaced with!this.parsedTsConfig.options.isolatedModules, which is shorter and clearer.- const diagnosticsOpt = - options.diagnostics !== undefined - ? options.diagnostics - : /** - * Ensure that diagnostics always has false value when isolatedModules: true - * when users don't provide the value for it in Jest config. - * @TODO This behavior will be switched oppositely in the next major version. - * https://github.com/kulshekhar/ts-jest/issues/4859 - */ - this.parsedTsConfig.options.isolatedModules === true - ? false - : true + const diagnosticsOpt = + options.diagnostics !== undefined + ? options.diagnostics + : /** + * Default: disable diagnostics when `isolatedModules` is true. + * @TODO Switch behaviour in next major – see https://github.com/kulshekhar/ts-jest/issues/4859 + */ + !this.parsedTsConfig.options.isolatedModules🧰 Tools
🪛 Biome (1.9.4)
[error] 311-313: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with(lint/complexity/noUselessTernary)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/legacy/config/config-set.spec.ts(4 hunks)src/legacy/config/config-set.ts(3 hunks)src/transpilers/typescript/transpile-module.spec.ts(2 hunks)src/transpilers/typescript/transpile-module.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/legacy/config/config-set.ts
[error] 311-313: Unnecessary use of boolean literals in conditional expression.
Simplify your code by directly assigning the result without using a ternary operator.
If your goal is negation, you may use the logical NOT (!) or double NOT (!!) operator for clearer and concise code.
Check for more details about NOT operator.
Unsafe fix: Remove the conditional expression with
(lint/complexity/noUselessTernary)
🔇 Additional comments (4)
src/transpilers/typescript/transpile-module.ts (1)
175-184:Details
❌ Incorrect review comment
Avoid building a second
Program– reuse the one you already haveCreating a brand-new
ts.Programfor type-checking:
- Duplicates work the first
programhas already done.- Increases memory/time footprint for every file transpiled.
- Risks double-reporting syntactic diagnostics.
All semantic diagnostics can be queried from the existing
program:- // Explicitly perform typecheck, if required - const typeCheckProgram = ts.createProgram(inputs, { - ...transpileOptions.compilerOptions, - noEmit: true, // We're only type-checking - }) - diagnostics.push(...ts.getPreEmitDiagnostics(typeCheckProgram)) + // Get semantic / global diagnostics from the existing program + diagnostics.push(...program.getSemanticDiagnostics()) + diagnostics.push(...program.getGlobalDiagnostics())If the intent is specifically to bypass the
isolatedModuleslimitation, consider cloningoptionswith{ isolatedModules: false }before building the firstPrograminstead of paying the cost twice.Would you like a follow-up PR benchmark (or proof-of-concept) showing the performance delta between both approaches?
🏁 Script executed:
#!/bin/bash set -e # Locate the transpile-module.ts file file=$(fd --hidden --absolute-path "transpile-module.ts") echo "Found file: $file" # Display context around where 'program' and 'typeCheckProgram' are defined sed -n '1,200p' "$file" echo "----" sed -n '150,260p' "$file"Length of output: 9195
Ignore the “reuse the existing
Program” suggestionThe primary
programis built with a customCompilerHostthat only loads the in-memory source file (and a barebones lib). A secondts.Programusing the default TS host is required to resolve modules on disk and perform a full type‐check of imports and project files.Likely an incorrect or invalid review comment.
src/legacy/config/config-set.spec.ts (3)
800-815: Excellent refactoring for better test isolation!The migration from top-level spy creation with
mockClear()to properbeforeEach/afterEachhooks significantly improves test isolation and follows Jest best practices. This ensures each test starts with fresh spies and prevents potential test interference.
1133-1173: Comprehensive test coverage for the core PR functionality!This new test section excellently covers the interaction between
diagnosticsandisolatedModulessettings, which is the core functionality addressed in this PR. The parameterized test approach efficiently validates all key scenarios:
- Default behavior when
isolatedModulesis true (diagnostics ignored)- Override behavior when
diagnosticsis explicitly set to true- Standard behavior in other combinations
The use of
@ts-expect-errorto test the private method_shouldIgnoreDiagnosticsForFileis appropriate for this testing context.
1177-1185: Consistent application of improved spy management!This change maintains consistency with the earlier spy setup improvements, applying the same
beforeEach/afterEachpattern for proper test isolation in theisolatedModulestest section.
| * @internal | ||
| */ | ||
| private _setupConfigSet(options: TsJestTransformOptions['transformerConfig']): void { | ||
| // tsconfig |
There was a problem hiding this comment.
Moved this block up to make parsedTsConfig available at diagnostic handling time
| this.raiseDiagnostics(this.parsedTsConfig.errors, configFilePath) | ||
|
|
||
| this.logger.debug({ tsconfig: this.parsedTsConfig }, 'normalized typescript config via ts-jest option') | ||
| this.logger.debug({ diagnostics: this._diagnostics }, 'normalized diagnostics config via ts-jest option') |
There was a problem hiding this comment.
This log was previously executed in the // tsconfig block.
| diagnostics.push(...program.getSyntacticDiagnostics(sourceFile)) | ||
|
|
||
| // Explicitly perform typecheck, if required | ||
| const typeCheckProgram = ts.createProgram(inputs, { |
There was a problem hiding this comment.
Please double check this implementation is solid enough :)
1 participant
Motivation
TS config
isolatedModulesdisables type check. #4859Solution (just a RFC)
ConfigSet: force diagnostics tofalsewhenisolatedModulesis truetsTranspileModule: perform type check whenreportDiagnosticsis trueDemo
I've temporarily tested the change in this repro and it seems to work.
Does this PR introduce a breaking change?
Technically yes (in case of
isolatedModulesanddiagnostics=== true). But it could be interpreted as a fix.Other information
This PR is an attempt to implement the suggestions kindly provided in this thread: #4859
I updated
ConfigSetandtsTranspileModuleunit tests but I'm wondering whether there other higher level tests we might extend to cover this scenario.Summary by CodeRabbit
Refactor
New Features
Tests