Skip to content
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

[sort-ngmodule-metadata-arrays] suggestion - sort by package name #772

Open
ghost opened this issue Oct 28, 2021 · 2 comments
Open

[sort-ngmodule-metadata-arrays] suggestion - sort by package name #772

ghost opened this issue Oct 28, 2021 · 2 comments
Labels
blocked:issue This issue requires another issue on @angular-eslint to be resolved first enhancement New feature or request package: eslint-plugin Angular-specific TypeScript rules

Comments

@ghost
Copy link

ghost commented Oct 28, 2021

Hi! I use prettier with the prettier-plugin-organize-imports plugin. It sorts the imports alphabetically.

Let's say I have a module:

import { CommonModule } from '@angular/common';
import { NgModule } from '@angular/core';
import { ReactiveFormsModule } from '@angular/forms';
import { MatButtonModule } from '@angular/material/button';
import { MatCardModule } from '@angular/material/card';
import { MatInputModule } from '@angular/material/input';
import { MatSelectModule } from '@angular/material/select';
import { TranslocoModule } from '@ngneat/transloco';
import { AlertModule } from '../../shared/components/alert';
import { ControlErrorsModule } from '../../shared/components/control-errors';
import { VersionModule } from '../../shared/components/version';
import { LoginPageRoutingModule } from './login-page-routing.module';
import { LoginPageComponent } from './login-page.component';

@NgModule({
  imports: [
    CommonModule,
    ReactiveFormsModule,
    MatButtonModule,
    MatCardModule,
    MatInputModule,
    MatSelectModule,
    TranslocoModule,
    AlertModule,
    ControlErrorsModule,
    VersionModule,
    LoginPageRoutingModule,
  ],
  declarations: [LoginPageComponent],
  exports: [LoginPageComponent],
})
export class LoginPageModule {}

Notice that the modules are sorted alphabetically and items in the imports array have the same order. It looks nice and consistent. 3rd party modules go first, then modules from other directories, then modules from the same folder. But I have to maintain the order manually and sadly my colleagues might not care to do that.

Now, if I enable the sort-ngmodule-metadata-arrays to force the order of items, the code will look like this:

import { CommonModule } from '@angular/common';
import { NgModule } from '@angular/core';
import { ReactiveFormsModule } from '@angular/forms';
import { MatButtonModule } from '@angular/material/button';
import { MatCardModule } from '@angular/material/card';
import { MatInputModule } from '@angular/material/input';
import { MatSelectModule } from '@angular/material/select';
import { TranslocoModule } from '@ngneat/transloco';
import { AlertModule } from '../../shared/components/alert';
import { ControlErrorsModule } from '../../shared/components/control-errors';
import { VersionModule } from '../../shared/components/version';
import { LoginPageRoutingModule } from './login-page-routing.module';
import { LoginPageComponent } from './login-page.component';

@NgModule({
  imports: [
    AlertModule,
    CommonModule,
    ControlErrorsModule,
    LoginPageRoutingModule,
    MatButtonModule,
    MatCardModule,
    MatInputModule,
    MatSelectModule,
    ReactiveFormsModule,
    TranslocoModule,
    VersionModule,
  ],
  declarations: [LoginPageComponent],
  exports: [LoginPageComponent],
})
export class LoginPageModule {}

Now the order of items in the imports array does not match the order of imports. Now 3rd party and local modules are mixed. In my opinion, such order is not good for visual scanning, despite being in the alphabetical order.

Please consider having an option to sort items in the imports array by package name, if that's possible of course. Thanks!

@ghost ghost added package: eslint-plugin Angular-specific TypeScript rules triage This issue needs to be looked at and categorized by a maintainer labels Oct 28, 2021
@rubiesonthesky
Copy link
Contributor

I would suggest that the sort check would only happen in "groups" that can be separated by new line or row containing comment. This would not be sufficient for original poster's needs but it would make sense in regarding Angular Style guide.

In Angular style guide all examples are separating "external" and "internal" imports by new line - so it would make sense to me to have same kind of grouping also in metadata array sorting.

I think this would not need additional options to the rule but would just allow having manual groups in metadata array.

So lending above example, this would be valid

@NgModule({
  imports: [
    CommonModule,
    MatButtonModule,
    MatCardModule,
    MatInputModule,
    MatSelectModule,
    ReactiveFormsModule,
    TranslocoModule,

    AlertModule,
    ControlErrorsModule,
    LoginPageRoutingModule,
    VersionModule,
  ],
  declarations: [LoginPageComponent],
  exports: [LoginPageComponent],
})
export class LoginPageModule {}

and

@NgModule({
  imports: [
    CommonModule,
    MatButtonModule,
    MatCardModule,
    MatInputModule,
    MatSelectModule,
    ReactiveFormsModule,
    TranslocoModule,
    // app
    AlertModule,
    ControlErrorsModule,
    LoginPageRoutingModule,
    VersionModule,
  ],
  declarations: [LoginPageComponent],
  exports: [LoginPageComponent],
})
export class LoginPageModule {}

@JamesHenry
Copy link
Member

I understand the desire for this enhancement. For now, the rule has a lot of problems and those need to be tackled first. That work will be done to address #1232 please subscribe to that for updates.

Once the new rule lands and is proven to be a reliable replacement, we can look at how the enhancement discussed here could be applied

@JamesHenry JamesHenry added enhancement New feature or request blocked:issue This issue requires another issue on @angular-eslint to be resolved first and removed triage This issue needs to be looked at and categorized by a maintainer labels Nov 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocked:issue This issue requires another issue on @angular-eslint to be resolved first enhancement New feature or request package: eslint-plugin Angular-specific TypeScript rules
Projects
None yet
Development

No branches or pull requests

2 participants