-
Notifications
You must be signed in to change notification settings - Fork 0
Closed
Labels
bugSomething isn't workingSomething isn't working
Description
Technical Review: CJS↔ESM Transformation (Both Directions) and Platform Support
A review of the transformation pipeline with a focus on:
- Cross-platform (Linux/Windows) reliability
- CJS→ESM and ESM→CJS direction-specific edge cases
- Core features such as specifier rewrite, dual-package hazard, and circular detection
Issues Identified (Directionality Specified)
Applies to Both (CJS→ESM and ESM→CJS):
- Template literal specifier rewriting does not guard against expressions
- Risk: Might rewrite non-static specifiers with interpolations.
- Fix: Skip update or surface an explicit flag to callbacks.
- Default options object is vulnerable to mutation
- Risk: Passing the shared defaultOptions can cause global options state leaks if mutated by consumers.
- Fix: Always spread from a fresh default per call.
- Built-in specifier set logic duplicated across modules
- Risk: Potential for drift between modules as Node adds/changes built-ins.
- Fix: Extract to shared utility to ensure parity.
- Import cleanup opportunity
- Remove unused/duplicate imports in
module.tsfor clarity.
- Remove unused/duplicate imports in
CJS→ESM Specific:
- Cycle/circular dependency detection gaps (primary impact is CJS→ESM)
- Currently, this logic runs only when transforming to ESM (
opts.target === 'module'). - Gaps:
- Only considers
.js/.cjs/.mjsextensions and directory indices. - Ignores
.ts/.tsx/.mts/.cts, so cycles in mixed TS/JS projects are missed. - Path tracking does not realpath/normalize—may miss or duplicate cycles/barriers on Windows (case, separator) or with symlinks.
- Only considers
- Impact: False negatives or duplicate cycle warnings, especially on Windows or with mixed file types.
- Recommendation: Extend candidate extensions and use fs.realpath + path.normalize when tracking dependency graphs in cycle/checks.
- Currently, this logic runs only when transforming to ESM (
ESM→CJS Specific:
- Top-level await (TLA) transform behavior
- The pipeline allows explicit TLA policy (error, wrap, preserve)—this is well-structured. No bugs observed, but clarity in docs is important, as wrapping changes execution order in CJS output.
- Interop helpers for ESM→CJS (e.g., __esModule, createRequire) are injected as needed—current implementation appears robust from provided snippets.
- No blocking issues/risks identified for lowering ESM→CJS beyond general template, options, and specifier safeguards (above).
Recommendations
- Add TS extension and path normalization to cycle detection (CJS→ESM only).
- Guard against rewriting template literals containing expressions (Both directions).
- Refactor default options merging to prevent shared-object mutation (Both).
- Extract built-in specifier set to a common utility (Both).
- Update documentation or diagnostics for TLA/wrap in ESM→CJS path.
- Tidy imports (Both).
Impact
- Reliability and safety when working with cross-platform and mixed codebases.
- Correctness in detecting cycles for complex or mixed JS/TS projects (CJS→ESM only).
- Prevents accidental mutation bugs for all consumers.
- Hardened transformation for all entry points.
If you want concrete patch suggestions for each gap, let me know.
Reactions are currently unavailable
Metadata
Metadata
Assignees
Labels
bugSomething isn't workingSomething isn't working