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

[xpl]

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];
}

[coveralls]

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

[coveralls]

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

[coveralls]

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

[golopot]

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

[xpl]

Sure. Will add the case a bit later.

[xpl]

@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]

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

#1719

[ljharb]

Thanks, I'll handle getting both landed.

[ljharb]

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

[ljharb]

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.