-
-
Notifications
You must be signed in to change notification settings - Fork 450
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(isolated_declarations): Don't require private method arguments to have type annotations #5874
Conversation
… have type annotations
Your org has enabled the Graphite merge queue for merging into mainAdd the label “0-merge” to the PR and Graphite will automatically add it to the merge queue when it’s ready to merge. Or use the label “hotfix” to add to the merge queue as a hot fix. You must have a Graphite account and log in to Graphite in order to use the merge queue. Sign up using this link. |
@@ -404,6 +406,7 @@ impl<'a> IsolatedDeclarations<'a> { | |||
} | |||
|
|||
if method.accessibility.is_some_and(TSAccessibility::is_private) { | |||
self.errors.borrow_mut().truncate(errors_length); |
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.
This is a little bit hacky. I think a less hacky way might be to decouple methods like transform_formal_parameters
from generating errors and then move the check for errors after this block, but I wanted to get some feedback before starting to attempt something like 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.
I think we can handle the private method early, can you try it?
CodSpeed Performance ReportMerging #5874 will not alter performanceComparing Summary
|
Sorry, I didn't see any errors in your case. The private method will turn into a property without type annotations. The output is export class Foo {
private foo;
} |
That's the correct behavior, but currently there is an error when using oxc_isolated_declarations. This PR removes the error. If you pull this branch locally and undo the change I made to |
Ah, you are right. I found our example that doesn't report any isolated declaration errors. Fixes in #5918 |
Fixed in #5964 We are about to release a new version that includes fixes for many incorrect outputs in |
Thank you! |
In #4934 I introduced a regression which caused the following code to begin having an isolated declarations error where it shouldn't:
TS playground