`import/order` not respecting `builtin` #2687

thernstig · 2023-01-20T15:02:33Z

Using "eslint-plugin-import": "2.27.5"

// .eslintrc.json
{
  "root": true,
  "reportUnusedDisableDirectives": true,
  "parserOptions": {
    "ecmaVersion": 2023,
    "sourceType": "module"
  },
  "env": {
    "node": true,
    "es2022": true
  },
  "plugins": ["import"],
  "rules": {
    "import/order": [
      "error",
      {
        "groups": [
          [
            "builtin",
            "external",
            "internal",
            "parent",
            "sibling",
            "index",
            "object",
            "type"
          ]
        ]
      }
    ]
  }
}

// test.js
import chpro from 'child_process';
import log4js from 'log4js';
import util from 'util';

const execFile = util.promisify(chpro.execFile);
const logger = log4js.getLogger('server');

I would expect util to be sorted before log4js but this is not the case. Neither is node:util.

The text was updated successfully, but these errors were encountered:

shawnmcknight · 2023-02-24T14:24:55Z

I ran into this with a different built-in import (path). If I changed the import to use the new node protocol semantics (i.e. 'node:path') then the import order was correct. This made me believe that this detection might be due to a conflicting node core module and a module installed in node_modules. Sure enough, if I look at my node_modules there is a path dependency in there (also a util dependency, which might match @thernstig 's scenario).

If I use a core module like fs where I don't have a conflicting dependency in node_modules then the import order is correct (actually wanted to precede node:path which is what I would have expected).

Taking a guess here that import/order might be getting confused because of the duplicate names in core and node_modules, but that's just a shot in the dark guess.

thernstig · 2023-02-24T14:37:32Z

@shawnmcknight that is not my experience. The above config does not complain about this ordering:

import express from 'express';
import log4js from 'log4js';
import chpro from 'node:child_process';
import fsp from 'node:fs/promises';

I would expect all node: to end up first.

ljharb · 2023-04-14T18:16:26Z

@thernstig why do you have the nested array in groups?

ljharb · 2023-04-14T18:49:30Z

I was able to partially fix this in 1fa2971 - now the same error shows up with a nested group or a flat one - but the ordering still isn't fully correct.

thernstig · 2023-04-15T07:14:41Z

@ljharb I got that from eslint-config-airbnb-base which I used as a base for creating this issue, so maybe you can answer that. I saw though that https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/order.md does not have the nested array.

But I am using eslint-config-airbnb-base so in the end I get my config from there.

ljharb · 2023-04-15T19:12:29Z

@thernstig the base config has { groups: [['builtin', 'external', 'internal']] } in it, so something else must be providing that list.

thernstig · 2023-04-16T08:29:07Z

@ljharb I took it from eslint-config-airbnb-base, but then just added all items from here to test them all:

eslint-plugin-import/docs/rules/order.md

Lines 105 to 121 in 683d3a5

    
           ```ts 
        
           "import/order": [ 
        
             "error", 
        
             { 
        
               "groups": [ 
        
                 "index", 
        
                 "sibling", 
        
                 "parent", 
        
                 "internal", 
        
                 "external", 
        
                 "builtin", 
        
                 "object", 
        
                 "type" 
        
               ] 
        
             } 
        
           ] 
        
           ```

Is eslint-config-airbnb-base wrong?

ljharb · 2023-04-16T15:07:39Z

No, it’s not wrong, i was just curious where the config comes from.

yndajas · 2023-08-09T10:06:14Z

👋🏻 is the (partial) fix for this correct, particularly this conditional?

After updating to 2.28.0 we're seeing "groups": [["builtin", "external"]] (one group) lead to builtin and external being separated, when I believe these should be grouped together with everything else in a second group. This was the behaviour before 2.28.0 and seemingly what the docs suggest: ['sibling', 'parent'], // Then sibling and parent types. They can be mingled together ("They can be mingled together"). We've had to amend the config to keep the previous behaviour

I might not be fully understanding the original issue in this thread, but I would've thought it would be solved by de-nesting? It looks like the Airbnb config example is: groups: [['builtin', 'external', 'internal']], which would group builtin, external, and internal and then follow with everything else in a second group, so if you wanted builtin then external then internal then parent then sibling then index then object then type it would be groups: ["builtin", "external", "internal", "parent", "sibling", "index", "object", "type"] (de-nested)

I note that there's a TODO in the conditional and this is a described as a partial fix, so maybe the new behaviour is intended to be reverted eventually?

ljharb · 2023-08-09T10:45:08Z

@yndajas that sounds right to me. If the behavior has changed in v2.28.0 then that's a bug that needs fixing.

yndajas · 2023-08-09T10:46:37Z

@ljharb glad I'm not confused! Shall I open a new issue or leave it with you?

ljharb · 2023-08-09T10:47:57Z

@yndajas I think this issue probably covers it; a PR would be most helpful :-)

