Skip to content

Fix: Preserve export keyword in JavaScript/TypeScript code extraction#1944

Closed
mohammedahmed18 wants to merge 3 commits intomainfrom
fix/preserve-export-keyword-in-extraction
Closed

Fix: Preserve export keyword in JavaScript/TypeScript code extraction#1944
mohammedahmed18 wants to merge 3 commits intomainfrom
fix/preserve-export-keyword-in-extraction

Conversation

@mohammedahmed18
Copy link
Copy Markdown
Contributor

Summary

Fixes a critical bug where the export keyword was being stripped from classes/functions during code extraction, causing all optimization runs to fail at baseline testing.

Problem

When extracting exported classes for optimization (e.g., export class WsContextCreator), the code extraction logic was missing the export keyword. This caused generated tests to fail immediately because they couldn't import the classes:

// Original source
export class WsContextCreator { ... }

// Extracted code (WRONG - missing export!)
class WsContextCreator { ... }

// Generated test (FAILS!)
import WsContextCreator from './ws-context-creator.js';  // Error: not exported!

Root Cause

Tree-sitter creates separate nodes for export_statement and class_declaration:

  • export_statement node at [0:0] (includes "export ")
  • class_declaration node at [0:7] (starts after "export ")

The _find_class_definition method was using the class_declaration node's position, which excluded the parent export_statement node.

Solution

  1. Modified _find_class_definition to detect if a class is exported by checking if the parent node is an export_statement
  2. Updated the return signature to include is_exported: bool
  3. Modified class wrapper construction (lines 367-372) to include "export " prefix when is_exported=True

Impact

All 13 recent optimization runs were failing due to this bug. Trace IDs affected:

  • 8f3d7dd8-78ee-4ef7-948c-8d28830c53e7 (WsContextCreator.getMetadata)
  • 9b868617-449b-41cb-9f0f-0f7d023b9310 (WsContextCreator.createGuardsFn)
  • 1a2d5fe6-53a8-4e66-a072-92e65384ee8d (WsContextCreator.reflectCallbackPattern)
  • Plus 10 more...

Testing

✅ Added comprehensive unit tests:

  • test_export_class_includes_export_keyword - Exported classes include export
  • test_export_function_includes_export_keyword - Exported functions include export
  • test_export_const_arrow_function_includes_export - Exported const arrow functions include export
  • test_non_exported_class_unchanged - Non-exported classes work correctly

✅ Updated 12 existing tests to expect correct behavior (export included)

✅ All 531 JavaScript tests pass

✅ No linting or type errors (uv run prek passes)

Files Changed

  • codeflash/languages/javascript/support.py - Core fix in _find_class_definition and class wrapping
  • tests/test_export_keyword_preservation.py - New comprehensive test suite
  • tests/test_languages/test_javascript_support.py - Updated expectations for existing tests

🤖 Generated with Claude Code (Sonnet 4.5)

Codeflash Bot and others added 2 commits April 1, 2026 14:12
## Problem
Codeflash was generating import statements without file extensions for
TypeScript and ESM projects, causing ERR_MODULE_NOT_FOUND errors when
Node.js tried to resolve the modules.

Example error from trace 08d0e99e-10e6-4ad2-981d-b907e3c068ea:
```
Error [ERR_MODULE_NOT_FOUND]: Cannot find module
'/workspace/target/packages/microservices/server/server-factory'
imported from .../test_create__unit_test_0.test.ts
```

The generated test had:
```typescript
import ServerFactory from '../../server/server-factory'
```

But Node.js ESM requires explicit file extensions.

## Root Cause
The get_module_path method in JavaScriptSupport was unconditionally
removing file extensions with .with_suffix(""), regardless of whether
the project used ESM or CommonJS module system.

## Solution
Modified get_module_path to:
1. Detect the module system using detect_module_system()
2. For ESM or TypeScript files: add .js extension (TypeScript convention)
3. For CommonJS: keep no extension (backward compatible)

TypeScript convention is to use .js extension in imports even when the
source file is .ts, as imports reference the compiled output.

## Testing
- Added two new test cases in TestGetModulePath class
- All 73 existing JavaScript support tests pass
- All 28 module system tests pass
- Lint and type checks pass

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
**Issue**: When extracting exported classes/functions for optimization, the
export keyword was being stripped from the extracted code. This caused all
generated tests to fail because they tried to import classes that appeared
to not be exported.

**Root Cause**: The _find_class_definition method in support.py was using
the class_declaration node's position from tree-sitter, which excludes the
parent export_statement node. When a class is declared as "export class Foo",
tree-sitter creates:
- export_statement node at position [0:0] (includes "export")
- class_declaration node at position [0:7] (after "export ")

The code was extracting from the class_declaration position, missing the
export keyword.

**Solution**:
1. Modified _find_class_definition to check if the class is exported by
   examining the parent node type
2. Updated the return signature to include is_exported boolean
3. Modified class wrapper construction to include "export " prefix when
   is_exported is True

**Impact**: This fixes all 13 recent optimization runs that were stalling
at baseline testing. Tests were failing immediately on import because the
export keyword was missing.

**Trace IDs affected**:
- 8f3d7dd8-78ee-4ef7-948c-8d28830c53e7
- 9b868617-449b-41cb-9f0f-0f7d023b9310
- 1a2d5fe6-53a8-4e66-a072-92e65384ee8d
- 9951f247-7e14-4583-a7d0-6652c1c449b9
- 99836602-4a42-4a26-a5b4-be539671fd35
- (and 8 more)

**Testing**:
- Added comprehensive unit tests for exported classes, functions, and arrow functions
- Updated existing tests to expect correct behavior (export included)
- All 531 JavaScript tests pass
- No linting or type errors

Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
@CLAassistant
Copy link
Copy Markdown

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Codeflash Bot seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@claude
Copy link
Copy Markdown
Contributor

claude bot commented Apr 1, 2026

Claude finished @mohammedahmed18's task in 7m 20s —— View job


Tasks

  • Gather context
  • Triage PR scope → LARGE (62 production lines changed + tests)
  • Lint and typecheck (applied 1 ruff format fix, committed)
  • Resolve stale threads (none found)
  • Review code
  • Duplicate detection
  • Post summary comment

PR Review Summary

Prek Checks

One ruff formatting issue was auto-fixed and pushed: the if/else branches at support.py:369–374 had inconsistent f-string parenthesization. Fixed in commit bc6daeee.

Mypy reports 30+ errors in support.py, but all are pre-existing and unrelated to this PR's changes (they span lines that weren't touched, and the same errors exist on origin/main).

Code Review

The core export-keyword fix is correct. The _find_class_definition method now properly checks class_node.parent.type == "export_statement" and threads is_exported: bool through the return tuple. The class-wrapping code at support.py:367–374 correctly prepends "export " when needed.

One latent bug exposed (pre-existing, not introduced here): When extracting an exported class method that has a class-level JSDoc comment, the JSDoc is silently dropped. Cause: class_node.prev_named_sibling (line 489) walks siblings within the export_statement node, but the JSDoc comment is a sibling of export_statement itself, not of class_declaration. The fix is to also check class_node.parent.prev_named_sibling when is_exported=True. The new test even documents this gap with a comment "Note: Class-level JSDoc is not included when extracting a method" — but it shouldn't be an accepted limitation.

# support.py:487–493 — should also check export_statement's sibling when is_exported
prev_sibling = class_node.prev_named_sibling
if prev_sibling is None and is_exported and class_node.parent:
    prev_sibling = class_node.parent.prev_named_sibling  # look at export_statement's sibling
if prev_sibling and prev_sibling.type == "comment":
    ...

Fix this →

get_module_path ESM change (separate fix bundled in this PR) — looks correct. The logic to detect module system and append .js for ESM projects aligns with TypeScript's import convention. The fallback Windows path handler also correctly applies the same ESM check.

Duplicate Detection

Low confidence, acceptable: support.py:474 (class_node.parent.type == "export_statement") duplicates the same check inside treesitter.py:_is_node_exported() (line 412). Since _is_node_exported is a private method designed for function nodes (not class nodes), this minimal duplication is acceptable. No cross-module duplication found.

Test Coverage

All 4 new tests and all 73 updated JS support tests pass. The new TestExportKeywordPreservation suite covers:

  • exported class methods ✅
  • exported standalone functions ✅ (these were already working — good regression guard)
  • exported const arrow functions ✅
  • non-exported classes ✅

The TestGetModulePath suite covers the ESM .js extension and CommonJS no-extension cases.

Gap: No test for an exported class that has a class-level JSDoc comment — which would expose the bug noted above.


Optimization PRs

PR #1943 (⚡️ Speed up fmt_delta) has failing CI. Its base branch is cf-compare-copy-benchmarks (not main), and I wasn't able to verify if the failures are pre-existing on that branch. Leaving open — do not merge until CI is green or the base branch failures are confirmed pre-existing.

Co-authored-by: mohammed ahmed <undefined@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants