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.

Sign up for GitHub

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

Jump to bottom

fix(comp:tree): searchKeys is affected by the side effects of other v… #1483

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

fix(comp:tree): searchKeys is affected by the side effects of other v… #1483

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 21 additions & 16 deletions packages/components/tree/src/composables/useSearchable.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,9 +5,9 @@
* found in the LICENSE file at https://github.com/IDuxFE/idux/blob/main/LICENSE
*/

import { type ComputedRef, computed } from 'vue'
import { type ComputedRef, computed, watch } from 'vue'

import { NoopArray, type VKey } from '@idux/cdk/utils'
import { type VKey, useState } from '@idux/cdk/utils'

import { type MergedNode } from './useDataSource'
import { type TreeNode, type TreeProps } from '../types'
Expand All @@ -23,21 +23,26 @@ export function useSearchable(
): SearchableContext {
const mergedSearchFn = useSearchFn(props, mergedLabelKey)

const searchedKeys = computed(() => {
const { searchValue } = props
if (!searchValue) {
return NoopArray as unknown as VKey[]
}
const searchFn = mergedSearchFn.value
const keys: VKey[] = []
mergedNodeMap.value.forEach(node => {
if (searchFn(node.rawNode, searchValue)) {
keys.push(node.key)
const [searchedKeys, setSearchedKeys] = useState<VKey[]>([])

watch(
() => props.searchValue,
searchValue => {
const searchFn = mergedSearchFn.value
const keys: VKey[] = []
if (searchValue) {
mergedNodeMap.value.forEach(node => {
if (searchFn(node.rawNode, searchValue)) {
keys.push(node.key)
}
})
}
})

return keys
})
setSearchedKeys(keys)
},
{
immediate: true,
},
)

return { searchedKeys }
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with a brief code review

In this code patch, the main changes are importing watch from vue and replacing computed with useState from @idux/cdk/utils.
It also modifies the logic of searching nodes by keywords.

The first thing to check is if the changes made are syntactically correct. For example, does the function useSearchable still compile and run? Are there any errors in the code? If not, then the syntax is correct.

Next, we should check for any potential bugs, such as whether the search function works correctly for all conditions. Is it possible for the setSearchedKeys function to not be called when the searchValue changes, or for it to be called twice?

Finally, we should look for any potential improvements that could be made. For example, is there a way to make the code more efficient or readable? Could the logic of the search function be simplified?

Expand Down