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] Regression upgrading from 2.26.0 to 2.27.1+ #2669

Closed
MarcAndre-Rivet opened this issue Jan 12, 2023 · 11 comments
Closed

[import/order] Regression upgrading from 2.26.0 to 2.27.1+ #2669

MarcAndre-Rivet opened this issue Jan 12, 2023 · 11 comments

Comments

@MarcAndre-Rivet
Copy link

MarcAndre-Rivet commented Jan 12, 2023

I have the following setup and code experiencing a linting issue with 2.27.1 but not 2.26.0 for the import/order rule and what I think is the root issue.

.eslintrc fragment:

    'import/order': [
        2,
        {
            alphabetize: {
                order: 'asc',
                caseInsensitive: true
            },
            'newlines-between': 'always'
        }
    ],

tsconfig.json:

{
    "compilerOptions": {
        "module": "commonjs",
        "esModuleInterop": true,
        "declaration": true,
        "removeComments": true,
        "emitDecoratorMetadata": true,
        "experimentalDecorators": true,
        "allowSyntheticDefaultImports": true,
        "strictBindCallApply": true,
        "strictNullChecks": true,
        "target": "es2022",
        "sourceMap": true,
        "outDir": "./dist",
        "baseUrl": "./",
        "incremental": true,
        "skipLibCheck": true,
        "paths": {
            "@core/*": ["core/src/*"],
            "@schedule/*": ["schedule/src/*"],
        }
    }
}

I have a TS file that has the following structure:

import CoreA from '@core/A';
import CoreB from '@core/B';
import CoreC from '@core/C';

import PackageA from '@schedule/A';
import PackageB from '@schedule/B';
import PackageC from '@schedule/C';
import PackageD from '@schedule/D';
import PackageE from '@schedule/E';
import PackageF from '@schedule/F';
import PackageG from '@schedule/G';

export default () => {};

