Skip to content

Conversation

@chrisblossom
Copy link
Contributor

#95 might be considered a breaking change since the files are sorted in reverse order. I am not sure this reverts the old order exactly, but thought I'd put this PR in to let you decide what's best.

@sindresorhus
Copy link
Owner

Why do you think it’s a breaking change?

@chrisblossom
Copy link
Contributor Author

Currently if someone has code like this:

test('removes files', async () => {
    // assume these files exist
    // './remove-these-files/a.js'
    // './remove-these-files/b.js'
    const filesRemoved = await del(['**/*'], {
        cwd: path.resolve('./remove-these-files'),
    });

    expect(filesRemoved).toEqual([
        path.resolve('./remove-these-files/a.js'),
        path.resolve('./remove-these-files/b.js'),
    ]);
});

Their test will fail after this update.

Not 100% saying it is breaking, just something to consider. I noticed it on one of my projects.

@sindresorhus
Copy link
Owner

sindresorhus commented Jul 13, 2019

The order was already unstable as globbing is inherently not stable, and the order could change in patch releases because of glob optimizations. So yes, makes sense to sort it back for consistency.

@sindresorhus sindresorhus changed the title Reverse order back to prevent breaking change Reverse order back for the returned paths Jul 13, 2019
@sindresorhus sindresorhus merged commit 51662ac into sindresorhus:master Jul 13, 2019
@chrisblossom chrisblossom deleted the re-sort-after-completion branch July 13, 2019 08:03
@chrisblossom
Copy link
Contributor Author

The order was already unstable as globbing is inherently not stable, and the order could change in patch releases because of glob optimizations. So yes, makes sense to sort it back for consistency.

After thinking about this comment I think this PR should be updated to use .sort((a, b) => a.localeCompare(b)) instead of .reverse() to consistently return the same order every time.

Should I put another PR in for this, or do you want to amend this merge/leave as is?

@sindresorhus
Copy link
Owner

Sure, PR welcome.

@chrisblossom chrisblossom mentioned this pull request Jul 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants