v2.23 breaking group spacing/ordering

[lensbart]

The following comes up as an error since upgrading to 2.23:

// ESLint error due to the empty line between two import groups
import { UserInputError } from 'apollo-server-express'

import { new as assertNewEmail } from 'Assertions/Email'

// .eslintrc.json (abridged)
{
  "plugins": [
    "import",
  ],
  "rules": {
    "import/order": [
      "error",
      {
        "alphabetize": {
          "caseInsensitive": true,
          "order": "asc"
        },
        "groups": [
          "builtin",
          "external",
          "internal",
          "parent",
          "sibling",
          "index"
        ],
        "newlines-between": "always"
      }
    ],
  },
  "settings": {
    "import/resolver": {
      "babel-module": {},
      "webpack": {}
    }
  }
}

The codebase I’m working on uses the convention that aliases start with a capital letter (i.e. the second import statement belongs to the internal group). There should be a space between import groups. The error goes away when removing the empty line between both import statements.

[ljharb]

@lensbart so there's a few issues here. npm packages can, in fact, start with a capital letter - just not new ones. So, this is a really risky pattern to use for internal modules (see https://www.npmjs.com/package/JSONStream vs https://www.npmjs.com/package/jsonstream).

Still, when i add this test case (but use pathGroups to force it to be marked as internal, it does fail, so there's something here.

[ljharb]

When I modify the test case to use an actually invalid package name, everything passes.

This suggests that this was in fact a bug fix, and the warning is correct.

[ljharb]

(Additionally, you probably want node: {} to be the third item in your import/resolver settings)

[lensbart]

@lensbart so there's a few issues here. npm packages can, in fact, start with a capital letter - just not new ones. So, this is a really risky pattern to use for internal modules (see https://www.npmjs.com/package/JSONStream vs https://www.npmjs.com/package/jsonstream).

Still, when i add this test case (but use pathGroups to force it to be marked as internal, it does fail, so there's something here.

Thanks for this insight. In fact, the codebase doesn’t use a rule or pattern to disambiguate internal/external packages. It’s just a convention, all aliases are listed in tsconfig.json’s compilerOptions.paths.

[ljharb]

@lensbart then you'd need the typescript resolver to kick those in; your config only has a babel and webpack one.

[lensbart]

@lensbart then you'd need the typescript resolver to kick those in; your config only has a babel and webpack one.

Whoops, sorry again for the confusion that I’m causing on this Friday evening. In order to avoid repetition, I define my paths once in tsconfig.json, and import tsconfig.json in my .babelrc.js and use the paths defined in the former.

[ljharb]

ahh ok, that does clear that up :-)

are you saying that in v2.22, your tsconfig paths were recognized as "internal", but in v2.23, they're recognized as "external"?

[lensbart]

are you saying that in v2.22, your tsconfig paths were recognized as "internal", but in v2.23, they're recognized as "external"?

Yep!

Not sure if I should be proud or embarrassed of this pattern, but my babel config looks as follows (abridged):

// .babelrc.js
const { readFileSync } = require('fs')
const { resolve } = require('path')

const { parse } = require('json5')
const { filter, map } = require('ramda')

// Get module alias names from TypeScript configuration file
const alias = filter(
  (value) => !value.endsWith('/*'),
    map(
      (value) => `./${value[0]}`,
      parse(readFileSync(resolve('./tsconfig.json'))).compilerOptions.paths
    )
  )

module.exports = (api) => ({
  plugins: [
    ['module-resolver', { alias, extensions: ['.ts', '.tsx'] }],
  ]
})

[ljharb]

alrighty, while i'd still suggest another convention, this does seem to be an unintentional change.

[JanJakes]

Seeing probably the same issue in the following form:

@our-company/internal-package import should occur before import of apollo-server-express import/order

With the following piece of config:

pathGroups: [
  {
    pattern: '@our-company/**',
    group: 'external',
    position: 'after',
  },
  ...,
],

[jakubmazanec]

We have similar problem: in version 2.23.4 (compared to previous version we used 2.22.1), imports that are TypeScript path aliases are sorted differently.

Rule config:

'import/order': [
  'warn',
  {
    groups: [['builtin', 'external']],
    'newlines-between': 'always',
    alphabetize: { order: 'asc', caseInsensitive: true },
  },
]

Example file:

import { IncomingHttpHeaders, IncomingMessage, ServerResponse } from 'http';

import { isLocalhost } from '@core/utils'; // in tsconfig, there is compilerOptions.paths configured

Previously there were no warnings, now there are two:

There should be no empty line within import group
`@core/utils` import should occur before import of `http`

[ljharb]

Is this still an issue in v2.24.0?

[JanJakes]

It seems to work for us 👍 🎉

[JanJakes]

@ljharb Sorry, it was a false joy. I forgot to remove the resolutions field locally so the update in my test had no effect. I only discovered it now when a pull request from Renovate bot failed. So unfortunately, this is still an issue and thus should be reopened, I guess.