feat: node_modules preset config#5302
Conversation
📝 WalkthroughWalkthroughThis PR adds configurable transformation support for ChangesMJS Node Modules Transform Support
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/presets/create-jest-preset.spec.ts (1)
101-108:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBaseline “legacy preset” test is invoking the non-legacy factory.
Inside
describe('createDefaultLegacyPreset'), the first test currently snapshotscreateDefaultPreset(...), so it does not validate the intended baseline.Suggested fix
describe('createDefaultLegacyPreset', () => { it('should return preset config', () => { expect( - createDefaultPreset({ + createDefaultLegacyPreset({ tsconfig: 'tsconfig.spec.json', }), ).toMatchSnapshot() })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/presets/create-jest-preset.spec.ts` around lines 101 - 108, The test under describe('createDefaultLegacyPreset') is calling createDefaultPreset instead of the legacy factory; update the test to invoke createDefaultLegacyPreset with the same argument shape (e.g., { tsconfig: 'tsconfig.spec.json' }) so the snapshot covers the legacy preset path; ensure the expect(...) wraps createDefaultLegacyPreset(...) and leave the rest of the assertion (toMatchSnapshot) unchanged.
🧹 Nitpick comments (1)
src/presets/create-jest-preset.spec.ts (1)
110-112: ⚡ Quick winAdd
packageNamescoverage for the other updated CJS preset factories.This PR adds the second parameter to multiple CJS creators, but only
createDefaultPresetis snapshot-tested withpackageNames. Adding the same case(s) forcreateDefaultLegacyPreset,createJsWithTsPreset, andcreateJsWithTsLegacyPresetwould close a regression gap.Also applies to: 125-129, 142-146
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/presets/create-jest-preset.spec.ts` around lines 110 - 112, Add snapshot tests that pass the new second-argument option { packageNames: true } to the other CJS preset factories so they get the same coverage as createDefaultPreset; specifically add test cases calling createDefaultLegacyPreset(..., { packageNames: true }), createJsWithTsPreset(..., { packageNames: true }), and createJsWithTsLegacyPreset(..., { packageNames: true }) (mirroring the existing test pattern used for createDefaultPreset) and assert they toMatchSnapshot().
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/utils/node-modules-transform-pattern.ts`:
- Around line 21-23: The code currently interpolates raw packageNames into a
regex via packageNames.join('|') (used in the exclusions.push calls), which
allows regex metacharacters in package names to change the pattern; fix by
escaping regex metacharacters in each package name before joining (e.g., replace
'.' and other metacharacters via an escapeRegExp helper and use
packageNames.map(escapeRegExp).join('|')) for both places where exclusions.push
builds `(${...})/` so the generated regex matches literal package names only.
---
Outside diff comments:
In `@src/presets/create-jest-preset.spec.ts`:
- Around line 101-108: The test under describe('createDefaultLegacyPreset') is
calling createDefaultPreset instead of the legacy factory; update the test to
invoke createDefaultLegacyPreset with the same argument shape (e.g., { tsconfig:
'tsconfig.spec.json' }) so the snapshot covers the legacy preset path; ensure
the expect(...) wraps createDefaultLegacyPreset(...) and leave the rest of the
assertion (toMatchSnapshot) unchanged.
---
Nitpick comments:
In `@src/presets/create-jest-preset.spec.ts`:
- Around line 110-112: Add snapshot tests that pass the new second-argument
option { packageNames: true } to the other CJS preset factories so they get the
same coverage as createDefaultPreset; specifically add test cases calling
createDefaultLegacyPreset(..., { packageNames: true }),
createJsWithTsPreset(..., { packageNames: true }), and
createJsWithTsLegacyPreset(..., { packageNames: true }) (mirroring the existing
test pattern used for createDefaultPreset) and assert they toMatchSnapshot().
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 55f1b524-2bb1-4518-9400-910889eba11a
⛔ Files ignored due to path filters (1)
src/presets/__snapshots__/create-jest-preset.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (7)
src/constants.tssrc/legacy/ts-jest-transformer.tssrc/presets/create-jest-preset.spec.tssrc/presets/create-jest-preset.tssrc/types.tssrc/utils/index.tssrc/utils/node-modules-transform-pattern.ts
|
@ahnpnl
I would also ask if you like the nodemodules configuration as a 2nd parameter to the preset, or if you think it'd be better to add it as named option to the existing Please let me know if you like this and if there are any other changes you'd like to see. I left out website documentation from the PR for now, will document once we agree on the final approach. |
There was a problem hiding this comment.
The PR looks pretty clean. Some suggestions:
-
We should have 2 smaller PRs: one to change in
ts-jest-transformer.tsand one is changing in preset. Smaller PRs increase the readability and less time to review -
Each PR should have unit tests, e2e tests and docs updates. This PR only contains unit tests for
create-jest-preset.tsbut missing unit tests forts-jest-transformer.tsandnode-modules-transform-pattern.tsas well as missing docs updates
Or do you intent to make this PR only change for ts-jest-transformer and let #5291 takes care of preset update?
Created #5304 for transformer-only changes. If you could please review it.
I think what we should do right now is focus on #5304 and #5291 and leave this one in draft to do the preset config until they're both merged. How does that sound? |
Summary
Adds a
nodeModulesTransformOptionssecond parameter to the CJS preset functions (createDefaultPreset,createDefaultLegacyPreset,createJsWithTsPreset,createJsWithTsLegacyPreset). When provided, the preset automatically setstransformIgnorePatternsto allow the specified packages to be transpiled.New options:
packageNames— packages that should not be ignored by Jest's transform (e.g. ESM-only packages)mjsPackages— whentrue, also adds a transform entry and pattern for.mjsfiles innode_modulesExample:
The resulting config will include:
transformIgnorePatterns: ['/node_modules/(?!(some-esm-package/|.*\\.mjs$))']transformentry fornode_modules/.+\.mjs$Summary by CodeRabbit
New Features
Type Updates
Deprecations
Tests