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

Bug in order.js preventing from assigning external to patterns using pathGroups #1724

Merged
merged 1 commit into from
Apr 11, 2020

Conversation

xpl
Copy link
Contributor

@xpl xpl commented Apr 9, 2020

Here, if group is external or builtin then the returned rank will be 0:

if ((0, _minimatch2.default)(path, pattern, patternOptions || { nocomment: true })) {
      return ranks[group] + position / maxPosition;
}

...and then, there we would erroneously throw it away, as if 0 was not a valid rank (which isn't true):

if (!rank) {
    rank = ranks.groups[impType];
}

Making the comparison more strict fixes the bug:

if (rank === undefined) {
    rank = ranks.groups[impType];
}

Co-authored-by: Emily Marigold Klassen <forivall@gmail.com>
Co-authored-by: Vitaly Gordon <rocket.mind@gmail.com>
@coveralls
Copy link

coveralls commented Apr 9, 2020

Coverage Status

Coverage remained the same at 97.804% when pulling fe6cea9 on xpl:patch-1 into baf1a8c on benmosher:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.804% when pulling e7a1c70 on xpl:patch-1 into baf1a8c on benmosher:master.

1 similar comment
@coveralls
Copy link

Coverage Status

Coverage remained the same at 97.804% when pulling e7a1c70 on xpl:patch-1 into baf1a8c on benmosher:master.

@golopot
Copy link
Contributor

golopot commented Apr 10, 2020

Can you add test cases in https://github.com/benmosher/eslint-plugin-import/blob/master/tests/src/rules/order.js ?

@xpl
Copy link
Contributor Author

xpl commented Apr 10, 2020

Sure. Will add the case a bit later.

@xpl
Copy link
Contributor Author

xpl commented Apr 11, 2020

@golopot I've added the case. Also, sorry about some confusion in my initial message — in my case, builtin and external were merged to a single group, having the same rank (0), hence the title.

I should've probably titled the PR "preventing from assigning builtin", but that doesn't really matter as the bug is still there.

So here is the code, it assigns ./make-me-external to external group, and it fails without the fix, and succeeds with the fix:

    test({
      code: `
        import fs from 'fs';
        import external from 'external';
        import externalTooPlease from './make-me-external';

        import sibling from './sibling';`,
      options: [{
        'newlines-between': 'always',
        pathGroupsExcludedImportTypes: [],
        pathGroups: [
          { pattern: './make-me-external', group: 'external' },
        ],
        groups: [['builtin', 'external'], 'internal', 'parent', 'sibling', 'index'],
      }],
    }),

Under the hood, this happens:

{
    ranks: {
        builtin: 0,   // <----- having rank 0
        external: 0,  // <----- having rank 0
        internal: 1,
        parent: 2,
        sibling: 3,
        index: 4,
        unknown: 5
    },
    pathGroups: [{
        pattern: './make-me-external',
        group: 'external',
        position: 0  // because it's 0, the expression evaluates to 0:
                     //     ranks[group] + (position / maxPosition)
    }]
}

@xpl
Copy link
Contributor Author

xpl commented Apr 11, 2020

Oh my, here is a PR with the same fix (and with a bit more proper title)! :)

#1719

@ljharb ljharb added the bug label Apr 11, 2020
@ljharb
Copy link
Member

ljharb commented Apr 11, 2020

Thanks, I'll handle getting both landed.

Copy link
Member

@ljharb ljharb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combined the two PRs; the fix from both, plus your tests. Thanks!

@ljharb
Copy link
Member

ljharb commented Apr 11, 2020

Because I pushed this to two PRs, two appveyor builds ran. The second failed (https://ci.appveyor.com/project/benmosher/eslint-plugin-import/builds/32116513), but the first succeeded (https://ci.appveyor.com/project/benmosher/eslint-plugin-import/builds/32116512), so I'm going to merge through the failure.

@ljharb ljharb merged commit fe6cea9 into import-js:master Apr 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Development

Successfully merging this pull request may close these issues.

5 participants