-
-
Notifications
You must be signed in to change notification settings - Fork 2.4k
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
feat(useSort, useQuickSort): new function #1465
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self reviewed
packages/core/useSort/index.ts
Outdated
* @param sortFn sort function | ||
* @param options | ||
*/ | ||
export function useSort<T>(source: MaybeRef<T[]>, sortFn: UseSortFn<T>, options: UseSortOptions<T> = {}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
export function useSort<T>(source: MaybeRef<T[]>, sortFn: UseSortFn<T>, options: UseSortOptions<T> = {}) { | |
export function useSorted<T>(source: MaybeRef<T[]>, sortFn: UseSortFn<T>, options: UseSortOptions<T> = {}) { |
sort
sounds like mutating, while we are trying to be immutable. sorted
sounds like a better fit https://github.com/tc39/proposal-change-array-by-copy
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also like to see it matches with JS's sort more with an optional comparator (a, b) => number
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would also like to see it matches with JS's sort more with an optional comparator
(a, b) => number
packages/core/useQuickSort/index.ts
Outdated
import type { UseSortCompareFn } from '../useSort' | ||
import { useSortWrapFn } from '../useSort' | ||
|
||
export function quickSort<T>(source: T[], compareFn: UseSortCompareFn<T>): T[] { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit hesitated about this. Could we force on the general useSorted
first and then think about the possible way to support more algorithms?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, I feel JS's native sort should be quite fast already (https://stackoverflow.com/a/37245185), I have never written a sorting algorithm in JS actually. Is there any benefit to implementing a quick sort on our own?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am a bit hesitated about this. Could we force on the general
useSorted
first and then think about the possible way to support more algorithms?
ok~.
Also, I feel JS's native sort should be quite fast already (https://stackoverflow.com/a/37245185), I have never written a sorting algorithm in JS actually. Is there any benefit to implementing a quick sort on our own?
Not sure. Every browser type has it's own javascript engine implementation.
Just wanted to provide a useSorted
implementation here.
packages/core/useSort/index.ts
Outdated
* @param sortFn sort function | ||
* @param options | ||
*/ | ||
export function useSort<T>(source: MaybeRef<T[]>, sortFn: UseSortFn<T>, options: UseSortOptions<T> = {}) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@antfu Perhaps it would be better if I added sortFn to the options, providing a default sort method.
export interface UseSortedOptions<T> {
/**
* sort algorithm
*/
sortFn?: UseSortedFn<T>
/**
* compare function
*/
compareFn?: UseSortedCompareFn<T>
/**
* change the value of the source array
* @default false
*/
dirty?: boolean
}
const defaultSortFn = <T>(source: T[], compareFn: UseSortedCompareFn<T>): T[] => source.sort(compareFn)
export function useSorted<T>(source: MaybeRef<T[]>, options: UseSortedOptions<T> = {}) {
const { sortFn = defaultSortFn, compareFn = defaultCompare, dirty = false } = options
// ...
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>
Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>
Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>
Co-authored-by: Anthony Fu <anthonyfu117@hotmail.com>
Description
Composable reactive sort array.
If this PR passes the merge, I will start adding other common sorting algorithms.
Additional context
closes: #1464
What is the purpose of this pull request?
Before submitting the PR, please make sure you do the following
fixes #123
).