Interestingly enough this error only occurs once there is 10+ imports in the file, no `import/order' error occurs otherwise.

Digging into the rule's code, it seems the issue comes from a default .sort() that implicitly cast numbers into strings for ordering and will order 10 before 2 (in my case).

This results into this ranking assignment:

{
  '@schedule/A|value': 10,
  '@schedule/B|value': 11,
  '@schedule/C|value': 12,
  '@schedule/D|value': 13,
  '@schedule/E|value': 14,
  '@schedule/F|value': 15,
  '@schedule/G|value': 16,
  '@core/A|value': 9, // smaller than previous item, causing error reporting
  '@core/B|value': 10,
  '@core/C|value': 11
}

Changing it to .sort((lhs, rhs) => lhs - rhs) seems to resolve the issue although ranking has gaps (not sure this could cause issues elsewhere).

This results in this ranking:

{
  '@core/A|value': 2,
  '@core/B|value': 3,
  '@core/C|value': 4,
  '@schedule/A|value': 13,
  '@schedule/B|value': 14,
  '@schedule/C|value': 15,
  '@schedule/D|value': 16,
  '@schedule/E|value': 17,
  '@schedule/F|value': 18,
  '@schedule/G|value': 19
}

Let me know if you know more info/config to repro 😄

@ljharb
Copy link
Member

ljharb commented Jan 12, 2023

hm, i can't reproduce this in our tests, although the cause you suggest seems highly likely to be accurate.

@shooftie
Copy link

shooftie commented Jan 12, 2023

Rather than create a new issue, I am also getting a regression but for me it is to do with the ordering of imports with extensions.

My config is:

    'import/order': [
      'warn',
      {
        'newlines-between': 'always',
        alphabetize: {
          order: 'asc',
          caseInsensitive: false,
        },
        pathGroupsExcludedImportTypes: ['builtin'],
        warnOnUnassignedImports: true,
        pathGroups: [
          {
            pattern: '**/*.svg',
            patternOptions: { matchBase: true },
            group: 'type',
            position: 'after',
          },
        ],
      },
    ],

and I am now getting the error:

image

@ljharb
Copy link
Member

ljharb commented Jan 12, 2023

@shooftie can you share the code itself being warned on, in text form? (all the imports)

@shooftie
Copy link

There are loads. I will put together a smaller example.

@shooftie
Copy link

Full config

    'import/order': [
      'warn',
      {
        'newlines-between': 'always',
        alphabetize: {
          order: 'asc',
          caseInsensitive: false,
        },
        groups: ['builtin', 'external', 'internal', 'parent', 'sibling', 'index', 'object', 'type'],
        pathGroupsExcludedImportTypes: ['builtin'],
        warnOnUnassignedImports: true,
        pathGroups: [
          {
            pattern: '**/*.raw.js',
            group: 'type',
            position: 'after',
          },
          {
            pattern: '**/*.svg',
            group: 'type',
            position: 'after',
          },
          {
            pattern: '@app/config/**',
            group: 'internal',
            position: 'after',
          },
          {
            pattern: '@app/constants/cookies',
            group: 'internal',
            position: 'after',
          },
          {
            pattern: '@app/constants/**',
            group: 'internal',
            position: 'after',
          },
          {
            pattern: '@app/services/**',
            group: 'internal',
            position: 'after',
          },
          {
            pattern: '@app/util/**',
            group: 'internal',
            position: 'after',
          },
          {
            pattern: '@app/component/**',
            group: 'internal',
            position: 'after',
          },
          {
            pattern: './constants',
            group: 'sibling',
            position: 'after',
          },
          {
            pattern: './helper',
            group: 'sibling',
            position: 'after',
          },
        ],
      },
    ],

imports

import PropTypes from 'prop-types';
import React from 'react';
import styled from 'styled-components';

import { device } from '@app/config/theme/Styles';

import { COOKIE_FOO } from '@app/constants/cookies';

import { CLASS_BIG_LETTER, MIN_PARAGRAPH_LENGTH } from '@app/constants/article';
import { ID_CONTENT } from '@app/constants/ids';

import { isFeatureEnabled } from '@app/services/featureFlag';

import { addStrAfterPosition } from '@app/util/addStrAfterPosition';
import transformProduct from '@app/util/transformProduct';

import MPU from '@app/component/library/MPU';
import Product from '@app/component/library/Product';

import Newsletter from '../Newsletter';
import Quote from '../Quote';

import Readmore from './Readmore';
import SocialShare from './SocialShare';
import Table from './Table';
import Topics from './Topics';

import { readmoreOnce, removeEmptySnippet } from './helper';

import LogoStamp from '@app/component/icons/logoStamp.svg';

2.27.0, error

image

2.26.0, no error (other than no-unused-vars)

image

@MarcAndre-Rivet
Copy link
Author

hm, i can't reproduce this in our tests, although the cause you suggest seems highly likely to be accurate.

@ljharb maybe the missing detail is that this came up in a repo using yarn workspaces and the example code would exist in the schedule workspace, effectively causing all @core/* entries to be treated as externals vs. @schedule/* internals (core workspace) for the code provided above.

@MoeHamdan
Copy link

Hello

Same issue over here after the release of v 2.27

@cojoclaudiu
Copy link

cojoclaudiu commented Jan 14, 2023

same problem here, all of my imports where changed after the update to 2.27.4, I downgrade to 2.26.0 and all the imports are now in the correct order:
here is my import order

"import/order": [
	"error",
	{
		"groups": [
			"builtin", // Built-in imports (come from NodeJS native) go first
			"external", // <- External imports
			"internal", // <- Absolute imports
			["sibling", "parent"], // <- Relative imports, the sibling and parent types they can be mingled together
			"index", // <- index imports
			"unknown" // <- unknown
		],
		"newlines-between": "always",
		"alphabetize": {
			/* sort in ascending order. Options: ["ignore", "asc", "desc"] */
			"order": "asc",
			/* ignore case. Options: [true, false] */
			"caseInsensitive": true
		}
	}
],```

sreetamdas added a commit to sreetamdas/sreetamdas.com that referenced this issue Jan 16, 2023
Current regression in files with > 10 imports, ref: import-js/eslint-plugin-import#2669
sreetamdas added a commit to sreetamdas/sreetamdas.com that referenced this issue Jan 16, 2023
@MarcAndre-Rivet
Copy link
Author

Likely fixed by #2674

@ljharb
Copy link
Member

ljharb commented Jan 16, 2023

Please try v2.27.5, and we can reopen if it's not fixed.

@ljharb ljharb closed this as completed Jan 16, 2023
@MarcAndre-Rivet
Copy link
Author

@ljharb Looks all good on my end. Thanks!

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

No branches or pull requests

5 participants