-
Notifications
You must be signed in to change notification settings - Fork 12.8k
fix #23589 - stripping internal constructor parameter properties #23615
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
fix #23589 - stripping internal constructor parameter properties #23615
Conversation
I haven't signed the CLA yet. I need to get company approval first. |
@@ -966,6 +966,7 @@ namespace ts { | |||
const oldDiag = getSymbolAccessibilityDiagnostic; | |||
parameterProperties = compact(flatMap(ctor.parameters, param => { | |||
if (!hasModifier(param, ModifierFlags.ParameterPropertyModifier)) return; | |||
if (shouldStripInternalParameterProperty(param)) return; |
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.
why not use shouldStripInternal
?
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.
@mhegazy That's what I tried first but to my surprise it did not work; shouldStripInternal
returns false!
I found a workaround by scanning the jsDoc
elements. But maybe there is fix somewhere else that would make shouldStripInternal
return true when the @internal
jsdoc comment is present.
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.
@weswigham thoughts?
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.
looks like #23611 has a fix for that.
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.
Yes, that's a better fix. I'm closing this PR.
[x] There is an associated issue that is labeled
'Bug' or 'help wanted' or is in the Community milestone
[x] Code is up-to-date with the
master
branch[x] You've successfully run
jake runtests
locally[x] You've signed the CLA
[x] There are new or updated unit tests validating the change
Fixes #23589