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
Improving/replacing sort-ngmodule-metadata-arrays
#1232
Comments
@angular-eslint/sort-ngmodule-metadata-arrays
@angular-eslint/sort-ngmodule-metadata-arrays
@angular-eslint/sort-ngmodule-metadata-arrays
sort-ngmodule-metadata-arrays
FYI @JamesHenry I started looking into solutions for this issue. I have a draft PR up here, though it only contains boilerplate code for the new rule at the moment. Will continue to play around with it when I can. |
I have also played with this and have some sort of working version. I'll try to check that PR later if I could have some suggestions etc. :) I have opionated default groups for example :D |
Any news on this rule? |
I closed my draft PR (can always refer back to it whenever necessary) that attempted to resolve this issue because we discovered that there are cases where the order of imports matters. One case (as I commented in the draft PR) is when simulating a data server using the In-memory Web API. The docs explicitly state, "Always import the Another case is importing routing modules. There's a snippet in the Angular docs for the Router tutorial here that states, "The order of route configuration is important because the router accepts the first route that matches a navigation request path." With the Router tutorial example, if There could be other cases where the order of imports matters. It could be possible to use rule properties to work around these cases. However, even in the case where order does not matter, sorted imports is a formatting preference as described in this comment, so where to draw the line? Happy to discuss. |
Could sorting be ignored if there is an inline ignore rule? |
Yes and no... If I'm understanding correctly (anyone, please correct me if I'm wrong 🙂), ignore rule comments will prevent the reporting of warnings/errors, and will not prevent rule logic from processing code to evaluate. It could be possible to add logic to a rule to do something like, "if this import has an inline ignore comment, exclude it from sorting." However (personal opinion), I don't think rules should overload the purpose of |
In light of the number of issues reported with this rule I wanted to bring them together in one place in order to inform a greatly improved implementation of sorting
imports
metadata.Now that standalone components are stable, the rule should allow for sorting imports in
Component
metadata as well asNgModule
ssort-ngmodule-metadata-arrays
which should be deprecated in favour of it*(Originally reported here: [sort-ngmodule-metadata-arrays] support standalone component #1133)
Sorting of
imports
does not work when some have.forRoot()
(Originally reported here: @angular-eslint/sort-ngmodule-metadata-arrays : doesn't order module with .forRoot() #927)The rule implementation needs to be completely refactored in order to allow for reliable auto-fixing. I won't cover this in detail as this is done on [sort-ngmodule-metadata-arrays]
--fix
not able to correct all the reports once #675 but essentially the way ESLint auto-fixing works means we cannot do what we do for other rules and simply apply a fix to an item we detect as being in the wrong place. There could easily be more than 10 items in the wrong place, but ESLint would give up at this point.(Originally reported here: [sort-ngmodule-metadata-arrays]
--fix
not able to correct all the reports once #675)In light of this last point, the new rule instead needs to focus on extracting the relevant chunks of a file manually (not using granular AST selectors) and fixing the final form in one action.
This is the approach taken by this import sorting plugin for example: https://github.com/lydell/eslint-plugin-simple-import-sort/blob/main/src/imports.js
*The new rule will be called
@angular-eslint/sort-imports-metadata
The text was updated successfully, but these errors were encountered: