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] Force sort order of certain packages within "external" group #1665

Closed
salimhamed opened this issue Feb 20, 2020 · 9 comments
Closed

Comments

@salimhamed
Copy link

salimhamed commented Feb 20, 2020

We're using the import/order rule to enforce a convention in module imports. Here's how the import/order rule is currently configured in our project.

// in .eslintrc.js

module.exports = {
  rules: {
      'import/order': ['error', {
          groups: ['builtin', 'external', 'internal'],
          'newlines-between': 'always',
          alphabetize: {
              order: 'asc',
              caseInsensitive: true,
          },
      }],
}

This will result in the following import conventions:

// in a module with a React component 

// "external" modules are imported first
import { Button } from 'antd'
import PropTypes from 'prop-types'
import React from 'react'

// "internal" modules are imported second
import { BrandedIcon1 } from 'components/BrandedIcon1'
import { BrandedIcon2 } from 'components/BrandedIcon2'

All is well and good, except that our team's convention is to import React first when a module has React components. Therefore, the example above would become:

import React from 'react' // force React to be imported first in the "external" group
import { Button } from 'antd'
import PropTypes from 'prop-types'

import { BrandedIcon1 } from 'components/BrandedIcon1'
import { BrandedIcon2 } from 'components/BrandedIcon2'

Is it possible to achieve this desired sort order using the import/order rule? I was a bit confused by the documentation of the pathGroups and pathGroupsExcludedImportTypes and I wasn't sure if these options could be used to achieve this result. Thanks in advance for the help and keep up the great work on this plugin!

@salimhamed salimhamed changed the title Force sort order of certain packages within "external" group for import/order rule [import/order] Force sort order of certain packages within "external" group Feb 20, 2020
@kyranjamie
Copy link

At @blockstack we'd love this feature too.

@ljharb
Copy link
Member

ljharb commented Feb 25, 2020

You should be able to add pathGroups, marking React as going "before" the external group?

@salimhamed
Copy link
Author

@ljharb Thanks for the response. As far as I can tell, the pathGroups option doesn't work for builtin or external modules, as mentioned in the documentation.

image

The react module is external, so the following configuration options don't have any affect on the sort order.

module.exports = {
  rules: {
      'import/order': ['error', {
          groups: ['builtin', 'external', 'internal'],
          pathGroups: [
              {
                  pattern: 'react',  // this doesn't seem to work because react is `external`
                  group: 'external',
                  position: 'before',
              },
          ],
          'newlines-between': 'always',
          alphabetize: {
              order: 'asc',
              caseInsensitive: true,
          },
      }],
}

@egemon
Copy link

egemon commented Feb 26, 2020

this worked for me

      "newlines-between": "always",
      "groups": ["builtin", "external", "internal", "parent", "sibling", "index"],
      "pathGroups": [
        {
          "pattern": "weplay-singleton/**",
          "group": "internal",
          "position": "before"
        },
        {
          "pattern": "weplay-core/**",
          "group": "internal",
          "position": "before"
        },
        {
          "pattern": "weplay-components/**",
          "group": "internal",
          "position": "before"
        },
        {
          "pattern": "weplay-competitive/**",
          "group": "internal",
          "position": "before"
        },
        {
          "pattern": "weplay-events/**",
          "group": "internal",
          "position": "before"
        },
        {
          "pattern": "weplay-media/**",
          "group": "internal",
          "position": "before"
        },
        {
          "pattern": "weplay-platform/**",
          "group": "internal",
          "position": "before"
        },
      ],
      pathGroupsExcludedImportTypes: [
        'weplay-singleton',
        'weplay-core',
        'weplay-components',
        'weplay-competitive',
        'weplay-events',
        'weplay-media',
        'weplay-platform',
      ]
    }],```

@salimhamed
Copy link
Author

Thanks @egemon! Your solution worked for me. I was missing the pathGroupsExcludedImportTypes option.

Here's configuration that forces react to be imported above other external dependencies.

// in .eslintrc.js

module.exports = {
  rules: {
      'import/order': ['error', {
          groups: ['builtin', 'external', 'internal'],
          pathGroups: [
              {
                  pattern: 'react',
                  group: 'external',
                  position: 'before',
              },
          ],
          pathGroupsExcludedImportTypes: [
              'react',
          ],
          'newlines-between': 'always',
          alphabetize: {
              order: 'asc',
              caseInsensitive: true,
          },
      }],
}

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).

image

@ljharb
Copy link
Member

ljharb commented Mar 3, 2020

Glad you figured it out :-) so is this resolved? Happy to reopen if not.

@ljharb ljharb closed this as completed Mar 3, 2020
@egemon
Copy link

egemon commented Mar 3, 2020

@ljharb it is. I think I should open PR and issue to add smth like that into documentation?

@ljharb
Copy link
Member

ljharb commented Mar 3, 2020

Thanks, that'd be great.

@salimhamed
Copy link
Author

@ljharb Yes, this is resolved. Thanks!

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

No branches or pull requests

4 participants