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

import/order pathGroups not enforcing as expected #1565

Closed
osdiab opened this issue Dec 10, 2019 · 4 comments · Fixed by #1570
Closed

import/order pathGroups not enforcing as expected #1565

osdiab opened this issue Dec 10, 2019 · 4 comments · Fixed by #1570

Comments

@osdiab
Copy link

osdiab commented Dec 10, 2019

Sample repo: https://github.com/osdiab/eslint-plugin-import-order-pathGroups

I was wondering if I was misusing this new flag on the rule.

With this this code, I want it to behave as so:

// BEFORE FIX
import { testHelper } from "__tests__/helpers";
import { RecurringStatus } from "@my.company/common/es6/entity/types";
import { ChartistGraph } from "@app/components/common/ChartistGraph";
import { ILineChartOptions } from "chartist";

// AFTER FIX
// this is external, so before all this package's imports
import { ILineChartOptions } from "chartist";

// this is external, but monorepo package, so put it after normal external
import { RecurringStatus } from "@my.company/common/es6/entity/types";

// this is internal, so mixed with anything else internal
import { ChartistGraph } from "@app/components/common/ChartistGraph";

// this is internal but secondary, so after other internal code
import { testHelper } from "__tests__/helpers";

I configured my import/order like so:

        "pathGroups": [
          {
            "pattern": "@app/**",
            "group": "internal"
          },
          {
            "pattern": "__tests__/**",
            "group": "internal",
            "position": "after"
          },
          {
            "pattern": "@my.company/**",
            "group": "external",
            "position": "after"
          }
        ],

But this doesn't mark my BEFORE FIX code as an error at all :P not sure why. I'll try to make a quick repo to demonstrate it now!

@Mairu
Copy link
Contributor

Mairu commented Dec 13, 2019

So I debugged it, the problem is that I wrote it that way that path groups are not used for builtin and external imports, but your imports are all recognized as external, which is kind of wrong.

The question would now be how to fix this problem, I see following possibilities:

  1. the simplest one would be to allow pathGroups also for external imports
  2. define (or recognize) aliases somehow to know what is not an external import even if it looks like one
  3. make it configurable which import types can be sorted by pathGroups

@hcharley
Copy link

hcharley commented Dec 31, 2019

I think this would be a good change to make. Would be nice to keep my org's imports seperate from externals.

@osdiab
Copy link
Author

osdiab commented Dec 31, 2019

I think approach 1 seems fine, I don’t see any reason path groups ought to behave differently for external - I initially intuitively thought of it as an override on the original group value, which if I understand correctly is how it works for stuff categorized into other groups.

And separately I also think it would make sense to allow for arbitrary groups (it would be nice if I could explicitly name this class of path as it’s own group and specify where to order it relative to the existing default categories instead of trying to overload my use case into existing categories). If the order for a custom path not specified, can either error or default to the end or something with a helpful reason for the position in the lint error. But that can be a separate extension.

@Mairu
Copy link
Contributor

Mairu commented Jan 1, 2020

It was not wanted (by the maintainers) to add additional groups, that is the reason why I implemented it this way.

Most people wanted to sort their aliases, but I see the point to sort your company packages. However, in my opinion, this is also the only sensible use case to sort external groups.

I implemented option 3 because option 1 would be a BC-breaking.

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