Improving/replacing sort-ngmodule-metadata-arrays

[JamesHenry]

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 as NgModules

  • This is an important point and almost certainly means that a new rule should be created to replace sort-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

[abaran30]

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.

[rubiesonthesky]

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

[Nosfistis]

Any news on this rule?

[abaran30]

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 HttpClientInMemoryWebApiModule after the HttpClientModule to ensure that the in-memory backend provider supersedes the Angular version." Sorting the imports would break this.

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 AppRoutingModule does not get imported after HeroesModules, routing breaks; reproduced in the live example.

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.

[Nosfistis]

Could sorting be ignored if there is an inline ignore rule?

[abaran30]

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 eslint-disable comments. One could consider a new commenting system specific to a rule, but I would be curious to learn about the use case(s) that would warrant such complexity.