Skip to content

Fix methods containing nested functions and arrows - #231

Merged
redonkulus merged 3 commits into
yahoo:mainfrom
Shubham-Padkonde:fix/method-body-function-detection
Oct 23, 2026
Merged

redonkulus merged 3 commits into
yahoo:mainfrom
Shubham-Padkonde:fix/method-body-function-detection

Conversation

@Shubham-Padkonde

@Shubham-Padkonde Shubham-Padkonde commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Fixes #118. The function-kind regexes currently match anywhere in the source, so a concise method containing an arrow or function declaration is returned unchanged. That source is invalid in the generated object-property value and fails to deserialize.

Anchor detection to the outer function signature so concise methods still receive the existing function-expression conversion. Added round-trip cases for nested declarations, destructured arrow arguments, async methods, and generator methods. Additional cases preserve commented async signatures and arrows with function-valued defaults.

Explicitly preserve class expressions as well, including classes with and without nested arrows. A concise method named class still receives method conversion.

Validation (23 October 2026, Windows/Node 24.15.0): both new class regression tests fail before this follow-up and pass afterward; the concise-method guard passes. All 109 tests pass with node --test test/unit/*.js. git diff --check passes.

@redonkulus

Copy link
Copy Markdown
Collaborator

Thanks for this, and for the thorough round-trip tests. Anchoring the checks to the outer signature is the right fix for #118, and it merges cleanly onto current main with all tests passing. One regression before we can merge:

Classes now fall through to the method conversion. A class source starts with class, so it matches neither anchored regex and gets rewritten as if it were a concise method:

class A { m() { return x => x } }
serialize({ A })
// main: {"A":class A { m() { return x => x } }}      ← valid
// PR:   {"A":function(){ return x => x } }}          ← SyntaxError

(On main this only works by accident, because the unanchored arrow regex matches the nested =>. A class with no nested arrow or function, like class A { m() { return 1 } }, is already broken on main.)

Could you detect a class signature explicitly (e.g. /^class\b/ → emit the source unchanged) and add round-trip tests for both of those class cases? That would fix the regression and the existing bug together.

@redonkulus redonkulus left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fix and the quick turnaround on the class regression! Verified against current main: all tests pass, and nested arrows/declarations in concise, async, generator and computed-key methods now round-trip correctly.

@redonkulus
redonkulus merged commit fdb853d into yahoo:main Oct 23, 2026
4 checks passed
@redonkulus redonkulus mentioned this pull request Oct 23, 2026
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.

Bug when used an object with a function and a variable with a function and destructuring

2 participants