-
-
Notifications
You must be signed in to change notification settings - Fork 3.4k
fix: array filterFns
#5697
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
base: main
Are you sure you want to change the base?
fix: array filterFns
#5697
Conversation
WalkthroughThree array filter functions in the table-core library are enhanced to support both array-valued and scalar-valued row values. Each function now conditionally applies array or scalar comparison logic based on the row value type, maintaining backward compatibility with existing function signatures. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~18 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
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.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/table-core/src/filterFns.ts (1)
45-54: Array vs scalar handling inarrIncludes*looks correct; consider cachingrow.getValueonceThe new logic for
arrIncludes,arrIncludesAll, andarrIncludesSomekeeps the old behavior for array-valued cells and adds sensible scalar fallbacks (strict equality, “all equal”, and “any equal” respectively). This should address non-array values cleanly without breaking existing array use-cases.To tighten things up and avoid multiple
row.getValue(columnId)calls per row/filter item, you could cache the cell value once in each function and reuse it in both branches:const arrIncludes: FilterFn<any> = ( row, columnId: string, filterValue: unknown, ) => { - if (Array.isArray(row.getValue(columnId))) { - return row.getValue<unknown[]>(columnId)?.includes(filterValue) - } - return filterValue === row.getValue(columnId) + const value = row.getValue(columnId) + if (Array.isArray(value)) { + return (value as unknown[]).includes(filterValue) + } + return filterValue === value } const arrIncludesAll: FilterFn<any> = ( row, columnId: string, filterValue: unknown[], ) => { - if (Array.isArray(row.getValue(columnId))) { - return !filterValue.some( - val => !row.getValue<unknown[]>(columnId)?.includes(val) - ) - } - return !filterValue.some(val => val !== row.getValue(columnId)) + const value = row.getValue(columnId) + if (Array.isArray(value)) { + return !filterValue.some( + val => !(value as unknown[]).includes(val), + ) + } + return !filterValue.some(val => val !== value) } const arrIncludesSome: FilterFn<any> = ( row, columnId: string, filterValue: unknown[], ) => { - if (Array.isArray(row.getValue(columnId))) { - return filterValue.some(val => - row.getValue<unknown[]>(columnId)?.includes(val) - ) - } - return filterValue.some(val => val === row.getValue(columnId)) + const value = row.getValue(columnId) + if (Array.isArray(value)) { + return filterValue.some(val => + (value as unknown[]).includes(val), + ) + } + return filterValue.some(val => val === value) }This keeps semantics the same while slightly improving readability and avoiding repeated lookups and generic calls.
Also applies to: 58-69, 73-84
Addresses the issue #5617
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.