willhoney7 / eslint-plugin-import-helpers

ESLint plugin to help enforce a configurable order for import statements
289 stars 18 forks source link

Importing from parent behaving unexpectedly #14

Open alexlafroscia opened 5 years ago

alexlafroscia commented 5 years ago

I have import-helpers/sort-order configured like so:

    {
        groups: [
          'absolute',
          'module',
          '/^\\$src/',
          'parent',
          'sibling',
          'index'
        ],
        newlinesBetween: 'always',
        alphabetize: { order: 'asc' }
      }

I am seeing that either of these are considered "allowable" by the plugin

import { useCapabilities } from '..';

import { Capabilities, PREFIX } from '../useCapabilities';
import { Capabilities, PREFIX } from '../useCapabilities';

import { useCapabilities } from '..';

But this is not, because there must be a newline between groups

import { Capabilities, PREFIX } from '../useCapabilities';
import { useCapabilities } from '..';

This doesn't seem right to me, since it seems like they are in separate groups, but the groups can be ordered either way. Any idea what might be going on?

willhoney7 commented 5 years ago

Hi there!

Sorry for the late response. I've done some investigating and have determined the issue is with the .. import statement. The rule doesn't know how to classify it (which is a bug). I'm working on it and hope to have a fix out soon.

willhoney7 commented 5 years ago

Hmm, question for you @alexlafroscia:

What would you expect .. to be classified as? What about ../.. or ../../index.ts?

I think by strict definition they should be in the index group... but I don't think it's super valuable to classify these parent directory imports as index. To me, if I'm importing from an index file a few directories up, I want it to be sorted with the other files from that directory, not in a separate group of index files.

I see a few possible solutions:

  1. If the import path ends in a directory (../.. , . , ../) or relative directories and then an index (../../index.ts , ./index), it is classified as index.
  2. If the path is one of: ., ./, ./index, ./index.js (or any line ending), classify as an index. Anything with .. will be classed as relative to parent
  3. Remove the index group and have them all be classified as relative to parent or relative to sibling.

Which solution do you like best? or do you have a different one? I'm leaning towards 3 and releasing this change as v2.

alexlafroscia commented 5 years ago

I think option 3 sounds right to me, too!

On Thu, Jul 4 2019 at 12:03 AM, < notifications@github.com > wrote:

Hmm, question for you @alexlafroscia ( https://github.com/alexlafroscia ) :

What would you expect.. to be classified as? What about../.. or../../index.ts ?

I think by strict definition they should be in the index group... but I don't think it's super valuable to classify these parent directory imports as index. To me, if I'm importing from an index file a few directories up, I want it to be sorted with the other files from that directory, not in a separate group of index files.

I see a few possible solutions:

  • If the import path ends in a directory (../.. ,. ,../ ) or relative directories and then an index (../../index.ts ,./index ), it is classified as index.

  • If the path is one of:. ,./ ,./index ,./index.js (or any line ending), classify as an index. Anything with.. will be classed as relative to parent

  • Remove the index group and have them all be classified as relative to parent or relative to sibling.

Which solution do you like best? or do you have a different one? I'm leaning towards 3 and releasing this change as v2.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub ( https://github.com/Tibfib/eslint-plugin-import-helpers/issues/14?email_source=notifications&email_token=AAMR2OJHEBRQC65QGMIDRQDP5VZCNA5CNFSM4H3DA3ZKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZGH6OQ#issuecomment-508329786 ) , or mute the thread ( https://github.com/notifications/unsubscribe-auth/AAMR2OI4242ZFDWFZLIPPFLP5VZCNANCNFSM4H3DA3ZA ).

willhoney7 commented 5 years ago

@ai, as a vocal user of this rule, do you have any other thoughts on this?

ai commented 5 years ago

I think ../index.js should be classified as parent since it is not different between x/ and x/index.js.

According to the docs, I thought that only ./ and ./index is part of index group.

// 5. "index" of the current directory
import main from './';