Automatic import sorting

[olemartinorg]

Description

I've noticed that people and editors sometimes write different things, and the import block in the beginning of our files gets cluttered quickly. If it only gets appended to it might cause annoying merge conflicts, and when there are multiple paths leading to the same file, some lines might swap back and forth between multiple variants. I've been solving annoyances like these with automatic sorting and rewriting of import paths before, so this PR is meant as a proof-of-concept to check if developers agree this is a valid path going forwards.

Because no plugin apparently does all of this (I almost gave up searching), I ended up with a combination of three eslint plugins:

• First, eslint-plugin-no-relative-import-paths rewrites imports to all be absolute (from the project root)
• Then eslint-plugin-preferred-import-path takes over to rewrite these absolute paths to paths that follow the path roots defined in our tsconfig.json (with src and altinn-shared)
• Lastly, eslint-plugin-simple-import-sort sorts these imports (using both the 'from' value to sort into logical groups, and sorting imported symbols when importing multiple from one file.

Considerations

I've added a few changes to 3 files, to show representative examples from complex files (and less complex files) to give an indication of what this does to our imports.

• Do we want this at all?
  • Yes: @haakemon @olemartinorg
  • No:
• Should we also rewrite sibling imports/relative imports from the same folder?
  • Yes (all absolute paths): @haakemon @olemartinorg
  • No (some relative paths):
• Get rid of grouping with and keep all type imports in one big block:
  • Yes (one big block):
  • No (keep groups): @haakemon @olemartinorg
• Type imports:
  • Group these together in one big type-group:
  • Have them grouped alongside their respective groups (internal/external, etc): @haakemon @olemartinorg

Verification

• [x] Your code builds clean without any errors or warnings
• [x] Manual testing done (required)
• [ ] Relevant automated test added (if you find this hard, leave it and we'll help out)
• [x] All tests run green

Documentation

• [ ] User documentation is updated with a separate linked PR in altinn-studio-docs. (if applicable)

[haakemon]

I think this is a great idea and something like this would solve the same thing prettier does - less noise in PR's, and thats a big win in my book 😁

Did something similar for design-system repo: https://github.com/Altinn/altinn-design-system/blob/main/.eslintrc.js#L25-L33

Perhaps we should extract these rules (and other common eslint rules) to a separate npm package, that all our projects can extend? Then all projects would be in sync with the same rules. That would be really great IMO. (same thing can be done with prettier and probably other configs).

My thoughts:

• I think all imports can be resolved from root/aliases defined in tsconfig.
  • For sibling imports, both relative and root/alias is fine by me, but perhaps easier to just stick with the same rules for all imports? If its all handled automatically anyway on commit it doesnt really matter since it will be fixed for me even if I write a relative import.
• Personally I like grouping them with an empty line between. Not important that React is first IMO, but I think that will be done by default? Seems to be the case at least in design system repo which has a bit simpler config.
• Type imports could go in the same group as they belong to IMO.

[olemartinorg]

Oh! I haven't been doing any development in the design system yet, so I didn't know you already had a rule in place there. I tried import/order at first, but found it a bit lacking in some ways:

• It did not respect existing comments, so some automatic fixes (especially in validation.ts) had their comments disjointed by the rewrite.
• It doesn't sort imported symbols (or at least I don't think so, I think I've just read it, but never tested it)
• It's less flexible. If we don't have a need for that flexibility, that's just a bonus.

Of course, we already have it, so it's always nice not having to pull another dependency. But it still doesn't solve the problem of importing from absolute paths using tsconfig.json, and even though eslint-plugin-simple-import-sort is a pretty big cannon with its complex regex rules, I preferred those results over import/order.

Other than that, I agree with you on all points. I don't see the need for any relative path imports at all, if they can be rewritten automatically. It also seems the plugins I pulled doesn't figure out that '..' (implicitly '../index.ts') is a non-sibling relative import - weird. If a file is moved to another location, all-absolute imports would make the diff cleaner.

[olemartinorg]

@haakemon I tested with the simpler import/order rule, and while I can see it working in the design system, it wasn't all that great here.

1. It doesn't know the difference between a regular package, our 'src/' and altinn-shared, so none of the grouping functoinality there seems to work.
2. The above could in theory be fixed using the pathGroups configuration, but according to the docs it 'will not be used for builtins or externals' :roll_eyes:
3. As mentioned earlier, it doesn't sort imported symbols

In effect, it ended up roughly sorting imports, but I saw src/ imports mixed in between external imports, and it all looked like a mess.

[sonarcloud[bot]]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
4 Code Smells

93.2% Coverage
0.0% Duplication

[haakemon]

@haakemon I tested with the simpler import/order rule, and while I can see it working in the design system, it wasn't all that great here.

1. It doesn't know the difference between a regular package, our 'src/' and altinn-shared, so none of the grouping functoinality there seems to work.
2. The above could in theory be fixed using the pathGroups configuration, but according to the docs it 'will not be used for builtins or externals' 🙄
3. As mentioned earlier, it doesn't sort imported symbols

In effect, it ended up roughly sorting imports, but I saw src/ imports mixed in between external imports, and it all looked like a mess.

I dont think it matters that much about grouping "altinn-shared" in a separate group - that stuff is still internal, and ideally we should get rid of that entirely anyway.

If we cant make it work with a generic plugin (or config in a generic way) thats fine of course - but it would have been great if this config could be generic enough so that we could share it across repos.

Most important IMO is to get this automated, so it doesnt cause extra noise in PR's etc...

[olemartinorg]

It was a bit unclear in my comment, but I consider import/order to be close to useless here. :wink: The result of that 'sort' mixed all kinds of different imports together in a big block, and for some reason lines with an import from react got sorted in-between two lines importing from src/. Without symbol sorting it also fails to mitigate merge conflicts from files we import many different symbols from.

This is the import block from validation.ts sorted with import/order:

import {
  getLanguageFromKey,
  getParsedLanguageFromKey,
} from 'altinn-shared/utils';
import moment from 'moment';
import type { Options } from 'ajv';
import Ajv from 'ajv';
import type * as AjvCore from 'ajv/dist/core';
import Ajv2020 from 'ajv/dist/2020';
import dot from 'dot-object';
import addFormats from 'ajv-formats';
import type {
  IComponentValidations,
  IValidations,
  IComponentBindingValidation,
  ITextResource,
  IValidationResult,
  ISchemaValidator,
  IRepeatingGroups,
  ILayoutValidations,
  IDataModelBindings,
  IRuntimeState,
  ITextResourceBindings,
  IValidationIssue,
  DateFlags,
} from 'src/types';
import JsonPointer from 'jsonpointer';
import type {
  IAttachment,
  IAttachments,
} from 'src/shared/resources/attachments';
import type { ILanguage } from 'altinn-shared/types';
import { AsciiUnitSeparator } from 'src/utils/attachment';
import type { ReactNode } from 'react';
import type { IFormData } from 'src/features/form/data';
import type {
  ILayouts,
  ILayoutComponent,
  ILayoutGroup,
  ILayout,
} from 'src/features/form/layout';
import { Severity } from 'src/types';
import {
  getFieldName,
  getFormDataForComponent,
} from 'src/utils/formComponentUtils';
import { getParsedTextResourceByKey } from 'src/utils/textResource';
import {
  convertDataBindingToModel,
  getFormDataFromFieldKey,
  getKeyWithoutIndex,
} from 'src/utils/databindings';
import { matchLayoutComponent, setupGroupComponents } from 'src/utils/layout';
import {
  createRepeatingGroupComponents,
  getRepeatingGroupStartStopIndex,
  splitDashedKey,
} from 'src/utils/formLayout';
import { getDataTaskDataTypeId } from 'src/utils/appMetadata';
import { getFlagBasedDate } from 'src/utils/dateHelpers';

Compare that to the one in this branch.

[haakemon]

Oh, I see what you mean now :) I got a slightly different result in my very limited testing, but thats probably because I dont have the rewritten import paths. Seems with the rewritten paths, everything ends up in the same group since it doesnt know about the alias paths. When using import/order, you would also have to add manual grouping to get this to work with aliases, f.ex

      "pathGroups": [
          {
            "pattern": "altinn-shared/**/*",
            "group": "internal"
          },
          {
            "pattern": "src/**/*",
            "group": "internal"
          }
        ],

Another option would be to prefix path imports with a special character so it doesnt "appear to be external" (https://github.com/lydell/eslint-plugin-simple-import-sort#custom-grouping), like "~/" or "@/". That way, we shouldnt need to add any custom config for grouping it seems. Something to try out perhaps?

I think both plugins have this feature, at least it seems to work that way in altinn-design-system repo (where aliases are setup with @/) so I dont think it matters much which one we end up with.

[olemartinorg]

Yep, I mentioned the pathGroups option in a comment above, but as these imports are assumed to be external at first, pathGroups has no effect on these import paths.

Prefixing could work, but then we'd need to enforce that as well (and make sure our resolver can handle such imports, if not handled already?). At that point, I think using eslint-plugin-simple-import-sort is the better compromise - even if requires pulling an extra dependency.

[haakemon]

Yep, if we were to prefix the imports, we would also need to update the different resolvers to handle the new alias path. As far as I can remember its defined in 3 places; webpack, jest and tsconfig.

I'm not too worried about pulling an extra dependency (as long as the dependency is "actively maintained", and this seems like it is). Trying to keep the config simple is more important IMO (but not always feasible).

My initial thought was also to use a generic config (by using default f.ex) so we could easily to extract as a shared config - but maybe its no point. You would either need to rely on conventions (f.ex all consumers would also need to prefix aliases with @/, and put code in /src, etc..), and we might quickly get into problems where this doesnt fit with existing solutions and would just be more painful than its worth. So creating a shared eslint config still seems like a good idea to me, but import order would probably not be a part of it - but it could be recommended as something to add manually with some nice recipes 😅

To sum up, I think the end result here is great and gives value. Lets bring it in and reevaluate if we should modify it after we get some experience with it 🙂 Good thing to get it in now while there are so few other things in motion.