Ordered imports grouping #4134

abierbaum · 2018-08-25T20:34:13Z

PR checklist

Addresses an existing issue: helps [Rule Suggestion] ordered-imports: Allow for optional new lines between groups. #3450, allows for flexibility to address other custom cases
New enhancement
- Includes tests
Documentation update

Overview of change:

This PR adds support for configuring custom groups with the ordered-import rule.

Previously the rule would only group into third-party, parent directories, and current directory. This change allows for fully customizing the grouping but still keeps a default that provides exactly the same as was there previously.

An example configuration would be something like this:

"rules": {
    "ordered-imports": [
        true,
        {
            "import-sources-order": "case-insensitive",
            "named-imports-order": "case-insensitive",
            "grouped-imports": true,
            "groups": [
                { "match": "^@(rxjs/angular)", "order": 10 },
                { "match": "^app", "order": 20 },
                { "name": "parent_dir", "match": "^[.][.]", "order": 50 },
                { "name": "current dir", "match": "^[.]", "order": 40 },
                { "match": null, "order": 30 }
            ]
        }
    ]
}

This would enforce an ordering where all rxjs and angular imports would come in first group sorted, then all application imports, then all additional third-party imports, then relative parent imports, and finally current directory imports.

This provides the flexibility to support just many use cases and can be adapted easily.

There are other projects such as vscode import-sorter and a few unsupported tslint extensions that try to address this issue but it seems like this is something that would best be handled by tslint since it can already reorder imports.

One of the goals of this change is to allow using the fixer to automatically reformat imports on the fly. So for example when using vscode or another editor with tslint support the user could enable the fixer to be run upon file save and have tslint restructure all imports to meet the given team's conventions.

Is there anything you'd like reviewers to focus on?

I tried to add some solid test examples but I may have missed a corner case. Please let me know if you see something I should check.

I tried to keep the changes to a minimum, but I did refactor some of the existing code to make the grouping implementation a bit more clear and straight forward. I also added some comments to clarify a few places in the code that tripped me up. If you would like these stripped back out just let me know.

