Skip to content

Migrating from tslint ordered-imports to import/order #1311

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

Open
mistic opened this issue Mar 29, 2019 · 17 comments
Open

Migrating from tslint ordered-imports to import/order #1311

mistic opened this issue Mar 29, 2019 · 17 comments

Comments

@mistic
Copy link

mistic commented Mar 29, 2019

When migrating from tslint ordered-imports rule import/order is the one recommended here https://github.com/typescript-eslint/typescript-eslint/blob/master/packages/eslint-plugin/ROADMAP.md

I was trying to configure in the following way, which seems to be the most approachable one:

'import/order': ['error', {
    'groups': [
        ['external', 'builtin'],
	[ 'internal', "index", "sibling", "parent" ]
    ],
}],

However there is one option from tslint ordered-imports not covered by that plugin: the ability to have multiple different groups separated by new lines (more info here: https://palantir.github.io/tslint/rules/ordered-imports/)

Are there any plans to support this @benmosher @ljharb ?

@ljharb
Copy link
Member

ljharb commented Mar 30, 2019

Seems like you’d want the groups option to be available on https://github.com/benmosher/eslint-plugin-import/blob/master/docs/rules/newline-after-import.md ?

@mistic
Copy link
Author

mistic commented Apr 1, 2019

@ljharb thanks for the quick feedback!

What I think we need, in order to have a compatible behaviour here, is to be able to apply the required group order defined above, but every time we found a new line between the imports, just considerer it as a new group and then restart the group rank and build starting on the next import (in case it exists). I think the better way is to add a new option for newlines-between called as-a-new-group into the order rule. What do you think? Do u think there is any other option to achieve it right now?

@ljharb
Copy link
Member

ljharb commented Apr 1, 2019

Since groups are configurable I’m not sure how that would work - what I’m envisioning is actually moving the entire groups option to plugin-wide settings, and modifying this rule to respect them.

@mistic
Copy link
Author

mistic commented Apr 1, 2019

@ljharb Before I made the suggestion on the last comment, I had a quick look into the code.

I think in case we wanna implement the option newlines-between: as-a-new-group every time we register a new import node, we can look for the next line and realise if it is a new empty line. In case it is a new empty line we can now consider the next import (if any) as the first import of a new group. All the current logic to calculate the ranks for groups, instead of applying to a data structure of the type Array will now apply to Array<Array>. Also I think there would be no special fix for this newlines-between option as we have for the others because the only fix applicable when this option is enabled is related with the specific order for each group.

What do you think?

@ljharb
Copy link
Member

ljharb commented Apr 1, 2019

I do not think that logic would be good. Groups should be explicit, not inferred by the presence or absence of newlines.

@mistic
Copy link
Author

mistic commented Apr 1, 2019

@ljharb thanks for your thoughts!
You might be right as it should make sense in the big picture of that plugin.

In my perspective, what is import here, is the ability to achieve multiple batches of groups (thats why I was suggesting to extend the logic for new lines). That would be important to assure compatibility for the users migrating from tslint to eslint with typescript-eslint like me.

The current tslint rule defines the following (https://palantir.github.io/tslint/rules/ordered-imports):

  • Named imports must be alphabetized (i.e. “import {A, B, C} from “foo”;”)
    The exact ordering can be controlled by the named-imports-order option.
    “longName as name” imports are ordered by “longName”.
  • Import sources must be alphabetized within groups, i.e.: import * as foo from “a”; import * as bar from “b”;
  • 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.

I can think at least in one use case where this batch of groups logic would make sense: when someone just want to order their imports for example by matter however they still wanna assure that per matter the group order is assured.

Do you think we can think about any way of supporting this using that plugin? 😃

@ljharb
Copy link
Member

ljharb commented Apr 1, 2019

I don’t think it’s useful to allow that kind of haphazard “however you want” grouping, no. I think this plugin’s existing grouping convention is what we need to follow.

@mistic
Copy link
Author

mistic commented Apr 1, 2019

@ljharb thanks again for the feedback. I just think it is not possible for us to use that plugin at all then.
Also, in that case, I think I'm okay closing that issue, though I think other issues about the same matter will be created as soon as others start migrating.

@AdrielLimanthie
Copy link

@ljharb personally I think this is a reasonable feature to add to import/order. People may want to group their imports based on other things, e.g. different groups for React components, utility functions, and vendor libraries.

This could be done easily in tslint by using ordered-imports, but currently not possible with import/order.

@slvganesh
Copy link

@mistic thanks for raising this issue. I'm also running into the same problem. It would be great if we have this feature.

@mistic
Copy link
Author

mistic commented Sep 3, 2019

@slvganesh I just gave up on that issue and also that plugin since last @ljharb answer. It was clear that implementing this feature was not on the path they wanna follow on the library, so that is nothing else we can do here!

@falkenhawk
Copy link

falkenhawk commented Sep 6, 2019

@mistic did you postpone your tslint -> eslint migration due to this issue or did you find another way? (any chance you wrote your own plugin?)

@abierbaum
Copy link

@mistic @falkenhawk This is holding up our tslint -> eslint migration too, so if either of you have found a solution I would love to know about it. I really don't want to have to reimplement the tslint ordered-imports code again especially if it doesn't look like it would be accepted.

@mistic
Copy link
Author

mistic commented Oct 1, 2019

@falkenhawk @abierbaum I did not find any bullet proof solution / workaround other than re-implement the plugin on my own. As we didn't want to dig and use our time for it I just did the most similar thing I could:

'import/order': ['error', {
            'groups': [
              ['external', 'builtin'],
              'internal',
              ['parent', 'sibling', 'index'],
            ],
          }],

Then I just fixed the remaining conflicts and move on with the migration. We just didn't want to loose to much time with that problem, so we just found another different way of doing things.

@sveyret
Copy link
Contributor

sveyret commented Nov 12, 2019

I'm hitting the same problem. I understand the purpose of the plugin, and that I should probably try to modify the way I work on my project, instead of trying to modify the way the plugin works. But I think my use case is worth mentioning: I am actually working on a scoped monorepo project, and I would like all import coming from other packages of the same project to be grouped together, which makes, for example:

import http from 'http';

import { MyClass } from '@my-project/other-package';

import { LocalClass, localFunction } from '../utils';

@valerybugakov
Copy link

valerybugakov commented Jan 28, 2020

@benmosher @ljharb we're facing the same issue with migration from tslint to eslint.
This functionality is very useful in monorepos when you need to split internal group into subgroups by monorepo package.

@ljharb
Copy link
Member

ljharb commented Jan 28, 2020

Try the new pathGroups option.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

9 participants