[Docs] order: improve the documentation for the pathGroupsExcludedImportTypes option

[liby]

Motivation: #1665

I think there's an opportunity to improve the documentation for the pathGroupsExcludedImportTypes option. The documentation made me think the allowed values for pathGroupsExcludedImportTypes were only groups (e.g., builtin, external, etc.) and not patterns (e.g., react, react-router-dom, etc).

I think this paragraph makes a lot of sense, and for a while, I couldn't figure out how to configure this option.

[liby]

@ljharb

Do you have any other questions? If you do not consider a merger, please let me know. Thank you.

[liby]

@ljharb PTAL.

[ljharb]

Thanks!

[yola-0316]

Hi, I see the impType is resolved as a fixed string in predefined set, it can't be a pattern.

[ljharb]

@yola-0316 yes, do you have a question?

[Akiq2016]

this PR add below demo

{
  "import/order": [
    "error",
    {
      "pathGroups": [
        {
          "pattern": "react",
          "group": "builtin",
          "position": "before"
        }
      ],
      "pathGroupsExcludedImportTypes": ["react"]
    }
  ]
}

"pathGroupsExcludedImportTypes": ["react"] might be confused. As source code doesn't handle patterns like 'react', it just treats it as 'unknown' type.
Though this demo works, it's because pathGroupsExcludedImportTypes doesn't include external(default value) anymore, not because we set a 'react'

[ljharb]

@Akiq2016 i'm not sure I understand, but I'd love to review a PR that either fixes the docs, or adds a failing test case :-)

[Akiq2016]

#1665 I read this issue and knowing why we had this PR, but I think they use pathGroupsExcludedImportTypes in the wrong way. If we need to let pathGroup works, sometimes we have to add pathGroupsExcludedImportTypes as [].

I think the origin docs describe clearly enough

pathGroupsExcludedImportTypes: [array]:

This defines import types that are not handled by configured pathGroups. This is mostly needed when you want to handle path groups that look like external imports.

As pathGroupsExcludedImportTypes was set to ['builtin', 'external'] as default. While we have some customize pattern in pathGroups which looks like an external import, we should ensure pathGroupsExcludedImportTypes without an 'external' value. In this way, the pathGroups will works.

This is also why #1665 's config doesn't work and after they add a pattern pathGroupsExcludedImportTypes works. But the value they set to pathGroupsExcludedImportTypes was treated as unknown, the correct way is to set pathGroupsExcludedImportTypes to [].

conclusion: this PR saying 'You can also use patterns' is wrong, the source code doesn't support this feature. and pathGroupsExcludedImportTypes: [react] doesn't make sense

[yola-0316]

Maybe we could improve the original description:

pathGroupsExcludedImportTypes: [array]:

This defines import types that are not handled by configured pathGroups. This is mostly needed when you want to handle path groups that look like external imports.

In many times, the pathGroup you configured will work as your expected. but if the pathGroups pattern is same as builtin or external. it will ignored by default. we should override pathGroupExcludedImportTypes" by set it to [] ensure it not include the external. the ["react"] is a lucky hit, not the correct way.

Incorrect:

{
  "import/order": [
    "error",
    {
      "pathGroups": [
        {
          "pattern": "react",
          "group": "builtin",
          "position": "before"
        }
      ],
      "pathGroupsExcludedImportTypes": ["react"]
    }
  ]
}

Correct:

{
  "import/order": [
    "error",
    {
      "pathGroups": [
        {
          "pattern": "react",
          "group": "builtin",
          "position": "before"
        }
      ],
      "pathGroupsExcludedImportTypes": [] // not include external, because the react is belong to external form
    }
  ]
}

If you want the pathGroups to work, it should not be listed in pathGroupsExcludedImportTypes. If you want to disable pathGroups, it should appear in pathGroupsExcludedImportTypes. This is the original meaning. But now, it has been applied exactly the opposite way

FYI: the exclude feature has been added in this PR.
#1570
I found the original author also @Mairu #1386

Perhaps we can discuss whether this configuration is necessary. Many people are now perplexed by it, even in the test case. (e.g. https://github.com/import-js/eslint-plugin-import/blob/main/tests/src/rules/order.js#L2385, the type is not necessary. ) We can either remove this configuration or change the default to only builtin.

[liby]

@Akiq2016

#1665 I read this issue and knowing why we had this PR, but I think they use pathGroupsExcludedImportTypes in the wrong way. If we need to let pathGroup works, sometimes we have to add pathGroupsExcludedImportTypes as [].

I think you’re right. We were using pathGroupsExcludedImportTypes incorrectly.

I think the origin docs describe clearly enough

I don’t think so. As can be seen from #1665, there are still a lot of people who are confused about it (including me).

conclusion: this PR saying 'You can also use patterns' is wrong, the source code doesn't support this feature. and pathGroupsExcludedImportTypes: [react] doesn't make sense

I think we should update the document so that no one else uses it incorrectly.

@yola-0316
I very much agree with your proposal for improvement. The improved description will go a long way to avoid confusion or misuse by others.

However, I have some questions about the corrected configuration.

Correct:

{
  "import/order": [
    "error",
    {
      "pathGroups": [
        {
          "pattern": "react",
          "group": "builtin",
          "position": "before"
        }
      ],
      "pathGroupsExcludedImportTypes": [] // not include external, because the react is belong to external form
    }
  ]
}

pathGroupsExcludedImportTypes is set to ['buildin', 'external'] by default. In our case, the pathGroups pattern is the same as external and there should be no need to set pathGroupsExcludedImportTypes to [], we just need to remove external from pathGroupsExcludedImportTypes. Right?

{
  "import/order": [
    "error",
    {
      "pathGroups": [
        {
          "pattern": "react*",
          "group": "external",
          "position": "before"
        }
      ],
      "pathGroupsExcludedImportTypes": ['builtin'],
    }
  ]
}

[Mairu]

Just to five my 2 cents. I introduced this option only because of backward compatibility reasons. I didn't want to change the default value of this exclusion list in a minor version.

And for the explanation, in my original documentation I added an example to make the usage clear. Because I don't know how to describe it clearly without adding too much sentences.

I can understand that this description could lead to confusions for people that are not into the internals.

I think something like

This defines import types that are not handled by configured pathGroups.
If you have added path groups with patterns that look like builtin or external imports, you have to remove this group ( builtin
and/or external ) from the default exclusion list (["builtin", "external"]) to sort these path groups correctly.

would perhaps be better?
Together with the example this should be then hopefully clear enough.