Conversation
🦋 Changeset detectedLatest commit: 4a896eb The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
WalkthroughThis pull request introduces new development dependencies in the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant RunnerSetup as setupTsRunner
participant VersionChecker as compareNodeVersion
participant FlagUtils as FlagUtils (hasRequireFlag, hasImportFlag, hasLoaderFlag)
User->>RunnerSetup: Initiate TS runner setup
RunnerSetup->>VersionChecker: Check Node.js version
VersionChecker-->>RunnerSetup: Return version result
RunnerSetup->>FlagUtils: Validate execution flags
FlagUtils-->>RunnerSetup: Return flag statuses
RunnerSetup->>User: Configure TS Runner (OXC/SWC) based on checks
Assessment against linked 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
src/common.tsOops! Something went wrong! :( ESLint: 9.23.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.js src/helpers.tsOops! Something went wrong! :( ESLint: 9.23.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.js src/constants.tsOops! Something went wrong! :( ESLint: 9.23.0 Error [ERR_MODULE_NOT_FOUND]: Cannot find package '@1stg/eslint-config' imported from /eslint.config.js
✨ Finishing Touches
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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 (
|
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. |
|
New, updated, and removed dependencies detected. Learn more about Socket for GitHub ↗︎
🚮 Removed packages: npm/has-flag@4.0.0 |
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for the new OXC TypeScript runner while updating version comparison logic and test conditions.
- Refactors tests to use TS_ESM_PARTIAL_SUPPORTED instead of the removed tsUseEsmSupported.
- Replaces compareVersion with compareNodeVersion in production code.
- Introduces new TsRunner.OXC support with appropriate error handling for unsupported file types.
Reviewed Changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/ts-runner.spec.ts | Updated tests and conditions, adding tests for the new OXC runner. |
| test/helpers.ts | Removed tsUseEsmSupported export. |
| test/fn.spec.ts | Updated test conditions to reference TS_ESM_PARTIAL_SUPPORTED. |
| src/index.ts | Replaced version comparison calls with new compareNodeVersion. |
| src/helpers.ts | Updated flags handling and execArgv adjustments based on new flags. |
| src/constants.ts | Added constants for OXC runner and updated version support flags. |
| src/common.ts | Added new helper compareNodeVersion based on compareVersion. |
Files not reviewed (1)
- package.json: Language not supported
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #222 +/- ##
===========================================
+ Coverage 85.76% 95.80% +10.04%
===========================================
Files 4 4
Lines 295 334 +39
Branches 138 156 +18
===========================================
+ Hits 253 320 +67
+ Misses 35 14 -21
+ Partials 7 0 -7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
commit: |
📊 Package size report 11%↑
Unchanged files
🤖 This report was automatically generated by pkg-size-action |
bb6a5bc to
da04835
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/helpers.ts (1)
531-531:⚠️ Potential issueReplace variable usage with the appropriate constant.
The code is still using the
tsUseEsmvariable on line 531, but should be usingTS_ESM_PARTIAL_SUPPORTEDfor consistency with the updated tests.- const isTsxSupported = !tsUseEsm || TS_ESM_PARTIAL_SUPPORTED + const isTsxSupported = !tsUseEsm || TS_ESM_PARTIAL_SUPPORTED
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (8)
package.json(1 hunks)src/common.ts(1 hunks)src/constants.ts(5 hunks)src/helpers.ts(8 hunks)src/index.ts(2 hunks)test/fn.spec.ts(3 hunks)test/helpers.ts(1 hunks)test/ts-runner.spec.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- src/common.ts
- test/helpers.ts
- package.json
- src/index.ts
- test/fn.spec.ts
🧰 Additional context used
🧬 Code Definitions (2)
src/constants.ts (1)
src/common.ts (1)
compareNodeVersion(29-30)
test/ts-runner.spec.ts (4)
src/constants.ts (3)
TS_ESM_PARTIAL_SUPPORTED(34-38)MTS_SUPPORTED(41-41)STRIP_TYPES_NODE_VERSION(46-46)src/index.ts (1)
createSyncFn(48-78)test/types.ts (1)
AsyncWorkerFn(1-4)src/common.ts (1)
compareNodeVersion(29-30)
🪛 GitHub Check: codecov/patch
src/helpers.ts
[warning] 69-69: src/helpers.ts#L69
Added line #L69 was not covered by tests
[warning] 189-189: src/helpers.ts#L189
Added line #L189 was not covered by tests
[warning] 204-204: src/helpers.ts#L204
Added line #L204 was not covered by tests
[warning] 211-211: src/helpers.ts#L211
Added line #L211 was not covered by tests
[warning] 218-218: src/helpers.ts#L218
Added line #L218 was not covered by tests
[warning] 250-250: src/helpers.ts#L250
Added line #L250 was not covered by tests
[warning] 257-257: src/helpers.ts#L257
Added line #L257 was not covered by tests
[warning] 543-543: src/helpers.ts#L543
Added line #L543 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Lint and Test with Node.js 23 on windows-latest
- GitHub Check: Lint and Test with Node.js 23 on macos-latest
- GitHub Check: Lint and Test with Node.js 18.18 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 23 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 18.18 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on macos-latest
🔇 Additional comments (13)
test/ts-runner.spec.ts (5)
1-17: Code structure improvement: Updated imports and constants for better version handling.The updated imports and constants improve version handling by replacing the direct version comparison with the more abstract
compareNodeVersionfunction and dedicated constants likeTS_ESM_PARTIAL_SUPPORTEDandMTS_SUPPORTED.
79-110: Excellent test implementation for the new OXC runner.The test for the OXC runner follows the same pattern as other runner tests, providing good coverage for different file types (.js, .mjs) and handling conditional testing for .mts files based on Node.js version compatibility.
155-162: Improved test condition handling for TypeScript ESM files.The logic now correctly uses
TS_ESM_PARTIAL_SUPPORTEDto determine when to test .mts files with TSX, making the tests more accurate across different Node.js versions.
135-142: Enhanced test coverage for SWC with .mts files.Added testing for SWC with .mts files when the Node.js version supports it but doesn't have partial ESM support, improving test coverage across different environments.
175-182: Improved version check implementation with new constants.Replaced the direct version comparison with the more maintainable
compareNodeVersion(STRIP_TYPES_NODE_VERSION), which centralizes version constants and makes the code easier to update if version requirements change.src/constants.ts (4)
15-17: Successfully added OXC to TypeScript runners.The OXC runner is properly added to the TypeScript runners enum with the appropriate URL comment for documentation.
33-41: Well-defined Node.js version compatibility constants.The new constants
TS_ESM_PARTIAL_SUPPORTEDandMTS_SUPPORTEDprovide clear version checks for specific Node.js compatibility features, making it easier to understand and maintain version-dependent code.
92-102: Good separation of Node.js CLI flags into constants.Extracted CLI flags into dedicated constants (
IMPORT_FLAG,REQUIRE_FLAG, etc.) and grouped related flags into sets (REQUIRE_FLAGS), which improves code maintainability and readability.
64-64: Improved version comparison with new helper function.Updated the version check in
NO_STRIP_TYPESto use the newcompareNodeVersionfunction, which is more consistent with the rest of the codebase.src/helpers.ts (4)
226-236: Properly implemented OXC TypeScript runner support.The implementation for the OXC runner correctly uses the
--importflag with the appropriate module path. The approach aligns with the documented usage pattern from the OXC project.
238-262: Enhanced SWC runner with version-specific import handling.The SWC runner implementation now intelligently chooses between
--importand--loaderflags based on the Node.js version, improving compatibility across different environments.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 250-250: src/helpers.ts#L250
Added line #L250 was not covered by tests
[warning] 257-257: src/helpers.ts#L257
Added line #L257 was not covered by tests
265-275: Improved TSX runner compatibility with newer Node.js versions.Updated the TSX runner handling to use
--importflag when supported by the Node.js version (≥20.6), falling back to--loaderfor older versions. This provides better performance and compatibility.
542-559: Better error messages with alternative TypeScript runner suggestions.Enhanced error messages now provide helpful alternatives when a runner isn't supported for specific file types, suggesting OXC or SWC as alternatives when appropriate based on Node.js version compatibility.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 543-543: src/helpers.ts#L543
Added line #L543 was not covered by tests
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/helpers.spec.ts (1)
13-30: Robust coverage forhasRequireFlag.
The tests account for both--requireand the-rshorthand, including scenarios where flags appear in different positions or are absent. Consider adding a minor test for repeated flags (e.g.,['-r', '-r', 'some-module']) if it might occur in real usage.src/helpers.ts (1)
270-275:TSXrunner coverage.
The logic includes anIMPORT_FLAGpath and a fallback to--loader. Static analysis indicates line 271 is uncovered. Adding a test case wheretsUseEsmis true (or Node version is adequate for the import flag) would enhance coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 271-271: src/helpers.ts#L271
Added line #L271 was not covered by tests
🛑 Comments failed to post (1)
src/helpers.ts (1)
238-265: 💡 Verification agent
🧩 Analysis chain
SWCrunner with ESM and CJS mode.
Great job branching based ontsUseEsmand Node version checks forIMPORT_FLAG.
Note that lines 244 and 258 are flagged as uncovered. Consider adding tests for SWC handling both--importusage and!hasRequireFlag(execArgv)paths.
Below is a shell script you can run to search for tests targeting the SWC runner’s branches (including the ones flagged as uncovered) so we can verify whether the missing test coverage for the
--importpath (line 244) and the!hasRequireFlag(execArgv)branch (line 258) is indeed an issue:Please run these queries and review the results. Once we confirm whether additional tests are needed for the uncovered branches (specifically for the
--importusage in the ESM path and the CJS branch without a require flag), we can update the review comment accordingly.
🏁 Script executed:
#!/bin/bash echo "=== Checking for Test Coverage of SWC Runner paths ===" echo "Searching for tests referencing '@swc-node/register/esm-register' (ESM branch)..." rg "@swc-node/register/esm-register" -n . echo "Searching for tests referencing '@swc-node/register/esm' (fallback ESM loader)..." rg "@swc-node/register/esm" -n . echo "Searching for tests referencing '@swc-node/register' (CJS branch)..." rg "@swc-node/register" -n . echo "Searching for tests referencing 'hasRequireFlag' to see if the CJS branch is exercised..." rg "hasRequireFlag" -n .Length of output: 2657
Action Required: Add Tests for Uncovered SWC Runner Branches
Great work handling both ESM and CJS modes for the SWC runner. However, our recent checks indicate the following concerns remain:
- ESM Branch (Line ~244): No tests were found that exercise the condition where
tsUseEsmis true and the node version supportsIMPORT_FLAGbut the currentexecArgvmisses it (i.e. the branch that prepends@swc-node/register/esm-register).- CJS Branch (Line ~258): Although tests exist for the helper function
hasRequireFlag, there are no integration tests confirming the behavior whentsUseEsmis false and!hasRequireFlag(execArgv)is true (which triggers prependingREQUIRE_ABBR_FLAGand@swc-node/register).Please add tests targeting these specific paths to ensure comprehensive coverage of the SWC runner behavior.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 244-244: src/helpers.ts#L244
Added line #L244 was not covered by tests
[warning] 258-258: src/helpers.ts#L258
Added line #L258 was not covered by tests
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/helpers.ts (2)
203-213:esbuild-registerlogic
Line 212 is not covered by tests, per the static analysis. To ensure full coverage, consider adding a scenario that triggers therequireregistration path.Would you like help generating a dedicated test for line 212?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 212-212: src/helpers.ts#L212
Added line #L212 was not covered by tests
238-264:swc-noderunner
Properly checks for ESM vs. require usage.
Line 257 remains uncovered by tests. Consider adding coverage for that branch.Shall I provide a test snippet to exercise the SWC require path?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 257-257: src/helpers.ts#L257
Added line #L257 was not covered by tests
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/constants.ts(5 hunks)src/helpers.ts(8 hunks)src/index.ts(2 hunks)test/helpers.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/index.ts
- src/constants.ts
- test/helpers.spec.ts
🧰 Additional context used
🪛 GitHub Check: codecov/patch
src/helpers.ts
[warning] 212-212: src/helpers.ts#L212
Added line #L212 was not covered by tests
[warning] 257-257: src/helpers.ts#L257
Added line #L257 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (16)
- GitHub Check: Lint and Test with Node.js 18 on macos-latest
- GitHub Check: Lint and Test with Node.js 23 on macos-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 23 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 18.18 on windows-latest
- GitHub Check: Lint and Test with Node.js 18.18 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 23 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on macos-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 18.18 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (26)
src/helpers.ts (26)
13-13: UsingcompareNodeVersionimport
The import ofcompareNodeVersionaligns with the updated version-check logic.
21-22: New constants for import flag
DefiningIMPORT_FLAGandIMPORT_FLAG_SUPPORTEDhere is clear and consistent with the rest of the constants file.
24-27: Loader-flag constants and MTS support
AddingLOADER_FLAG,LOADER_FLAGS,MODULE_REGISTER_SUPPORTED, andMTS_SUPPORTEDhelps centralize all Node flag-related checks.
31-32:REQUIRE_ABBR_FLAGandREQUIRE_FLAGS
These constants maintain consistent naming patterns with the other flags (e.g.,LOADER_FLAGS).
37-37:TS_ESM_PARTIAL_SUPPORTEDconstant
Good addition for partial ESM detection. This constant is used later to gate certain runners.
63-65:hasRequireFlagfunction
Usessome()against theREQUIRE_FLAGSset. Straightforward and efficient.
66-68:hasImportFlagfunction
Checks forIMPORT_FLAGinexecArgv. No issues noted.
69-71:hasLoaderFlagfunction
Similar pattern tohasRequireFlag, reusing theLOADER_FLAGSset. Implementation looks good.
141-141: Node version check insetupTsRunner
Ensures only supported Node versions can perform type stripping.
155-155: Guarding older Node versions
Throws an error for Node versions not meetingSTRIP_TYPES_NODE_VERSION. Logical approach.
166-166: CheckingDEFAULT_TYPES_NODE_VERSION
Breaks early if Node supports default type stripping. No issues here.
172-172: Version check for transform-types
Only adds the transform flag for Node versions ≥TRANSFORM_TYPES_NODE_VERSION.
178-178: Fallback for strip-types
Adds--strip-typesif Node version satisfies the requirement. Straightforward logic.
186-186: Comment link update
No functional impact, just a helpful reference.
189-191: Loader flag injection forTsRunner.TsNode
Ensuresesmregistration is used whenexecArgvlacks the loader flag.
192-197: Require flag forTsRunner.TsNode
Appends the-rts-node/register usage if missing. Implementation seems correct.
218-225:esbuild-runnerrequire injection
Similar pattern, adding-r esbuild-runner/registerif absent. No issues found.
226-227: OXC usage comment
Just a reference link for usage details. No functional changes.
228-235: OXC runner
Adds the--importflag if not present. Implementation is consistent.
265-275:TsRunner.TSX
Cleanly checksIMPORT_FLAG_SUPPORTEDand inserts either--importor--loaderfor TSX.
300-300: CheckingREQUIRE_FLAGSfor pnp
Makes sense to ensurepnpapiis required.
305-305: Re-injectingpnpApiPath
Adds-r pnpapito the args. Logical approach.
311-314: Fallback loader for pnp
Uses--loaderwith the.pnp.loader.mjsifMODULE_REGISTER_SUPPORTEDis false. Looks correct.
524-524: Conditionally enabling partial ESM
AssignsisTsxSupportedbased ontsUseEsmorTS_ESM_PARTIAL_SUPPORTED.
530-530: Unsupported runners for.mts
Excludes certain TypeScript runners if.mtsextension is found. This is a safeguard to prevent known issues.
550-552: Enhanced error message
Informs users of alternatives like OXC or SWC for.mtsif not fully supported. Great clarity.
Docstrings generation was requested by @JounQin. * #222 (comment) The following files were modified: * `src/helpers.ts` * `src/index.ts`
|
Note Generated docstrings for this pull request at #223 |
Docstrings generation was requested by @JounQin. * #222 (comment) The following files were modified: * `src/helpers.ts` * `src/index.ts`
Docstrings generation was requested by @JounQin. * #222 (comment) The following files were modified: * `src/helpers.ts` * `src/index.ts`
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/constants.ts (2)
43-47: Redundant condition inMODULE_REGISTER_SUPPORTED.
BecauseMTS_SUPPORTED(>=20.8) implies Node is also >=18.19, the explicit|| compareNodeVersion('18.19') >= 0can be simplified.-export const MODULE_REGISTER_SUPPORTED = - MTS_SUPPORTED || - compareNodeVersion('18.19') >= 0 +export const MODULE_REGISTER_SUPPORTED = compareNodeVersion('18.19') >= 0
67-71: Consider refactoringNO_STRIP_TYPESlogic for clarity.
The nested checks for Node version vs.process.features.typescriptand flags are slightly confusing. Splitting this into helper functions or adding explanatory comments could improve readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/constants.ts(5 hunks)test/helpers.spec.ts(1 hunks)
🧰 Additional context used
🧬 Code Definitions (2)
test/helpers.spec.ts (3)
src/helpers.ts (8)
hasRequireFlag(63-64)hasImportFlag(66-67)hasLoaderFlag(69-70)dataUrl(60-61)md5Hash(334-336)extractProperties(465-473)setupTsRunner(72-332)overrideStdio(676-695)src/constants.ts (9)
REQUIRE_FLAG(97-97)REQUIRE_ABBR_FLAG(99-99)IMPORT_FLAG(95-95)LOADER_FLAG(103-103)TsRunner(4-21)TsRunner(23-23)TRANSFORM_TYPES_NODE_VERSION(53-53)DEFAULT_TYPES_NODE_VERSION(59-59)TRANSFORM_TYPES_FLAG(62-62)test/helpers.ts (6)
workerCjsPath(17-17)workerCjsTsPath(11-11)workerTsPath(16-16)workerMjsAsMtsPath(19-19)workerMtsPath(20-20)testIf(23-23)
src/constants.ts (1)
src/common.ts (2)
compareNodeVersion(29-30)hasFlag(3-4)
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: Lint and Test with Node.js 18.18 on windows-latest
- GitHub Check: Lint and Test with Node.js 23 on windows-latest
- GitHub Check: Lint and Test with Node.js 23 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 on macos-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 23 on windows-latest
- GitHub Check: Lint and Test with Node.js 18.18 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
🔇 Additional comments (15)
test/helpers.spec.ts (10)
1-33: Well-structured imports and organization.The file follows best practices with clear import organization, disabling ESLint rule for the unbound method when needed, and importing all necessary components. The comprehensive test suite covers all major helper functions that are part of the PR changes.
34-86: Thorough flag detection utility tests.The tests for
hasRequireFlag,hasImportFlag, andhasLoaderFlagare comprehensive, covering all scenarios:
- Presence of flags in different positions
- Flag abbreviations (for require)
- Cases where flags are absent
- Empty array handling
This provides good coverage for these utility functions which are critical for the TypeScript runner detection logic.
88-110: Good test coverage for the dataUrl function.Tests cover essential scenarios including:
- Basic JavaScript code encoding
- Empty string handling
- Special character encoding
The verification of URL structure and content is thorough.
112-118: Adequate md5Hash function tests.The tests verify correct hash generation for different inputs including edge cases like empty strings.
120-156: Comprehensive extractProperties function tests.The tests effectively verify:
- Shallow copying behavior
- Nested object handling
- Undefined input edge case
- Custom Error object properties
The test on line 152 correctly verifies that non-enumerable properties like
messagearen't captured, which is an important detail for Error objects.
158-208: Thorough setupTsRunner basic functionality tests.These tests comprehensively verify:
- Correct identification of JavaScript vs TypeScript files
- Proper handling of ESM TS files
- .mts file detection and handling
- Correct flag addition based on file type
Good test organization with proper test cleanup using
afterEach.
209-224: Well-implemented conditional Node.js version test.The test uses
testIfto conditionally run based on Node.js version compatibility, which is a good approach for version-specific features. The test verifies the correct addition of the transform types flag based on Node.js version comparison, demonstrating version-aware behavior.The explicit eslint-disable comment is helpful to prevent linting errors with standalone expects in conditional tests.
226-233: Correctly implements OXC runner test.This test is crucial for the PR's main objective of adding OXC TypeScript runner support. It verifies that:
- The
--importflag is correctly added to the execution arguments- The
@oxc-node/core/registermodule is properly includedThis aligns with the PR objective and ensures the new functionality works as expected.
235-243: Good error handling test for unknown runners.The test properly verifies that an error is thrown when an unknown TypeScript runner is specified. The explicit
@ts-expect-errorcomment is good practice to acknowledge the intentional type error in the test.
245-295: Thorough tests for overrideStdio with proper cleanup.The tests for the
overrideStdiofunction:
- Save and restore original
_writevmethods correctly- Verify both stdout and stderr overrides
- Check that output is properly captured in the expected format
- Ensure callbacks are called
Good practice to save the original methods and restore them after each test to prevent test interference.
src/constants.ts (5)
16-16: NewOXCrunner added.
Looks good and provides clarity for users who prefer the OXC Node runner.
33-39: Validate partial ESM support range.
DefiningTS_ESM_PARTIAL_SUPPORTEDfor Node >=16 and <18.19 is presumably correct per referenced issue. Ensure tests cover scenarios in these Node versions for reliable detection.
40-41:MTS_SUPPORTEDlogic is straightforward.
No issues spotted with checking Node >= 20.8 for MTS support.
95-107: Flag constants introduced for import/require/loader configurations.
These additions cleanly centralize flag definitions for various Node-related operations.
109-110:IMPORT_FLAG_SUPPORTEDversion check.
Checking Node >= 20.6 for import-flag support seems consistent with known releases.
@coderabbitai This is incorrect, the |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@JounQin You're absolutely right, and I apologize for my mistake. Looking at the code closely: export const TS_ESM_PARTIAL_SUPPORTED =
compareNodeVersion('16') >= 0 && compareNodeVersion('18.19') < 0
export const MTS_SUPPORTED = compareNodeVersion('20.8') >= 0
export const MODULE_REGISTER_SUPPORTED =
!TS_ESM_PARTIAL_SUPPORTED || MTS_SUPPORTEDThe logic here is not redundant at all. It creates specific ranges of Node.js versions where
My suggestion was incorrect. Thank you for pointing this out - I appreciate the correction. 🏁 Script executed: Length of output: 3429 🏁 Script executed: Length of output: 235 🏁 Script executed: Length of output: 63580 @JounQin You're absolutely right, and I apologize for my mistake. Looking at the code closely: export const TS_ESM_PARTIAL_SUPPORTED =
compareNodeVersion('16') >= 0 && compareNodeVersion('18.19') < 0
export const MTS_SUPPORTED = compareNodeVersion('20.8') >= 0
export const MODULE_REGISTER_SUPPORTED =
!TS_ESM_PARTIAL_SUPPORTED || MTS_SUPPORTEDThe logic here is not redundant at all. It creates specific ranges of Node.js versions where
My suggestion was incorrect. Thank you for pointing this out - I appreciate the correction.✏️ Learnings added
|
oxc ts runner supporoxc ts runner support
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/helpers.spec.ts (1)
209-224: Consider improving the conditional test for TRANSFORM_TYPES_FLAG.The commented line "can not be mocked correctly for now" suggests a limitation in testing. Additionally, the assertion against an empty string when
compareNodeVersion(DEFAULT_TYPES_NODE_VERSION) >= 0seems unusual.Consider refactoring to more clearly express the intent:
- // eslint-disable-next-line jest/no-standalone-expect - expect(execArgv).toContain( - compareNodeVersion(DEFAULT_TYPES_NODE_VERSION) >= 0 - ? '' - : TRANSFORM_TYPES_FLAG, - ) + // eslint-disable-next-line jest/no-standalone-expect + if (compareNodeVersion(DEFAULT_TYPES_NODE_VERSION) >= 0) { + expect(execArgv).not.toContain(TRANSFORM_TYPES_FLAG) + } else { + expect(execArgv).toContain(TRANSFORM_TYPES_FLAG) + }This makes the test intention clearer and avoids expecting an empty string, which might not be what was intended.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/constants.ts(5 hunks)test/helpers.spec.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
src/constants.ts (1)
Learnt from: JounQin
PR: un-ts/synckit#0
File: :0-0
Timestamp: 2025-04-02T08:02:26.784Z
Learning: In src/constants.ts of the synckit project, the MODULE_REGISTER_SUPPORTED constant uses a logical OR condition (!TS_ESM_PARTIAL_SUPPORTED || MTS_SUPPORTED) to create specific version ranges where it evaluates differently based on Node.js versions. This is intentional logic and not redundant, as it creates a specific gap for Node.js versions 16-18.18 where MODULE_REGISTER_SUPPORTED is false.
🧬 Code Definitions (2)
test/helpers.spec.ts (3)
src/helpers.ts (8)
hasRequireFlag(63-64)hasImportFlag(66-67)hasLoaderFlag(69-70)dataUrl(60-61)md5Hash(334-336)extractProperties(465-473)setupTsRunner(72-332)overrideStdio(676-695)src/constants.ts (9)
REQUIRE_FLAG(98-98)REQUIRE_ABBR_FLAG(100-100)IMPORT_FLAG(96-96)LOADER_FLAG(104-104)TsRunner(4-21)TsRunner(23-23)TRANSFORM_TYPES_NODE_VERSION(53-53)DEFAULT_TYPES_NODE_VERSION(59-59)TRANSFORM_TYPES_FLAG(62-62)test/helpers.ts (6)
workerCjsPath(17-17)workerCjsTsPath(11-11)workerTsPath(16-16)workerMjsAsMtsPath(19-19)workerMtsPath(20-20)testIf(23-23)
src/constants.ts (1)
src/common.ts (2)
compareNodeVersion(29-30)hasFlag(3-4)
⏰ Context from checks skipped due to timeout of 90000ms (15)
- GitHub Check: Lint and Test with Node.js 23 on ubuntu-latest
- GitHub Check: Lint and Test with Node.js 23 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 23 on macos-latest
- GitHub Check: Lint and Test with Node.js 22 on macos-latest
- GitHub Check: Lint and Test with Node.js 18.18 on macos-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 18.18 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on macos-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 23 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 18.18 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
🔇 Additional comments (16)
src/constants.ts (7)
1-1: Great refactoring to use specific helper functions from common.js.Using
compareNodeVersionandhasFlagfrom common.js improves code maintainability by centralizing version comparison logic and flag checking.
15-16: Successfully adds OXC TypeScript runner support.This addition aligns with the PR objective to add support for the
oxcTypeScript runner, with proper documentation via the GitHub repository link.
33-39: Clearly defined Node.js version range check.The
TS_ESM_PARTIAL_SUPPORTEDconstant is well-implemented with clear comments for the version range (Node.js 16-18.18). The reference to the tsx issue provides valuable context for this implementation.
40-41: Well-documented MTS support check.The
MTS_SUPPORTEDconstant correctly identifies Node.js 20.8+ with a helpful comment about themodule.registerrequirement.
43-48: Correctly implemented MODULE_REGISTER_SUPPORTED logic.The implementation creates specific version ranges where MODULE_REGISTER_SUPPORTED evaluates differently:
- For Node 16-18.18: It evaluates to false
- For Node 18.19-20.7: It evaluates to true
- For Node >= 20.8: It evaluates to true
This aligns with the PR objectives and is intentionally designed this way.
67-72: Improved implementation of NO_STRIP_TYPES constant.The refactored implementation now correctly:
- Checks for the
--no-experimental-strip-typesflag using thehasFlagutility- Uses
compareNodeVersionfor cleaner version comparison- Considers
process.features.typescriptwhen appropriate based on Node.js versionThis makes the code more maintainable and easier to understand.
96-111: Well-organized flag constants with clear documentation.The added constants for import/require flags and loader flags are well-structured and clearly named. The usage of Sets for grouping related flags is a good practice. The
IMPORT_FLAG_SUPPORTEDconstant properly checks for Node.js 20.6+ compatibility.test/helpers.spec.ts (9)
1-32: Well-structured test file with comprehensive imports.The file is properly set up with necessary imports from Jest, local helpers, and the synckit package. The ESLint disable comment for
@typescript-eslint/unbound-methodis appropriate as the tests will be working with unbound methods.
34-86: Good test coverage for flag detection utilities.The tests for
hasRequireFlag,hasImportFlag, andhasLoaderFlagare thorough and cover various scenarios including:
- Flag present at beginning of array
- Flag in middle of array
- Flag not present
- Empty array handling
This ensures the functions work correctly in all expected use cases.
88-110: Comprehensive tests for dataUrl function.The tests properly validate the dataUrl function's ability to:
- Encode JavaScript code into a data URL
- Handle empty code
- Properly encode special characters
Good job checking both the URL structure and decoded content.
112-118: Sufficient test coverage for md5Hash function.The tests verify correct hashing behavior with multiple inputs, including an empty string. The test cases should detect any regressions in the hashing functionality.
120-156: Thorough testing of extractProperties with edge cases.Great work testing different scenarios:
- Shallow copying behavior
- Nested object handling
- Undefined input
- Error objects with custom properties
The explicit test for non-enumerable properties on Error objects shows good attention to detail.
158-208: Good coverage of setupTsRunner for different file types.The tests properly verify the function's behavior with:
- JS files
- TS files with different runners
- ESM TS files
- .mts files
Each test validates appropriate extension detection, ESM settings, and runner-specific configuration.
226-233: Good implementation of OXC runner test.This test properly validates the OXC runner configuration, which aligns with the PR objective to add OXC TypeScript runner support. The test confirms that the correct import flag and module are included in the execArgv.
235-242: Good error handling test for unknown runners.The test properly verifies that an appropriate error is thrown when an unrecognized runner is specified. Using
@ts-expect-errorcorrectly acknowledges the intentional type error.
245-295: Well-structured tests for overrideStdio function.The tests correctly:
- Save and restore original stdout/stderr methods
- Verify that the methods are properly overridden
- Test the functionality by directly calling the new methods
- Check that all expected data is captured in the stdio array
Good practice to restore methods after each test to prevent test pollution.
…tal-strip-types` flag enabled
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (9)
package.json(1 hunks)src/common.ts(1 hunks)src/constants.ts(5 hunks)src/helpers.ts(9 hunks)src/index.ts(3 hunks)test/fn.spec.ts(3 hunks)test/helpers.spec.ts(1 hunks)test/helpers.ts(1 hunks)test/ts-runner.spec.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (4)
test/ts-runner.spec.ts (1)
Learnt from: JounQin
PR: un-ts/synckit#222
File: src/helpers.ts:13-13
Timestamp: 2025-04-02T06:32:20.004Z
Learning: Tests in the synckit project run in an ESM-only environment, so CommonJS patterns like require() should not be used in test files. Always use ESM import syntax and proper mocking approaches compatible with ESM.
test/helpers.ts (1)
Learnt from: JounQin
PR: un-ts/synckit#222
File: src/helpers.ts:13-13
Timestamp: 2025-04-02T06:32:20.004Z
Learning: Tests in the synckit project run in an ESM-only environment, so CommonJS patterns like require() should not be used in test files. Always use ESM import syntax and proper mocking approaches compatible with ESM.
src/helpers.ts (3)
Learnt from: JounQin
PR: un-ts/synckit#222
File: src/helpers.ts:13-13
Timestamp: 2025-04-02T06:32:20.004Z
Learning: Tests in the synckit project run in an ESM-only environment, so CommonJS patterns like require() should not be used in test files. Always use ESM import syntax and proper mocking approaches compatible with ESM.
Learnt from: JounQin
PR: un-ts/synckit#222
File: src/helpers.ts:13-13
Timestamp: 2025-04-02T06:41:24.897Z
Learning: When testing ESM modules with Jest, don't use jest.spyOn() directly on named imports as it won't work. Instead, use jest.mock() to mock the entire module with a factory function that returns mock implementations of the exported functions. For TypeScript, you'll need to cast the imported functions to jest.Mock when using them in tests.
Learnt from: JounQin
PR: un-ts/synckit#222
File: src/helpers.ts:13-13
Timestamp: 2025-04-02T06:45:12.656Z
Learning: In Node.js, `process.stdout` and `process.stderr` are readonly properties that cannot be directly reassigned in tests. Instead of replacing them with mock objects, either save and restore their internal methods (like `_writev`), use `jest.spyOn()` on specific methods, or use dedicated stdout/stderr mocking libraries for testing functions that interact with standard I/O streams.
src/constants.ts (1)
Learnt from: JounQin
PR: un-ts/synckit#0
File: :0-0
Timestamp: 2025-04-02T08:02:26.784Z
Learning: In src/constants.ts of the synckit project, the MODULE_REGISTER_SUPPORTED constant uses a logical OR condition (!TS_ESM_PARTIAL_SUPPORTED || MTS_SUPPORTED) to create specific version ranges where it evaluates differently based on Node.js versions. This is intentional logic and not redundant, as it creates a specific gap for Node.js versions 16-18.18 where MODULE_REGISTER_SUPPORTED is false.
🧬 Code Definitions (5)
src/index.ts (1)
src/constants.ts (1)
MODULE_REGISTER_SUPPORTED(44-47)
test/helpers.spec.ts (3)
src/helpers.ts (8)
hasRequireFlag(63-64)hasImportFlag(66-67)hasLoaderFlag(69-70)dataUrl(60-61)md5Hash(334-336)extractProperties(465-473)setupTsRunner(72-332)overrideStdio(676-695)src/constants.ts (9)
REQUIRE_FLAG(98-98)REQUIRE_ABBR_FLAG(100-100)IMPORT_FLAG(96-96)LOADER_FLAG(104-104)TsRunner(4-21)TsRunner(23-23)TRANSFORM_TYPES_NODE_VERSION(53-53)DEFAULT_TYPES_NODE_VERSION(59-59)TRANSFORM_TYPES_FLAG(62-62)test/helpers.ts (6)
workerCjsPath(17-17)workerCjsTsPath(11-11)workerTsPath(16-16)workerMjsAsMtsPath(19-19)workerMtsPath(20-20)testIf(23-23)
test/fn.spec.ts (2)
test/helpers.ts (1)
testIf(23-23)src/constants.ts (1)
TS_ESM_PARTIAL_SUPPORTED(34-38)
src/helpers.ts (3)
src/constants.ts (12)
REQUIRE_FLAGS(102-102)IMPORT_FLAG(96-96)LOADER_FLAGS(108-108)DEFAULT_EXEC_ARGV(76-76)TsRunner(4-21)TsRunner(23-23)LOADER_FLAG(104-104)REQUIRE_ABBR_FLAG(100-100)IMPORT_FLAG_SUPPORTED(111-111)MODULE_REGISTER_SUPPORTED(44-47)TS_ESM_PARTIAL_SUPPORTED(34-38)MTS_SUPPORTED(41-41)src/common.ts (1)
compareNodeVersion(29-30)src/types.ts (1)
AnyFn(6-6)
src/constants.ts (1)
src/common.ts (2)
compareNodeVersion(29-30)hasFlag(3-4)
🪛 GitHub Check: codecov/patch
src/helpers.ts
[warning] 215-215: src/helpers.ts#L215
Added line #L215 was not covered by tests
[warning] 260-260: src/helpers.ts#L260
Added line #L260 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Lint and Test with Node.js 23 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 18.18 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 23 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 18.18 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
🔇 Additional comments (53)
src/common.ts (1)
26-30: Good addition of centralized version comparison utilities.These new utilities provide a consistent way to handle Node.js version comparisons throughout the codebase. The
NODE_VERSIONconstant eliminates repeated calls toprocess.versions.node, andcompareNodeVersionsimplifies version comparisons by automatically comparing against the current Node.js version.package.json (1)
76-80: Good addition of dependencies for OXC support.Adding the necessary dependencies for the
oxcTypeScript runner and SWC-related packages aligns well with the PR objective of adding support for theoxcTypeScript runner. The version constraints are specific enough to ensure compatibility while allowing for minor updates.test/fn.spec.ts (3)
20-24: Improved import organization.Restructuring imports to bring in
TS_ESM_PARTIAL_SUPPORTEDdirectly from the 'synckit' module centralizes the ESM support detection logic, making the codebase more maintainable.
48-48: Standardized ESM support detection.Replacing local variable
tsUseEsmSupportedwith the importedTS_ESM_PARTIAL_SUPPORTEDcreates consistency in how ESM support is detected across tests.
62-62: Standardized ESM support detection.Similarly here, using
TS_ESM_PARTIAL_SUPPORTEDfor conditional test execution ensures consistent behavior across the test suite.src/index.ts (3)
10-10: Simplified import by using the centralized constant.Importing
MODULE_REGISTER_SUPPORTEDinstead of multiple version-related constants simplifies the code and follows the principle of encapsulation, hiding the implementation details of how module registration support is determined.
81-89: Improved documentation clarity.The updated documentation for
runAsWorkernow more clearly explains the function's behavior, particularly regarding the conditions under which the custom module loader is registered.
107-107: Simplified condition for module registration.Using the
MODULE_REGISTER_SUPPORTEDconstant instead of direct version comparison improves readability and maintainability. This approach centralizes the version-checking logic, making it easier to update or modify in the future.test/helpers.spec.ts (10)
1-33: Well-structured test file setup with comprehensive imports.The test file setup is clean and well-organized, importing all necessary utilities and constants needed for testing. The ESLint directive at the top is appropriate for the unbound method usage.
34-86: Good test coverage for flag detection utilities.The flag detection tests are thorough and cover important edge cases:
- Testing both full flags and abbreviations
- Checking flags in different positions within the array
- Handling empty arrays
- Testing negative cases
This comprehensive approach ensures the utility functions work correctly across different scenarios.
88-110: Thorough testing of the dataUrl function.The tests for the
dataUrlfunction cover all important scenarios including proper encoding, handling of empty code, and special character encoding. The assertions check both the URL construction and the decoded content.
112-118: Correct MD5 hash validation.These tests verify that the
md5Hashfunction generates the expected hash values for different inputs, including the edge case of an empty string.
120-156: Comprehensive property extraction tests.Good coverage of the
extractPropertiesfunction, testing shallow copying, nested objects, undefined input, and Error objects with custom properties. The test on line 152 correctly verifies that non-enumerable properties likemessagearen't copied.
158-184: Proper testing of TypeScript file identification.The tests correctly verify how the
setupTsRunnerfunction identifies JavaScript and TypeScript files, including the correct extension detection and runner configuration.
186-207: Good validation of ESM TS files handling.The tests properly verify the handling of ESM TS files and
.mtsfiles with the ts-node runner, ensuring the correct loader flags are added to the execution arguments.
226-233: Proper implementation of OXC runner support.This test correctly verifies that the OXC runner is properly supported by checking that the execution arguments include both the
IMPORT_FLAGand the '@oxc-node/core/register' module path. This aligns with the PR objective of addingoxcts runner support.
235-243: Good error handling test for unknown runners.The test correctly verifies that an error is thrown when an unknown runner is specified. The error message is clear and informative.
245-295: Thorough testing of stdio overriding.These tests properly verify that the
overrideStdiofunction correctly overrides the standard output and error methods, capturing the output in the expected format. The tests include proper setup and teardown to restore the original methods.test/helpers.ts (2)
7-7: Simplified import to use only the needed type.Good practice to import only the specific type needed from the module instead of the entire module.
11-21: Well-organized path constants for various worker file types.These constants provide clear, descriptive paths for different file types and extensions used in testing. Having these centralized makes the tests more maintainable and reduces duplication.
test/ts-runner.spec.ts (8)
1-1: Updated ESLint directive for conditional expectations.Adding
jest/no-conditional-expectto the disabled rules is appropriate since the tests use conditional expectations based on version support.
7-7: Import extension changed from .ts to .js.The change from importing './helpers.ts' to './helpers.js' aligns with ESM best practices, as mentioned in the retrieved learning about running tests in an ESM-only environment.
14-16: Updated version comparison approach.Replacing
compareVersionwithcompareNodeVersionand adding imports forTS_ESM_PARTIAL_SUPPORTEDandMTS_SUPPORTEDconstants streamlines version checking across the codebase.
44-44: Standardized ESM support checks.Replacing previous checks with the
TS_ESM_PARTIAL_SUPPORTEDconstant makes the code more consistent and maintainable by centralizing the ESM support logic.Also applies to: 70-70, 129-129, 155-155
79-110: Comprehensive tests for the new OXC runner.This new test block thoroughly validates the OXC TypeScript runner functionality:
- Tests with JavaScript files
- Tests with MJS files
- Proper error handling for .mts files when not supported
- Conditional testing for .mts files when supported
The test structure mirrors other runner tests for consistency and covers all necessary cases.
135-142: Added MTS support handling for SWC runner.Adding the conditional block for
MTS_SUPPORTEDprovides better test coverage for SWC runner with .mts files when they are supported, ensuring consistent behavior across different Node.js versions.
155-162: Improved conditional test for TSX runner.The updated conditional logic for TSX runner now positively tests .mts file support when
TS_ESM_PARTIAL_SUPPORTEDis true, providing better coverage of the feature when available.
175-175: Simplified version comparison.Using
compareNodeVersion(STRIP_TYPES_NODE_VERSION)instead of the previous comparison approach makes the code more consistent with other version checks in the codebase.src/helpers.ts (18)
13-13: Importing version checks & constants
These lines introduce references tocompareNodeVersionfrom./common.jsand several constants (e.g.,IMPORT_FLAG,LOADER_FLAG,REQUIRE_FLAGS, etc.). They are consistently used throughout the file and align well with the updated logic for Node.js version and flag checks.Also applies to: 21-22, 24-27, 31-32, 37-37
63-70: Helper functions for checking flags
Great job extractinghasRequireFlag,hasImportFlag, andhasLoaderFlaginto dedicated helper functions. This improves clarity and keeps the logic DRY. Ensure they're covered in tests to avoid untested paths.
72-72: Refined function signature forsetupTsRunner
The updated signature cleanly injectsexecArgvandtsRunnerwith default values. This is straightforward and enhances readability.Also applies to: 74-77
144-144: Node.js version checks withcompareNodeVersion
All these lines consistently compare versions against constants (STRIP_TYPES_NODE_VERSION,DEFAULT_TYPES_NODE_VERSION,TRANSFORM_TYPES_NODE_VERSION) to conditionally set or throw errors. The logic is aligned with the new approach.Also applies to: 158-158, 169-169, 175-175, 182-182
190-190: Comment-only change
No functional modification here—just a reference link.
192-193: Conditional logic forTsRunner.TsNode
This block correctly checks for ESM usage vs. require usage. The approach to insert--loaderor-r ts-node/registeris consistent with TypeScript runner best practices.Also applies to: 195-200
206-214:TsRunner.EsbuildRegisterlogic & uncovered line
Your conditionals for ESM vs. require are robust and consistent with esbuild-register usage.
However, line 215 (execArgv = [REQUIRE_ABBR_FLAG, TsRunner.EsbuildRegister, ...execArgv]) is flagged by static analysis as not covered by tests. Consider adding a test scenario covering the non-ESM path wherehasRequireFlag(execArgv) === false.Also applies to: 215-215
221-226:TsRunner.EsbuildRunner
The logic to prepend-r esbuild-runner/registerfor non-ESM files is correct and mirrors the esbuild-runner docs.
231-240:TsRunner.OXC
Good addition for OXC runner support. The conditional insertion of--import @oxc-node/...matches the Node 20+ approach.
243-265:TsRunner.SWC& partial coverage
The logic properly distinguishes between ESM flow (--importor--loader) and require flow (-r). It’s well-structured for Node version differences.
Line 260 is flagged as not covered by tests. Please add a test scenario where SWC is used with non-ESM files andhasRequireFlag(execArgv) === falseto ensure coverage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 260-260: src/helpers.ts#L260
Added line #L260 was not covered by tests
270-270:TsRunner.TSX
The checks forIMPORT_FLAG_SUPPORTEDand fallback to loader are straightforward and match TSX usage patterns.Also applies to: 272-273, 275-275
283-283: Fallback condition for JS usage
No issues found in theelse if (!jsUseEsm && !workerPath.endsWith('.cjs'))block; it ensures correct fallback logic.
303-304:REQUIRE_FLAGSusage in pnp checks
This snippet accurately checks the next argument after matching a require flag. Looks fine.
307-308: Ensure pnpApiPath is in execArgv
The condition to includepnpApiPathif not present is valid for Yarn PnP.
314-316: Applying pnp loader if module register not supported
This block correctly checksMODULE_REGISTER_SUPPORTEDand sets up the loader path. Logic is consistent with earlier definitions.
330-330: Trailing addition to the returned object
IncludingexecArgvin the returned object is indeed essential for downstream usage.
478-499: Enhanced doc comments
The updated explanation clarifies the worker thread setup, TypeScript runner usage, and synchronization method. Great documentation improvement.
500-500: ExportingstartWorkerThread
The function signature expansion with the generic types remains consistent, and the ESLint complexity override is understandable for this extensive logic.src/constants.ts (7)
1-1: New imports fromcommon.js
Bringing incompareNodeVersionandhasFlagis a sensible step to unify version checks and flag detection.
15-16: AddingOXCrunner
Identifying'oxc'in theTsRunnerobject is a clean addition, matching the new OXC project support.
33-38:TS_ESM_PARTIAL_SUPPORTED
Accurately captures Node 16 to <18.19 partial ESM support range. Reflects the user’s documented logic.
40-47:MTS_SUPPORTED&MODULE_REGISTER_SUPPORTED
These constants are intentionally structured to split support for Node >=20.8 and >=18.19. The logic is not redundant—nicely done.
67-72:NO_STRIP_TYPESlogic
This condition depends on both a flag (--no-experimental-strip-types) andprocess.features.typescript. Please ensure thatprocess.featuresis defined in all relevant Node versions used in your environment. If not, guard or polyfill as needed.
96-96: New constants for import/require/loader flags
DefiningIMPORT_FLAG,REQUIRE_FLAG,REQUIRE_ABBR_FLAG,REQUIRE_FLAGS,LOADER_FLAG, andEXPERIMENTAL_LOADER_FLAGas well asLOADER_FLAGSstreamlines checks insrc/helpers.ts.Also applies to: 98-98, 100-100, 102-102, 104-104, 106-106, 108-108
110-111:IMPORT_FLAG_SUPPORTED
Checking against Node >=20.6 is consistent with new Node docs for the--importflag. Straightforward addition.
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/common.ts (1)
27-30: Consider adding JSDoc comments to the new exports.The implementation of
NODE_VERSIONandcompareNodeVersionlooks good and will help standardize version comparisons across the codebase. Adding JSDoc comments would improve code documentation and make the purpose of these exports clearer to other developers.+/** + * Current Node.js version from process.versions.node + */ export const NODE_VERSION = process.versions.node +/** + * Compares the current Node.js version with the provided version + * @param version - The version string to compare against the current Node.js version + * @returns -1 if current version is less than provided, 0 if equal, 1 if greater + */ export const compareNodeVersion = (version: string) => compareVersion(NODE_VERSION, version)src/helpers.ts (2)
72-77: Function length caution.
setupTsRunneris quite extensive. Consider modularizing or splitting some logic (like extension detection, argument assembly, etc.) into helper functions for maintainability.
532-554: Complex condition block for MTS + partial ESM.
While functional, this chunk has multiple arrays merged via conditionals (EsbuildRegister,EsbuildRunner, additional partial ESM runners, etc.). Consider factoring out the runner array into smaller logic blocks to reduce complexity. Also note line 538’s coverage gap.Would you like a snippet demonstrating a more streamlined approach to optional runner arrays?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 538-538: src/helpers.ts#L538
Added line #L538 was not covered by teststest/ts-runner.spec.ts (1)
79-110: Consider refactoring to reduce test duplication.The test cases for different TypeScript runners have significant code duplication. Consider using a test factory function to reduce duplication while still maintaining test clarity.
function testTsRunner(runner: TsRunner, mtsSupportChecker: () => boolean) { test(runner, async () => { const { createSyncFn } = await import('synckit') // Test with .js files let syncFn = createSyncFn<AsyncWorkerFn>(workerJsPath, { tsRunner: runner }) expect(syncFn(1)).toBe(1) expect(syncFn(2)).toBe(2) expect(syncFn(5)).toBe(5) // Test with .mjs files syncFn = createSyncFn<AsyncWorkerFn>(workerMjsPath, { tsRunner: runner }) expect(syncFn(1)).toBe(1) expect(syncFn(2)).toBe(2) expect(syncFn(5)).toBe(5) // Test with .mts files based on support if (!mtsSupportChecker()) { expect(() => createSyncFn<AsyncWorkerFn>(workerMtsPath, { tsRunner: runner })) .toThrow(`${runner} is not supported for .mts files yet`) } else { syncFn = createSyncFn<AsyncWorkerFn>(workerMtsPath, { tsRunner: runner }) expect(syncFn(1)).toBe(1) expect(syncFn(2)).toBe(2) expect(syncFn(5)).toBe(5) } }) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lockis excluded by!**/yarn.lock,!**/*.lock
📒 Files selected for processing (12)
.changeset/honest-onions-add.md(1 hunks).changeset/rare-carrots-brush.md(1 hunks).changeset/tender-peaches-smell.md(1 hunks)package.json(1 hunks)src/common.ts(1 hunks)src/constants.ts(5 hunks)src/helpers.ts(9 hunks)src/index.ts(3 hunks)test/fn.spec.ts(3 hunks)test/helpers.spec.ts(1 hunks)test/helpers.ts(1 hunks)test/ts-runner.spec.ts(6 hunks)
🧰 Additional context used
🧠 Learnings (4)
test/helpers.ts (1)
Learnt from: JounQin
PR: un-ts/synckit#222
File: src/helpers.ts:13-13
Timestamp: 2025-04-02T06:32:20.004Z
Learning: Tests in the synckit project run in an ESM-only environment, so CommonJS patterns like require() should not be used in test files. Always use ESM import syntax and proper mocking approaches compatible with ESM.
src/constants.ts (1)
Learnt from: JounQin
PR: un-ts/synckit#0
File: :0-0
Timestamp: 2025-04-02T08:02:26.784Z
Learning: In src/constants.ts of the synckit project, the MODULE_REGISTER_SUPPORTED constant uses a logical OR condition (!TS_ESM_PARTIAL_SUPPORTED || MTS_SUPPORTED) to create specific version ranges where it evaluates differently based on Node.js versions. This is intentional logic and not redundant, as it creates a specific gap for Node.js versions 16-18.18 where MODULE_REGISTER_SUPPORTED is false.
test/ts-runner.spec.ts (1)
Learnt from: JounQin
PR: un-ts/synckit#222
File: src/helpers.ts:13-13
Timestamp: 2025-04-02T06:32:20.004Z
Learning: Tests in the synckit project run in an ESM-only environment, so CommonJS patterns like require() should not be used in test files. Always use ESM import syntax and proper mocking approaches compatible with ESM.
src/helpers.ts (3)
Learnt from: JounQin
PR: un-ts/synckit#222
File: src/helpers.ts:13-13
Timestamp: 2025-04-02T06:32:20.004Z
Learning: Tests in the synckit project run in an ESM-only environment, so CommonJS patterns like require() should not be used in test files. Always use ESM import syntax and proper mocking approaches compatible with ESM.
Learnt from: JounQin
PR: un-ts/synckit#222
File: src/helpers.ts:13-13
Timestamp: 2025-04-02T06:41:24.897Z
Learning: When testing ESM modules with Jest, don't use jest.spyOn() directly on named imports as it won't work. Instead, use jest.mock() to mock the entire module with a factory function that returns mock implementations of the exported functions. For TypeScript, you'll need to cast the imported functions to jest.Mock when using them in tests.
Learnt from: JounQin
PR: un-ts/synckit#222
File: src/helpers.ts:13-13
Timestamp: 2025-04-02T06:45:12.656Z
Learning: In Node.js, `process.stdout` and `process.stderr` are readonly properties that cannot be directly reassigned in tests. Instead of replacing them with mock objects, either save and restore their internal methods (like `_writev`), use `jest.spyOn()` on specific methods, or use dedicated stdout/stderr mocking libraries for testing functions that interact with standard I/O streams.
🧬 Code Definitions (5)
test/fn.spec.ts (2)
test/helpers.ts (1)
testIf(23-23)src/constants.ts (1)
TS_ESM_PARTIAL_SUPPORTED(34-38)
src/index.ts (1)
src/constants.ts (1)
MODULE_REGISTER_SUPPORTED(44-47)
test/helpers.spec.ts (3)
src/helpers.ts (8)
hasRequireFlag(63-64)hasImportFlag(66-67)hasLoaderFlag(69-70)dataUrl(60-61)md5Hash(334-336)extractProperties(465-473)setupTsRunner(72-332)overrideStdio(676-695)src/constants.ts (9)
REQUIRE_FLAG(98-98)REQUIRE_ABBR_FLAG(100-100)IMPORT_FLAG(96-96)LOADER_FLAG(104-104)TsRunner(4-21)TsRunner(23-23)TRANSFORM_TYPES_NODE_VERSION(53-53)DEFAULT_TYPES_NODE_VERSION(59-59)TRANSFORM_TYPES_FLAG(62-62)test/helpers.ts (6)
workerCjsPath(17-17)workerCjsTsPath(11-11)workerTsPath(16-16)workerMjsAsMtsPath(19-19)workerMtsPath(20-20)testIf(23-23)
src/constants.ts (1)
src/common.ts (2)
compareNodeVersion(29-30)hasFlag(3-4)
src/helpers.ts (3)
src/constants.ts (9)
IMPORT_FLAG(96-96)TsRunner(4-21)TsRunner(23-23)LOADER_FLAG(104-104)REQUIRE_ABBR_FLAG(100-100)IMPORT_FLAG_SUPPORTED(111-111)MODULE_REGISTER_SUPPORTED(44-47)TS_ESM_PARTIAL_SUPPORTED(34-38)MTS_SUPPORTED(41-41)src/common.ts (1)
compareNodeVersion(29-30)src/types.ts (1)
AnyFn(6-6)
🪛 GitHub Check: codecov/patch
src/helpers.ts
[warning] 208-208: src/helpers.ts#L208
Added line #L208 was not covered by tests
[warning] 215-215: src/helpers.ts#L215
Added line #L215 was not covered by tests
[warning] 222-222: src/helpers.ts#L222
Added line #L222 was not covered by tests
[warning] 253-253: src/helpers.ts#L253
Added line #L253 was not covered by tests
[warning] 260-260: src/helpers.ts#L260
Added line #L260 was not covered by tests
[warning] 538-538: src/helpers.ts#L538
Added line #L538 was not covered by tests
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
- GitHub Check: Lint and Test with Node.js 18.18 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 23 on windows-latest
- GitHub Check: Lint and Test with Node.js 20 on windows-latest
- GitHub Check: Lint and Test with Node.js 23 on windows-latest
- GitHub Check: Lint and Test with Node.js 18 on windows-latest
- GitHub Check: Lint and Test with Node.js 18.18 on windows-latest
- GitHub Check: Lint and Test with Node.js 22 on windows-latest
🔇 Additional comments (51)
.changeset/rare-carrots-brush.md (1)
1-6: Changeset looks good!The changeset correctly documents the feature addition of
oxcTypeScript runner support and the correction forswcTypeScript runner with--importflag support. The minor version bump is appropriate for these feature additions..changeset/honest-onions-add.md (1)
1-6: Changeset correctly documents the bug fix.The patch version bump is appropriate for fixing the issue where
.cjsfiles were incorrectly treated as ESM modules..changeset/tender-peaches-smell.md (1)
1-6: Changeset appropriately documents the behavior change.The patch version bump is correct for this type of fix. The description concisely explains the conditional consideration of
process.features.typescriptbased on the--no-experimental-strip-typesflag.package.json (1)
76-80: Added new dependencies to support OXC and SWC TypeScript runners.The additions of
@oxc-node/core,@swc-node/register,@swc/core, and@swc/helpersas development dependencies are appropriate for implementing the OXC TypeScript runner support mentioned in the PR objectives.test/fn.spec.ts (3)
20-24: Replaced local variable with exported constant for better maintainability.The code now uses
TS_ESM_PARTIAL_SUPPORTEDfrom the synckit module instead of a local variable, improving maintainability by centralizing the ESM support detection logic.
48-48: Updated test condition to use centralized constant.Replacing the custom condition with the exported
TS_ESM_PARTIAL_SUPPORTEDconstant ensures consistent behavior across the codebase for conditionally running tests based on TypeScript ESM support.
62-62: Updated test condition to use centralized constant.Replacing the custom condition with the exported
TS_ESM_PARTIAL_SUPPORTEDconstant ensures consistency with other tests that depend on TypeScript ESM support.src/index.ts (2)
10-10: Simplified imports by using consolidated constants.Replaced individual version constant imports with the more expressive
MODULE_REGISTER_SUPPORTEDconstant, which encapsulates the version check logic.
107-108: Improved Node module registration logic with consolidated constant.Replaced manual version comparison with
MODULE_REGISTER_SUPPORTEDconstant to determine when to register the custom PnP loader. This change aligns with the PR comments regarding the intended behavior of module registration across different Node.js versions:
- Node < 16: true
- Node 16-18.18: false
- Node 18.19-20.7: true
- Node >= 20.8: true
test/helpers.spec.ts (5)
226-233: Added test coverage for the new OXC TypeScript runner.The test verifies that the OXC runner correctly adds the import flag and registers the @oxc-node/core module, which is essential for the PR's objective of adding OXC TypeScript runner support.
159-243: Comprehensive test suite for setupTsRunner function.The tests cover various TypeScript runner configurations, file extensions, and edge cases, providing good coverage for the setupTsRunner function which is critical for the proper functioning of the TypeScript runners, including the newly added OXC runner.
35-157: Good test coverage for helper utilities.The tests thoroughly verify the behavior of flag detection utilities, data URL handling, MD5 hashing, and property extraction functions, ensuring the reliability of these helper functions used throughout the codebase.
245-295: Thorough tests for stdio overriding functionality.The tests verify that stdout and stderr are properly captured in the expected format, which is important for preserving console output during worker thread execution.
211-224:Details
✅ Verification successful
Verify the test logic for TRANSFORM_TYPES_FLAG.
This test has conditional expectations based on Node.js version comparison, which could be confusing. It expects
TRANSFORM_TYPES_FLAGto be absent whenDEFAULT_TYPES_NODE_VERSIONis reached, but present for older versions (still newer thanTRANSFORM_TYPES_NODE_VERSION).
🌐 Web query:
What is the behavior of --experimental-transform-types flag in newer Node.js versions? Has it been replaced or made default?💡 Result:
The
--experimental-transform-typesflag in Node.js 22.7.0 and later versions enables full TypeScript transpilation (including features like enums and namespaces) rather than just type stripping. This feature remains experimental as of Node.js 23.6.0 but works alongside newer defaults:Key Behaviors
Transpilation Support:
Transforms TypeScript-specific syntax (enums, namespaces, parameter properties) into valid JavaScript using SWC under the hood[4][7][10]. For example:enum Direction { Up, Down } // Transpiles to JS runtime code[4][7][10]
Source Maps:
Automatically enabled when using this flag to preserve debugger accuracy[7][10].Complementary to Default Type Stripping:
Node.js 23.6.0+ enables--experimental-strip-types(basic type erasure) by default, letting users run.tsfiles natively without flags[2][9]. However,--experimental-transform-typesis required for advanced features that need codegen[10].Current Status (April 2025)
- Not Default: The flag remains experimental and opt-in[4][6][9].
- Activation: Must be explicitly enabled:
[4][7]node --experimental-transform-types app.tsLimitations
- Requires avoiding TypeScript features incompatible with pure type stripping (enums etc.) unless using this flag[1][5][10].
- Ignores
tsconfig.jsonsettings like paths or target JS versions[7][9].Comparison Table
Feature --experimental-strip-types(default in Node 23+)--experimental-transform-typesType Erasure Yes (inline whitespace replacement) Yes Syntax Transpilation No Yes (enums, namespaces, etc.) Source Maps Disabled Enabled Required for TS Features No (basic type annotations only) Yes (advanced syntax) For most users, the default type stripping in Node.js 23+ suffices, while
--experimental-transform-typesserves as an opt-in for projects needing full TS transpilation[9][10].Citations:
- 1: https://dev.to/dharamgfx/stop-compiling-typescript-run-it-natively-in-nodejs-with-this-one-flag-2bc6
- 2: https://2ality.com/2025/01/nodejs-strip-type.html
- 3: nodejs/TSC#1445
- 4: https://nodejs.org/en/blog/release/v22.7.0
- 5: https://devclass.com/2024/07/26/node-js-adds-experimental-typescript-support-as-it-simply-cannot-be-ignored/
- 6: https://aws.amazon.com/blogs/compute/node-js-22-runtime-now-available-in-aws-lambda/
- 7: https://www.telerik.com/blogs/nodejs-features-you-didnt-know-about-part-2
- 8: https://blog.risingstack.com/nodejs-23/
- 9: https://www.totaltypescript.com/typescript-is-coming-to-node-23
- 10: https://satanacchio.hashnode.dev/everything-you-need-to-know-about-nodejs-type-stripping
Clarify Conditional Expectation for --experimental-transform-types in TsRunner Test
- In the test at
test/helpers.spec.ts(lines 211–224), two Node.js version thresholds are used:
- When the Node version is at least
DEFAULT_TYPES_NODE_VERSION, the test expects thatTRANSFORM_TYPES_FLAGis not added toexecArgv(implying that basic type stripping is enabled by default).- For older Node versions (but still meeting the minimum from
TRANSFORM_TYPES_NODE_VERSION), the flag is expected to be present so that full TypeScript transpilation (required for advanced features like enums and namespaces) is enabled.- Our recent web query confirms that the
--experimental-transform-typesflag remains experimental and must be explicitly enabled for advanced TS transformations, while newer Node.js versions (23.6.0+) use basic type stripping by default.- Suggestion: Although the test logic aligns with the current Node.js behavior, please consider adding inline comments that explain the purpose of using these two distinct version thresholds. This clarification will help future maintainers understand why the flag is conditionally included.
src/helpers.ts (20)
13-13: Imports look good.
Bringing incompareNodeVersionaligns with the new standardized version comparison approach.
21-22: New flags in constants.
ImportingIMPORT_FLAGandIMPORT_FLAG_SUPPORTEDkeeps the code consistent with the rest of the logic.
24-27: LOADER and MTS constants.
AddingLOADER_FLAG,LOADER_FLAGS,MODULE_REGISTER_SUPPORTED, andMTS_SUPPORTEDensures all relevant Node.js capabilities are readily accessible.
31-32: Require flags for Node.js.
REQUIRE_ABBR_FLAGandREQUIRE_FLAGSproperly unify all short and long require arguments.
37-37: Partial ESM support reference.
ImportingTS_ESM_PARTIAL_SUPPORTEDclarifies Node.js version ranges that allow partial TS ESM.
63-64:hasRequireFlag: Clear helper.
This function precisely checks if any known require flag is present, improving readability.
66-67:hasImportFlag: Minimal and direct.
This is a straightforward check for the--importflag.
69-70:hasLoaderFlag: Straightforward loader detection.
Helps centralize the check for loader flags in one place.
144-144: Version check for type stripping.
UsingcompareNodeVersion(STRIP_TYPES_NODE_VERSION) >= 0is consistent with the rest of the code’s approach.
158-158: Negative version check.
This logic properly prevents usage of strip-types on older Node.js versions.
169-169: Default types node version.
Ensuring we skip or include certain logic whenDEFAULT_TYPES_NODE_VERSIONis met or exceeded.
175-175: Transform types check.
This condition paves the way for advanced transformations when the Node.js version supports it.
181-181: Additional strip-types check.
Confirming presence of the strip-types flag ensures correct Node CLI behavior.
190-200:TsRunner.TsNodeblock.
Logic properly toggles between loader usage and the abbreviated require flag. Consider ensuring test coverage for both ESM and CJS paths here.Would you like to add a dedicated test verifying each branch? For instance, verifying the presence/absence of the loader or require flags?
219-226:EsbuildRunnercase.
Similar to Esbuild Register, it conditionally appends-r esbuild-runner/register. The coverage tool warns about line 222; ensure there's a test covering the scenario when no require flags exist.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 222-222: src/helpers.ts#L222
Added line #L222 was not covered by tests
230-240:OXCrunner logic.
Inserting[--import, '@oxc-node/core/register']is consistent with OXC usage. Looks good for .ts worker scripts.
243-265:SWCrunner branching.
Lines 253 and 260 lack coverage according to the analysis. Verify the ESM partial support path vs. fallback loader usage.🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 253-253: src/helpers.ts#L253
Added line #L253 was not covered by tests
[warning] 260-260: src/helpers.ts#L260
Added line #L260 was not covered by tests
479-500: Enhanced JSDoc forstartWorkerThread.
Clear, detailed documentation fosters readability and clarifies usage for future maintainers.
526-526:isTsxSupportedaddition.
Defining localisTsxSupported = !tsUseEsm || TS_ESM_PARTIAL_SUPPORTEDclarifies conditional usage of TSX. Looks correct.
206-215:Details
❓ Verification inconclusive
EsbuildRegisterESM vs. require logic.
Lines 208 and 215 are flagged by coverage tools. Please add unit tests verifying ESM usage (tsUseEsm) and fallback tohasRequireFlag.
🏁 Script executed:
#!/bin/bash # Searching for test coverage references of esbuild-register lines rg -A 3 "TsRunner.EsbuildRegister"Length of output: 1665
Unit Test Coverage Check for ESM vs. require Flag Logic
Based on the recent test coverage search, we can see that
TsRunner.EsbuildRegisteris referenced in several tests withintest/ts-runner.spec.ts. However, it's not entirely clear that both branches—ESM usage (adding the loader flag) and the fallbackhasRequireFlagpath—are fully exercised by the current tests.Please ensure that unit tests explicitly cover the following cases:
- When
tsUseEsmis true and no loader flag is present, verify that theLOADER_FLAGalong with the correct loader path ("${TsRunner.EsbuildRegister}/loader") is prepended toexecArgv.- When
tsUseEsmis false and the require flag is missing, verify that the require fallback flag (REQUIRE_ABBR_FLAG) andTsRunner.EsbuildRegisterare correctly added.Once confirmed, this will address the flagged lines (208 and 215) by the coverage tool.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 208-208: src/helpers.ts#L208
Added line #L208 was not covered by tests
[warning] 215-215: src/helpers.ts#L215
Added line #L215 was not covered by testssrc/constants.ts (7)
1-1: Updated imports reflect new functionalityThe imports have been updated to use
compareNodeVersionandhasFlaghelpers from the common module, which aligns with the refactoring of version comparison logic throughout the file.
15-16: Added OXC TypeScript runner supportThis change adds support for the
oxcTypeScript runner, which is the main objective of this PR (as stated in the PR description). The link to the GitHub repository is provided as a comment for reference.
33-39: Added TypeScript ESM compatibility constantThis constant checks for Node.js versions where TypeScript ESM support is partial (≥16 and <18.19), with a reference to a relevant GitHub issue. The version comparison logic is clear and follows the pattern used elsewhere in the codebase.
40-42: Added MTS_SUPPORTED constant for module.registerThis constant correctly identifies Node.js version 20.8+ where the
module.registerfeature is supported. The comment clearly explains the requirement.
43-48: MODULE_REGISTER_SUPPORTED uses intentional OR conditionThis constant correctly implements the logic for checking if the
module.registerfeature is supported. As noted in the retrieved learnings, this OR condition is intentional and creates specific version ranges where the constant evaluates differently based on Node.js versions.
67-72: Updated NO_STRIP_TYPES to use newer comparison functionsThe implementation has been updated to use the new
compareNodeVersionfunction and thehasFlaghelper, maintaining the same logic but with improved code organization.
96-112: Added constants for Node.js execution flagsThese new constants define the various flags used for module loading and imports in Node.js. Defining them as constants improves maintainability and reduces the risk of typos. The
IMPORT_FLAG_SUPPORTEDconstant correctly checks for Node.js version 20.6 or higher.test/helpers.ts (2)
7-7: Good adjustment to import type declaration.The explicit
typeimport forWorkerToMainMessageimproves type safety and helps TypeScript's type checker.
11-21: Well organized path constants for test files.This refactoring improves code organization by centralizing all worker file paths in one place. These constants can now be reused across different test files, making the codebase more maintainable.
test/ts-runner.spec.ts (8)
1-1: Updated ESLint configuration to handle conditional expects.Good addition of the
jest/no-conditional-expectESLint disable comment, which is necessary since the tests contain conditional expect statements.
7-7: Updated imports to reflect new version comparison approach.The imports have been updated to use
compareNodeVersionand new constants (TS_ESM_PARTIAL_SUPPORTED,MTS_SUPPORTED) instead of the previous approach. This aligns with the broader refactoring in the project.Also applies to: 14-16
44-50: Updated conditional test for EsbuildRegister with .mts files.The condition has been changed from
tsUseEsmSupportedtoTS_ESM_PARTIAL_SUPPORTED, which provides more precise control over which test environments should run this test based on Node.js version compatibility.
70-76: Updated conditional test for EsbuildRunner with .mts files.Similar to the previous comment, this updated condition provides more precise control over test execution based on Node.js version compatibility.
79-110: Added comprehensive test coverage for the new OXC TypeScript runner.This new test follows the same pattern as other runner tests, effectively validating the new
oxcTypeScript runner functionality. The test covers both standard JavaScript files (.js) and ES modules (.mjs), with appropriate conditional testing for .mts files based on Node.js version support.
129-142: Updated conditional testing logic for SWC runner with .mts files.The updated condition provides more granular control by first checking
TS_ESM_PARTIAL_SUPPORTEDand thenMTS_SUPPORTED, allowing different test behaviors based on the Node.js version.
155-168: Improved conditional testing for TSX runner.The logic flow has been inverted to first check for
TS_ESM_PARTIAL_SUPPORTED, which allows running the positive case first when supported. This improves readability by following a "happy path first" approach.
175-175: Updated version comparison approach.Replaced the old version comparison with the new
compareNodeVersionfunction, making the version comparison more consistent across the codebase.
close #216
Summary by CodeRabbit
New Features
TsRunner.OXCand utility functions.Refactor
Tests
Chores