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

Fix sorting of array with multiple types #64

Merged
merged 6 commits into from
Apr 15, 2023
Merged

Fix sorting of array with multiple types #64

merged 6 commits into from
Apr 15, 2023

Conversation

snovakovic
Copy link
Owner

@snovakovic snovakovic commented Aug 24, 2022

fixes #62

Running of benchmark and there is no noticeable effect on performance

image

@snovakovic
Copy link
Owner Author

snovakovic commented Aug 24, 2022

There is still some know issues with this update as e.g

sort([1, 3, 2, '1a', 'a', '2', '5', 6]).asc() // === [1,2,"2",3,"1a","5",6,"a"]

But currently trying to find balance between know issue and performance... It's easy to fix above but that require more check which have negative performance impact on every sort just for some edge cases. (&& we can always use naturalSort or any custom sort)

@mesqueeb
Copy link
Contributor

mesqueeb commented Sep 15, 2022

@snovakovic how about providing a separate sort function that can be imported just for the case of arrays with multiple types.

I agree we shouldn't cause any degredation in the existing performance.

You could call it import { sortMultiTypeArray } from 'fast-sort' or something

and tree-shaking will make it so no one will have negative impact.

@snovakovic
Copy link
Owner Author

@mesqueeb on second look manage to solve above issue without performance penalties so there is no need for separate function

@snovakovic snovakovic merged commit b56a82b into master Apr 15, 2023
@snovakovic snovakovic deleted the multi-array branch April 15, 2023 18:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

cannot sort on multiple types in an array
2 participants