fix: arrays with multiple types

mesqueeb commented 2 years ago

fixes #62

mesqueeb commented 2 years ago

@snovakovic fixed in this PR; I tried to follow your code style as much as I could. I hope this PR helps.

snovakovic commented 2 years ago

@mesqueeb Thanks for bringing this issue and for the PR!

One hesitation I have with merging this is that it has big impact on performance even on arrays that don't have multiple types (Which is more common use case).

From the attached benchmark you can notice that by average execution time has more than doubled with this change.

Also just to say that for this particular use case you can use naturalSort and it will sort array as expected

    const naturalSort = createNewSortInstance({
      comparer: new Intl.Collator(undefined, { numeric: true, sensitivity: 'base' }).compare,
    });

    const sorted = naturalSort(['b', 3, 2, 1, 5, 'a', 5, 4]).asc();
    assert.deepStrictEqual(sorted, [1, 2, 3, 4, 5, 5, 'a', 'b']);

I'll have to think some more on it (when I catch some free time) but for now I'm leaning to maybe just add "Know issues" section in documentation and mentioning in there that for multiple type arrays we should use natural sort or some other custom sorter that can handle that. Let me know if any other suggestions come to mind!

snovakovic commented 2 years ago

@mesqueeb I open PR (influenced with this one) with performance in mind https://github.com/snovakovic/fast-sort/pull/64

I would appreciate if you have time to peek at it. There is still some know issues edge cases there but I'm kind of joggling between known issues && performance.

snovakovic commented 1 year ago

Closing this PR due to performance implication mentioned in previous comment https://github.com/snovakovic/fast-sort/pull/61#issuecomment-1225906797

snovakovic / fast-sort

fix: arrays with multiple types #61