-
Notifications
You must be signed in to change notification settings - Fork 731
Fix: Support parameter properties in class constructors #619
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
@microsoft-github-policy-service agree company="Zanuka Labs" |
1 similar comment
|
@microsoft-github-policy-service agree company="Zanuka Labs" |
|
Is this a direct port from TS codebase or new addition to the codebase? Looking at issue #536 seems like constructor body transformation from constructor parameters to class properties hasn't been ported yet. |
|
@BansalShivanshu This is not a direct port of specific code from the TypeScript codebase, but rather a new implementation following the same transformation pattern that TypeScript uses. Since the TS compiler handles parameter properties through its transformation pipeline, it's my understanding that this core pattern involves:
I noticed that issue #536 correctly identified that this functionality wasn't properly implemented, so this PR fills this gap by adding logic to:
It follows TypeScript's pattern conceptually, though the specific code structure differs due to the language differences (Go vs TypeScript) and the architectural differences between the codebases. I think all edge cases have been handled including:
Happy to make adjustments as needed if anything was missed. I fixed the extra blank lines that had caused the initial check fail as well. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe the issue was actually fixed 4 days ago here: 8b95466#diff-602f9f2ee92480e93b3b71e218795f3b947dd7745781272ef927d1f2dd431ec1R718
| this.x = x; | ||
| } | ||
| }`}, | ||
| {title: "multiple parameter properties", input: `class Project { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running this test without the changes on up to date main, it also passes. I'm not so sure this is fixing anything, as there doesn't seem to be an issue at first look. Are you experiencing something different?
Theres of course still an issue with a super call's position as it relates to the parameter initializations, which I think just needs a minimal fix (#547)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the comments, much appreciated. After further investigation and testing, I've confirmed that this PR is redundant. The parameter property transformation functionality is already properly implemented in the codebase through commit 8b95466 (PR #479).
I checked the following:
- All tests pass on the main branch without these changes
- The existing implementation already handles:
- Parameter property detection
- Modifier stripping
- Property assignments in constructor
- Super call handling
- Multiple parameter properties
I'll close this PR since it would be duplicating existing functionality.
This PR addresses issue #536 where parameter properties in TypeScript class constructors
were not properly supported during transformation. Parameter properties like
constructor(public name: string)are now correctly transformed into class propertiesand assignments in the constructor body.
The fix modifies the constructor declaration visitor to properly handle parameter
property modifiers and generate the appropriate class properties.
Tests have been run and verified that the transformation now correctly handles all
parameter property cases.