-
-
Notifications
You must be signed in to change notification settings - Fork 721
feat(linter): add plugin for angular and implement rule prefer standalone #14959
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
feat(linter): add plugin for angular and implement rule prefer standalone #14959
Conversation
How to use the Graphite Merge QueueAdd either label to this PR to merge it via the merge queue:
You must have a Graphite account in order to use the merge queue. Sign up using this link. An organization admin has enabled the Graphite Merge Queue in this repository. Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue. |
|
I think this would be better as an external plugin? Does the angular eslint plugin not work with oxlint? |
| if !bool_value.value { | ||
| ctx.diagnostic_with_suggestion( | ||
| prefer_standalone_diagnostic(prop.span()), | ||
| |fixer| fixer.replace(prop.span(), ""), |
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.
The auto-fix produces invalid JavaScript syntax. When removing standalone: false, from the middle of an object, it leaves a leading comma on the next property. The test expectations show this bug:
@Component({
, // <- Invalid syntax!
template: '<div></div>'
})Fix: The fixer needs to handle comma removal properly. Replace with logic that removes the property along with its trailing comma, or if it's the last property, remove it along with the preceding comma.
| |fixer| fixer.replace(prop.span(), ""), | |
| |fixer| { | |
| // Get the text of the entire file | |
| let source = fixer.source_text(); | |
| let span = prop.span(); | |
| // Find the next non-whitespace character after the property | |
| let mut end = span.end; | |
| while end < source.len() && (source.as_bytes()[end] as char).is_whitespace() { | |
| end += 1; | |
| } | |
| // If the next character is a comma, include it in the removal | |
| if end < source.len() && source.as_bytes()[end] as char == ',' { | |
| end += 1; | |
| fixer.replace(oxc_span::Span::new(span.start, end), "") | |
| } else { | |
| // Otherwise, look for a preceding comma | |
| let mut start = span.start; | |
| while start > 0 && (source.as_bytes()[start - 1] as char).is_whitespace() { | |
| start -= 1; | |
| } | |
| // If there's a comma before the property, remove it too | |
| if start > 0 && source.as_bytes()[start - 1] as char == ',' { | |
| start -= 1; | |
| fixer.replace(oxc_span::Span::new(start, span.end), "") | |
| } else { | |
| fixer.replace(span, "") | |
| } | |
| } | |
| }, |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
| const ANGULAR_TEST_PATH: &str = | ||
| "https://raw.githubusercontent.com/angular-eslint/angular-eslint/tree/main/packages/eslint-plugin/tests/rules"; | ||
| const ANGULAR_RULES_PATH: &str = | ||
| "https://raw.githubusercontent.com/angular-eslint/angular-eslint/tree/main/packages/eslint-plugin/src/rules"; |
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.
The URLs contain /tree/main/ which is incorrect for raw.githubusercontent.com. This path segment is only used in the GitHub web interface, not for raw content access. These URLs will return 404 errors when the rule generator tries to fetch documentation.
Fix: Remove /tree from the URLs:
const ANGULAR_TEST_PATH: &str =
"https://raw.githubusercontent.com/angular-eslint/angular-eslint/main/packages/eslint-plugin/tests/rules";
const ANGULAR_RULES_PATH: &str =
"https://raw.githubusercontent.com/angular-eslint/angular-eslint/main/packages/eslint-plugin/src/rules";| const ANGULAR_TEST_PATH: &str = | |
| "https://raw.githubusercontent.com/angular-eslint/angular-eslint/tree/main/packages/eslint-plugin/tests/rules"; | |
| const ANGULAR_RULES_PATH: &str = | |
| "https://raw.githubusercontent.com/angular-eslint/angular-eslint/tree/main/packages/eslint-plugin/src/rules"; | |
| const ANGULAR_TEST_PATH: &str = | |
| "https://raw.githubusercontent.com/angular-eslint/angular-eslint/main/packages/eslint-plugin/tests/rules"; | |
| const ANGULAR_RULES_PATH: &str = | |
| "https://raw.githubusercontent.com/angular-eslint/angular-eslint/main/packages/eslint-plugin/src/rules"; |
Spotted by Graphite Agent
Is this helpful? React 👍 or 👎 to let us know.
|
@TheAlexLichter Here is the PR as discussed. |
5e90396 to
03d3d4d
Compare
I will check. |
I tried to use oxlint with the {
"$schema": "./node_modules/oxlint/configuration_schema.json",
"plugins": [],
"jsPlugins": ["angular-eslint"],
...
}When running @camc314 What do you mean by "external plugin"? Do you refer to the current ESLint plugin? |
|
yeah, by external, i mean not built into oxlint e.g. JS plugin Let see in the next release (later today) if it's working as expected, and go from there |
|
@camc314 Sounds good. As soon as the new version is released I will give it a try. Anyways, that means that rules for Angular are still written in JS. So my understanding is that migrating them to Rust would make all of them faster as well. Are there any plans to include more JS-based rules into Rust? I would try to help with some stuff since most projects I work on are Angular projects and I would be happy to use Oxlint instead of ESLint. |
@camc314 Now it seems to work as expected with |
awesome, i'm glad it's working
Yes, migrating the angular rules to rust would likley improve performance.
Not at this time, once custom JS plugins and type aware linting is more stable, we may re-evaluate. The main reasoning for this is just the high maintence burden of supporting more plugins when the JS version is probably fast enough for most people's use cases. That said, if demand for a particular plugin is incredibly high, we may re-evaluate that decision.
This is a more complex topic, we're hoping that custom JS plugins with raw transfer will be fast enough such that we don't need to build out a rust based plugin system. |
|
Thanks for working on this, and for using oxlint, but i'm going to close this for now. |
|
@camc314 Thank you for sharing some insights about your plans and your point of view.
Sure, thanks for your support. |
This PR adds support for Angular ESLint rules according to angular-eslint/angular-eslint.
It also implements first recommended rule
prefer-standalone.