feat: expose rank info in results#169
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds a new exported function Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/index.ts (1)
50-62:⚠️ Potential issue | 🟠 MajorOverload typing is unsound when
returnRankInfois a non-literal boolean.When
returnRankInfois a variable of typeboolean(not a literaltrueorfalse), TypeScript selects the second overload (line 93–97) and types the return asArray<ItemType>. However, the runtime can returnArray<RankedItem<ItemType>>if that variable evaluates totrueat runtime, creating a type safety gap.To fix, change
returnRankInfo?: booleantoreturnRankInfo?: falseinMatchSorterOptions. This forces narrowing of non-literal booleans and ensures overload selection aligns with runtime behavior.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` around lines 50 - 62, The overload is unsound because MatchSorterOptions currently declares returnRankInfo?: boolean; change that to returnRankInfo?: false so non-literal booleans are not treated as the true-specialized overload; update the interface definition for MatchSorterOptions (and keep MatchSorterRankInfoOptions with returnRankInfo: true) so TypeScript will narrow correctly and the overload resolution for functions that use MatchSorterOptions/MatchSorterRankInfoOptions matches runtime behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/index.ts`:
- Around line 50-62: The overload is unsound because MatchSorterOptions
currently declares returnRankInfo?: boolean; change that to returnRankInfo?:
false so non-literal booleans are not treated as the true-specialized overload;
update the interface definition for MatchSorterOptions (and keep
MatchSorterRankInfoOptions with returnRankInfo: true) so TypeScript will narrow
correctly and the overload resolution for functions that use
MatchSorterOptions/MatchSorterRankInfoOptions matches runtime behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 20c5038d-e245-4440-b7de-4311ec7e598d
📒 Files selected for processing (3)
README.mdsrc/__tests__/index.tssrc/index.ts
kentcdodds
left a comment
kentcdodds
left a comment
There was a problem hiding this comment.
Thanks for this! I think it's useful. Just some notes on types.
| value: string, | ||
| options: MatchSorterOptions<ItemType> = {}, | ||
| ): Array<ItemType> { | ||
| ): Array<ItemType> | Array<RankedItem<ItemType>> { |
There was a problem hiding this comment.
I think there's some clever TypeScript schenanigans you can do here to make sure the type is only Array<RankedItem<ItemType>> if options.returnRankInfo === true. Could you do that?
Either that, or let's make a new matchSorterWithRankInfo method to keep things tidy with types.
There was a problem hiding this comment.
hey kent,
that is the job of the function overloading in the top, and seems to be working as intended for me:
Screen.Recording.2026-04-14.at.6.42.33.PM.mov
but i'm also down to expose a separate function if that would feel safer
There was a problem hiding this comment.
also works if the property isn't provided at all:
Screen.Recording.2026-04-14.at.6.43.44.PM.mov
There was a problem hiding this comment.
to coderabbit's point, if it's not a literal, it just defaults to T[] which could be misleading.
There was a problem hiding this comment.
k pushed a fix and now the types behave as you would expect:
For literal true: RankedItem[]
For literal false/undefined (existing usages): T[]
For a boolean value: RankedItem[] | T[] (rare case)
Screen.Recording.2026-04-14.at.6.52.37.PM.mov
This could still technically break someone's existing types.
If a consumer had a function that was wrapping up the matchSorter types, and were doing passthrough options like this:
// this would now return `T[] | RankedItem<T>[]` which would be a type-breaking change
function myWrapper<T>({items, query, options}: {
items: T,
query: string,
options: Parameters<typeof matchSorter<T>>[2],
}) {
return matchSorter(items, query, options);
}this would now return T[] | RankedItem<T>[] instead of just T[] and would break their typechecks.
However, if the consumer was using the proper MatchSorterOptions<T> type, they would be unaffected:
// this continue to work fine, with typescript declaring T[] as the return type
function myWrapper<T>({items, query, options}: {
items: T,
query: string,
options: MatchSorterOptions<T>
}) {
return matchSorter(items, query, options);
}So your call, @kentcdodds, on whether you think this would be a problem. Options are:
- keep the function overloading as working now in the PR with slight risk of breaking someone's types if they weren't using the MatchSorterOptions and were instead inferring types of the parameters and passing along. I think this would be a pretty rare case.
- move to separate functions entirely which would obviously eliminate the type problem altogether. but now you have two functions to export, and two is more than one.
There was a problem hiding this comment.
Thanks for looking at that. Let's just do two functions
There was a problem hiding this comment.
@kentcdodds it is done, i've split them into separate functions while share core logic. lmk what you think
justinwaite
force-pushed
the
jdw/ranked-metadata
branch
from
06b74f0 to
82dac06
Compare
justinwaite
force-pushed
the
jdw/ranked-metadata
branch
from
82dac06 to
289ffc0
Compare
|
🎉 This PR is included in version 8.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
2 participants
What:
Allow callers to opt into match metadata so they can use ranking details without reimplementing the sorter.
Why:
For cases when you want to use matchSort but need to save/extract information about how each item was ranked after matchSort runs.
How:
Added a new option called
returnRankInfoand an overload for matchSorter. WhenreturnRankInfois true, the return type is{item: T, ...rankInfo}[]but when false it is still justT[]Checklist:
Summary by CodeRabbit
New Features
Documentation
Tests
Refactor