1fa2971 changed the way groups work when there is only one, leading to single nested groups being treated as though they were unnested. Prior to this, if you wanted to group e.g. builtin and external imports together at the top and everything else together as a second group, a single nested group was the way to do it [It appears that this change was unintentional][1], and was made to try to fix what seems to be a misunderstanding around nested groups ([import-js#2687]). [The docs][2] continue to suggest that nested groups should be "mingled together" and makes no reference to a single nested group with no other groups being an invalid option This therefore reverts the change to how groups work when there is only one. No documentation change should be necessary given this is already the described behaviour [1]: import-js#2687 (comment) [2]: https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/order.md#groups-array

thernstig · 2023-08-17T06:11:31Z

@yndajas @ljharb I am not sure how the problem reported there solves this issue? This is about it not respecting builtin at all.

1fa2971 changed the way groups work when there is only one, leading to single nested groups being treated as though they were unnested. Prior to this, if you wanted to group e.g. builtin and external imports together at the top and everything else together as a second group, a single nested group was the way to do it [It appears that this change was unintentional][1], and was made to try to fix what seems to be a misunderstanding around nested groups ([import-js#2687]). [The docs][2] continue to suggest that nested groups should be "mingled together" and makes no reference to a single nested group with no other groups being an invalid option This therefore reverts the change to how groups work when there is only one. No documentation change should be necessary given this is already the described behaviour [1]: import-js#2687 (comment) [2]: https://github.com/import-js/eslint-plugin-import/blob/main/docs/rules/order.md#groups-array

ljharb · 2023-08-17T06:15:41Z

@thernstig when it's nested like that, all of those groups can be intermingled. The config needs to de-nest the groups in order for their order to be enforced.

yndajas · 2023-08-17T09:58:08Z

@thernstig per @ljharb, I believe the original issue in this thread would be solved by denesting the groups:

// .eslintrc.json
{
  "root": true,
  "reportUnusedDisableDirectives": true,
  "parserOptions": {
    "ecmaVersion": 2023,
    "sourceType": "module"
  },
  "env": {
    "node": true,
    "es2022": true
  },
  "plugins": ["import"],
  "rules": {
    "import/order": [
      "error",
      {
        "groups": [
          "builtin",
          "external",
          "internal",
          "parent",
          "sibling",
          "index",
          "object",
          "type"
        ]
      }
    ]
  }
}

More detailed explanation in my earlier comment

Hope that clarifies!

ljharb · 2023-08-17T22:23:15Z

Per that comment, I'll close this, but can reopen if it doesn't work out.

ljharb added the import/export ordering label Jan 27, 2023

ljharb added a commit to ljharb/eslint-plugin-import that referenced this issue Apr 14, 2023

[Fix] order: partial fix for import-js#2687

1fa2971

ljharb added the help wanted label Apr 14, 2023

yndajas mentioned this issue Aug 9, 2023

[Fix] Revert breaking group change in order #2854

Merged

ljharb closed this as completed Aug 17, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`import/order` not respecting `builtin` #2687

`import/order` not respecting `builtin` #2687

thernstig commented Jan 20, 2023 •

edited

shawnmcknight commented Feb 24, 2023

thernstig commented Feb 24, 2023

ljharb commented Apr 14, 2023

ljharb commented Apr 14, 2023

thernstig commented Apr 15, 2023 •

edited

ljharb commented Apr 15, 2023

thernstig commented Apr 16, 2023 •

edited

ljharb commented Apr 16, 2023

yndajas commented Aug 9, 2023

ljharb commented Aug 9, 2023

yndajas commented Aug 9, 2023

ljharb commented Aug 9, 2023

thernstig commented Aug 17, 2023

ljharb commented Aug 17, 2023

yndajas commented Aug 17, 2023 •

edited

ljharb commented Aug 17, 2023

import/order not respecting builtin #2687

import/order not respecting builtin #2687

Comments

thernstig commented Jan 20, 2023 • edited

shawnmcknight commented Feb 24, 2023

thernstig commented Feb 24, 2023

ljharb commented Apr 14, 2023

ljharb commented Apr 14, 2023

thernstig commented Apr 15, 2023 • edited

ljharb commented Apr 15, 2023

thernstig commented Apr 16, 2023 • edited

ljharb commented Apr 16, 2023

yndajas commented Aug 9, 2023

ljharb commented Aug 9, 2023

yndajas commented Aug 9, 2023

ljharb commented Aug 9, 2023

thernstig commented Aug 17, 2023

ljharb commented Aug 17, 2023

yndajas commented Aug 17, 2023 • edited

ljharb commented Aug 17, 2023

`import/order` not respecting `builtin` #2687

`import/order` not respecting `builtin` #2687

thernstig commented Jan 20, 2023 •

edited

thernstig commented Apr 15, 2023 •

edited

thernstig commented Apr 16, 2023 •

edited

yndajas commented Aug 17, 2023 •

edited