Note: There are a number of code style changes that were made by prettier as part of the precommit hook that is now part of tslint. (just so you know I didn't go try to change the original authors coding style :) )

CHANGELOG.md entry:

[enhancement] ordered-imports now supports a groups option to provide custom grouping rules.

palantirtech · 2018-08-25T20:34:14Z

Thanks for your interest in palantir/tslint, @abierbaum! Before we can accept your pull request, you need to sign our contributor license agreement - just visit https://cla.palantir.com/ and follow the instructions. Once you sign, I'll automatically update this pull request.

abierbaum · 2018-08-28T14:57:51Z

@johnwiseheart @giladgray et al: Is there anything more I need to add before review?

giladgray

this looks really good @abierbaum! just a handful of questions, nothing major

giladgray · 2018-08-28T23:58:51Z

src/rules/orderedImportsRule.ts

@@ -42,7 +42,7 @@ export class Rule extends Lint.Rules.AbstractRule {
                    import * as bar from "b";
            - Groups of imports are delineated by blank lines. You can use these to group imports
                however you like, e.g. by first- vs. third-party or thematically or can you can
-                enforce a grouping of third-party, parent directories and the current directory.`,
+                enforce a group order and content.`,


funky language here. "can you can" on line above

I have cleaned up this language:

Groups of imports are delineated by blank lines. You can use this rule to group
imports however you like, e.g. by first- vs. third-party or thematically or
you can define groups based upon patterns in import path names.`

giladgray · 2018-08-28T23:59:55Z

src/rules/orderedImportsRule.ts

+                            match: { type: "string" },
+                            order: { type: "number" }
+                        },
+                        required: ["match", "order"]


can order default to index? so you can simply provide an already-ordered array in the simplest case.

whoa actually the simplest case then would be an array of match strings. so it' could be string[] | Object[]

I updated it to support passing a string[] or an Object[].

src/rules/orderedImportsRule.ts

giladgray · 2018-08-29T00:03:04Z

src/rules/orderedImportsRule.ts

+
+    const defaultGroups: JsonGroupOption[] = [
+        { name: "parent directories", match: "^[.][.]", order: 20 },
+        { name: "current directory", match: "^[.]", order: 30 },


prefer \. over [.] - more obvious

I agree but as far as I can tell that isn't allowed in TS/JS inside a string passed to the RegExp.

For example:

> RegExp('^\.').test('foo') > true > RegExp('^[.]').test('foo') > false

Looks like Javascript will process the "." escape character when assigning the string and just turn it into "." before it get's to the regex.

Other tools I saw that tried to do similar sorting ended up configuring it with [.][.] as well.

ah good point on the string. how about \\.? i guess that's the same length, but [.] relies on the weird truth that . in a character class is treated as the literal . character, not the "match-anything" that it means in other contexts.

giladgray · 2018-08-29T00:04:04Z

src/rules/orderedImportsRule.ts

+
+interface JsonGroupOption {
+    name?: string;
+    match: string | null;


null is not documented above. is it valid in the options?

i might prefer not supporting null and expecting a .* match-everything pattern. bit of an API question there.

I could see that. The reason I allowed match to null originally was an implementation detail that could probably be avoided now. I will change it and see how it works out.

It worked out well so I removed it. I still use an defaultGroup object internally for unmatched values, but now I use match of /.*/ there.

giladgray · 2018-08-29T00:04:59Z

src/rules/orderedImportsRule.ts

    const {
        "grouped-imports": isGrouped = false,
        "import-sources-order": sources = "case-insensitive",
        "named-imports-order": named = "case-insensitive",
        "module-source-path": path = "full",
-    } = optionSet === undefined ? {} : optionSet;
+        groups: groups = defaultGroups


no need for : groups since the name is the same. please remove

done. good catch.

giladgray · 2018-08-29T00:06:38Z

src/rules/orderedImportsRule.ts

@@ -299,49 +403,90 @@ class Walker extends Lint.AbstractWalker<Options> {
        }
    }

+    /**
+     * Check all import blogs stopping at the first failure.


import blocks?

abierbaum · 2018-08-29T03:00:10Z

@giladgray I think I addressed all your comments and also added support for the items you suggested. Thanks for taking the time to review this PR. Please let me know if you see anything else.

giladgray · 2018-08-29T21:07:11Z

@abierbaum please do not amend and force push commits that have already been reviewed. it ruins the diff and makes it hard to incrementally review. we squash everything to one commit when merging anyways, so the default branch is always clean.

abierbaum · 2018-08-29T22:23:12Z

@giladgray no problem. Sorry about that, I didn’t know what convention was for this project.

giladgray · 2018-08-29T22:35:10Z

destroying history shouldn't be part of your default workflow 😄

abierbaum · 2018-08-30T20:01:03Z

@giladgray Any more changes need or is this good to go?

giladgray · 2018-08-30T21:04:07Z

i would prefer to use \\. in the regular expressions -- less suggestive

abierbaum · 2018-08-31T13:19:12Z

@giladgray Good idea. I made the changes. You will see in the diff I also added a space in the failure message to get separation between the last pattern name and the ending period of the sentence. This was to help make the message less confusing in cases like:

Import sources of different groups must be sorted by: ^app, ^@pkg, ^\.\., ^\. .

Let me know if you see anything else.

Thanks for reviewing so quickly.

abierbaum · 2018-09-05T11:32:58Z

@giladgray Anything else I can do to help get this finished off and merged?

abierbaum · 2018-09-12T15:53:41Z

@giladgray Just checking in again on status of this PR. Our team is excited to get using it after it gets merged.

abierbaum · 2018-10-02T13:18:44Z

@giladgray We have been using this now for a couple weeks in production with no issues. Do you think this could get merged in so it can make the next release?

AlexNisnevich · 2018-10-13T01:07:36Z

Any progress on getting this PR merged? It would really help my team's tslint workflow.

abierbaum · 2018-10-15T16:45:07Z

I don't know of anything left to do. We have been using it in production with a custom build of tslint for a month. I would like to get back to mainline though. Just waiting to hear from @giladgray or @johnwiseheart on acceptability of the PR.

abierbaum · 2018-11-05T13:45:24Z

@giladgray and @johnwiseheart . Just checking in again on the status of this PR. If this can't land into tslint, let me know so I can put it into a separate extension project like vrsource-tslint-rules

abierbaum · 2018-11-07T20:03:55Z

@JoshuaKGoldberg I just fixed up the branch on top of the latest tslint:master. I had to rerun prettier to remove some merge conflicts. Anything else you need?

JoshuaKGoldberg · 2018-11-07T20:09:04Z

@abierbaum great, nothing else from me!

Other TSLint maintainers (read: with more experience maintaining) have been swamped lately so there's work going on trying to find more folks. I'm holding off from merging larger PRs for a while to give them a chance to either find a full-time maintainer or get to this themselves.

I know that's not a particularly satisfying answer, and neither is the suggested workaround of publishing this a a standalone rule in your own npm package. But expect more soon!

abierbaum · 2018-11-07T20:11:13Z

@JoshuaKGoldberg Ok. Thanks for the update. I know there are a number of us hoping to get some of these larger improvements through so we can start using them more widely in production.

- Change the message shown when items are not grouped as expected The new message is more clear and understandable - Use a better default group name If a group has a regex and no name, the default name is now: /regex/ This makes it more clear in the output that it is a regex and helps to keep the boundaries of the regex string more clear.

This check is already handled by the check for groups needing to be sorted. The idea is that once the first block of imports is found that has an import that is out of order, then all future blocks also have blocks out of order. So instead of flagging all blocks we just flag the first block that is causing the issue and provide a fix that will fix all blocks. This makes the case for manually fixing the import ordering much more clear because people can solve one item at a time. The automattic fixing is also more clear because we don't end up with multiple failures that have the same fix.

abierbaum · 2019-01-26T13:59:18Z

@adidahiya I think I am ready for you to take a look at the changes I made in response to your review. Thank you for the great comments.

As far as running this change on the tslint codebase, I am more than happy to do that but I must admit I don't know how to run tslint with fix across the tslint codebase. If you can point me in the right direction I can submit a PR after this change gets through.

adidahiya · 2019-01-29T20:08:59Z

@abierbaum the lint:from-bin NPM script (also run as part of lint) will run the current tslint source to lint itself, using the root tslint.json file. So ordered-imports is already being run that way, but tslint:all (src/configs/all.ts) doesn't currently have the grouped imports option enabled -- that's what you'd have to enable.

abierbaum · 2019-01-29T21:28:04Z

@adidahiya Looks like you did the lint fix update already on master (95d9d95). Thanks for getting that done and thank you so much for the review. I hope the new code ends up useful for everyone.

adidahiya · 2019-01-29T22:06:51Z

@abierbaum yeah, I realized that the open PR (#4420) had no conflicts so it was ready to go as-is. thanks for the PR

jukben · 2019-01-29T22:30:05Z

Thanks for this, it's awesome work. Do we know a date when this gonna be deployed? 🚢

scote · 2019-01-31T14:16:37Z

Do you know when it gonna be release?

* master: (60 commits) Added tslint-brunch to the list of 3rd party tools (palantir#4251) Switch to tslint-plugin-prettier, clean up rule options config syntax (palantir#4488) Enable grouped-imports for ordered-imports rule in tslint:all config (palantir#4420) Ordered imports grouping (palantir#4134) trailing-comma: check for a closing parenthesis (palantir#4457) Update index.md (palantir#4473) [bugfix] `no-unsafe-any`: allow implicitly downcasting `any` to `unknown` (palantir#4442) Add v5.12.1 changelog Bump version to 5.12.1 Fix quotemark avoid-template issues (palantir#4408) Skip linting JSON files entirely (palantir#4001) Fix strict-type-predicate for unknown (palantir#4444) restrict increment-decrement fixer while fixing the postfix unary expressions (palantir#4415) Mention file names in script parse failures (palantir#4397) Revert breaking change to tslint:recommended, update tslint:latest (palantir#4404) Fix quotemark avoid-template issues (palantir#4408) Bump tslint dev dependency to 5.12.0 (palantir#4452) Skip linting JSON files entirely (palantir#4001) Fix strict-type-predicate for unknown (palantir#4444) [README] Update link for Webstorm (palantir#4450) ...

abierbaum · 2019-02-05T20:10:56Z

@jukben @scote Thanks for the feedback. I am very interested in a release as well. Our team is currently running an old version of tslint from a branch with this change. I am guessing @adidahiya or @JoshuaKGoldberg are the right people to ask about next release timeframes.

jukben · 2019-02-05T21:29:54Z

@abierbaum #4482 (comment) I just got this response from @adidahiya to my PR so I guess these two will go together pretty soonish! 🙌

robertu7 · 2019-02-16T06:02:39Z

cannot wait, please release it! @adidahiya 🌼

megabayt · 2019-02-20T19:25:28Z

Waiting for release

abierbaum · 2019-02-22T13:34:56Z

@adidahiya Any news on a release date or is there a way to use latest HEAD from npm so people can start using this update?

jukben · 2019-02-22T13:42:15Z

[OT] @abierbaum yep, you can use git repo url with NPM, Yarn. But I'd like to see release as well. Although I'm afraid it's better to explore ways how to lint TS with ESLint, which going to be a more preferred way in the future.

abierbaum · 2019-02-22T15:51:54Z

@jukben I am hoping they do another release before holding up for moving to ESLint. I don't have the time right now to try to learn a new system and port the rules our team relies up. :(

As far as using git repo url, I haven't found a way to do that with tslint. When I have tried the packages are not built correctly for use. Have you had luck doing that? If so I would love to know how.

jukben · 2019-02-22T17:00:22Z

Sorry @abierbaum you are right, there has to be build process, my bad. Yeah, I hope for the same, I'm waiting for release as well. I might end up forking this...

Koslun · 2019-02-23T13:06:23Z

Concerning the TSLint to ESLint migration I'd like to add that ESLint still doesn't support some of the concepts present in TSLint and that many rules are still missing, including external ones. So even you have the time it wouldn't make sense to completely migrate over to ESLint now as you'd potentially lose many of the rules in-use.

An example is the Angular linting package Codelyzer which is still in TSLint and will only be ported once some TSLint features are merged into ESLint. So given that it may take at least a few months to get ESLint all the missing features of TSLint it will then at least take some more months for the rest of the linting packages like Codelyzer to also migrate to ESLint.

So would be nice to just have this one final release before the inevitable long wait before we are able to actually migrate to ESLint without having regressions.

adidahiya · 2019-02-23T18:24:50Z

We are still committed to supporting TSLint for a few months and will continue to do releases to push out PRs like this. "Migrating to ESLint without having any regressions" is of concern to the TSLint community and all of Palantir's TypeScript codebases as well; we want to make that process as smooth as possible. I started this new issue thread to answer questions and clear up doubts about the deprecation path / migration path to ESLint: #4534. Thanks for your patience as I work to get this PR released.

adidahiya · 2019-02-24T00:24:42Z

I just released v5.13.0, please try it out and open new issues if you have problems with this feature. thanks again @abierbaum!

JWess · 2019-03-27T19:28:35Z

@abierbaum Awesome job! I have a couple questions:

Is there any way to target imports with no "from" such as:

import 'zone.js/dist/zone-testing';

I would like to put them in their own group.

Is there any way to specify a max line length for imports so that they will wrap?

Before:

import { ExtendSessionDialogComponent, SecondImport } from './core/components/extend-session-dialog/extend-session-dialog.component';

After:

import { 
  ExtendSessionDialogComponent, SecondImport
} from './core/components/extend-session-dialog/extend-session-dialog.component';

adidahiya · 2019-03-27T20:33:39Z

@JWess please start a new issue for additional features / enhancements to this rule. for (2), that's the job a formatter and such functionality won't be added to tslint at this point (it's not easy, I recommend using prettier instead, see #4534)

gogoyqj · 2019-05-29T04:13:59Z

@abierbaum it'll be very useful if those features supported in typescript-eslint

abierbaum force-pushed the ordered_imports_grouping branch 3 times, most recently from 7cc27f4 to daab755 Compare August 26, 2018 17:12

giladgray reviewed Aug 29, 2018

View reviewed changes

abierbaum force-pushed the ordered_imports_grouping branch from b19b7d2 to 1d560b9 Compare August 29, 2018 02:55

abierbaum force-pushed the ordered_imports_grouping branch from 1d560b9 to c92e44c Compare August 29, 2018 11:50

JoshuaKGoldberg added the PR: merge target branch label Nov 7, 2018

JoshuaKGoldberg approved these changes Nov 7, 2018

View reviewed changes

JoshuaKGoldberg added the PR: waiting for reviewer label Nov 7, 2018

abierbaum force-pushed the ordered_imports_grouping branch from ebc0fa3 to 724d46d Compare November 7, 2018 20:01

JoshuaKGoldberg removed the PR: merge target branch label Nov 7, 2018

AlexNisnevich mentioned this pull request Nov 13, 2018

Ordered imports grouping semanticmachines/tslint#1

Merged

abierbaum added 2 commits January 26, 2019 07:02

adidahiya approved these changes Jan 29, 2019

View reviewed changes

adidahiya removed the PR: waiting for reviewer label Jan 29, 2019

adidahiya merged commit 5476f15 into palantir:master Jan 29, 2019

adidahiya mentioned this pull request Jan 29, 2019

Add grouped imports by default in tslint:all #4420

Merged

abierbaum added a commit to abierbaum/tslint that referenced this pull request Jan 29, 2019

Ordered imports grouping (palantir#4134)

0143484

snyk-bot mentioned this pull request Oct 11, 2019

[Snyk] Upgrade tslint from 5.11.0 to 5.20.0 Narshe1412/nuarca-sql#21

Merged

snyk-bot mentioned this pull request Oct 26, 2019

[Snyk] Upgrade tslint from 5.12.1 to 5.20.0 ajesse11x/caprover#5

Open

Ordered imports grouping #4134

Ordered imports grouping #4134

Conversation

abierbaum commented Aug 25, 2018 • edited

PR checklist

Overview of change:

Is there anything you'd like reviewers to focus on?

CHANGELOG.md entry:

palantirtech commented Aug 25, 2018

abierbaum commented Aug 28, 2018

giladgray left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

abierbaum commented Aug 29, 2018

giladgray commented Aug 29, 2018

abierbaum commented Aug 29, 2018

giladgray commented Aug 29, 2018

abierbaum commented Aug 30, 2018

giladgray commented Aug 30, 2018

abierbaum commented Aug 31, 2018

abierbaum commented Sep 5, 2018

abierbaum commented Sep 12, 2018

abierbaum commented Oct 2, 2018

AlexNisnevich commented Oct 13, 2018

abierbaum commented Oct 15, 2018

abierbaum commented Nov 5, 2018

abierbaum commented Nov 7, 2018

JoshuaKGoldberg commented Nov 7, 2018

abierbaum commented Nov 7, 2018

abierbaum commented Jan 26, 2019

adidahiya commented Jan 29, 2019

abierbaum commented Jan 29, 2019

adidahiya commented Jan 29, 2019

jukben commented Jan 29, 2019

scote commented Jan 31, 2019

abierbaum commented Feb 5, 2019

jukben commented Feb 5, 2019

robertu7 commented Feb 16, 2019 • edited

megabayt commented Feb 20, 2019

abierbaum commented Feb 22, 2019

jukben commented Feb 22, 2019

abierbaum commented Feb 22, 2019

jukben commented Feb 22, 2019

Koslun commented Feb 23, 2019

adidahiya commented Feb 23, 2019

adidahiya commented Feb 24, 2019

JWess commented Mar 27, 2019 • edited

adidahiya commented Mar 27, 2019

gogoyqj commented May 29, 2019 • edited

abierbaum commented Aug 25, 2018 •

edited

robertu7 commented Feb 16, 2019 •

edited

JWess commented Mar 27, 2019 •

edited

gogoyqj commented May 29, 2019 •

edited