fix: Type regressions in ascending/descending fields#2919
fix: Type regressions in ascending/descending fields#2919swittk wants to merge 3 commits intoparse-community:alphafrom
Conversation
|
🚀 Thanks for opening this pull request! |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
📝 WalkthroughWalkthroughSignatures for ParseQuery sorting and selection methods were narrowed to generics constrained to model attributes and base attributes; Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 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.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ParseQuery.ts (1)
1566-1570:⚠️ Potential issue | 🟠 Major
sortByTextScorewill fail to compile with the newascendingconstraint.
'$score'is not a member ofkeyof T['attributes'] | keyof BaseAttributes, sothis.ascending('$score')on line 1567 will produce a type error after this change. Theselectcall already works around this withas any, butascendingdoes not.Proposed fix
sortByTextScore() { - this.ascending('$score'); + this.ascending('$score' as any); this.select(['$score'] as any); return this; }
🤖 Fix all issues with AI agents
In `@types/ParseQuery.d.ts`:
- Line 737: The new key constraint on
ascending/descending/addAscending/addDescending is too strict and breaks
sortByTextScore(), which calls this.ascending('$score'); relax the signatures in
types/ParseQuery.d.ts (the ascending, descending, addAscending, addDescending
declarations) to allow special string keys (either include | string or
explicitly include the literal '$score') in addition to keyof T['attributes'] |
keyof BaseAttributes so calls like this.ascending('$score') compile without
casts; update the declarations to accept (K | string | K[])[] or add an overload
that accepts string | string[] to preserve type safety for attribute keys while
supporting '$score'.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## alpha #2919 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 64 64
Lines 6235 6235
Branches 1493 1493
=========================================
Hits 6235 6235 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
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)
types/ParseQuery.d.ts (1)
747-764:⚠️ Potential issue | 🟡 Minor
'$score'is missing fromaddAscending,descending, andaddDescendingconstraints.
ascendingandselectinclude'$score'in their generic constraint, but the other three sort methods do not. A user who writesquery.addAscending('$score')orquery.descending('$score')will get a type error, even though the runtime accepts it. If the omission is intentional (only theascending('$score')pattern is officially supported), a brief JSDoc note on these methods would help; otherwise, add| '$score'for consistency.Proposed fix for consistency
- addAscending<K extends keyof T['attributes'] | keyof BaseAttributes>(...keys: (K | K[])[]): this; + addAscending<K extends keyof T['attributes'] | keyof BaseAttributes | '$score'>(...keys: (K | K[])[]): this; ... - descending<K extends keyof T['attributes'] | keyof BaseAttributes>(...keys: (K | K[])[]): this; + descending<K extends keyof T['attributes'] | keyof BaseAttributes | '$score'>(...keys: (K | K[])[]): this; ... - addDescending<K extends keyof T['attributes'] | keyof BaseAttributes>(...keys: (K | K[])[]): this; + addDescending<K extends keyof T['attributes'] | keyof BaseAttributes | '$score'>(...keys: (K | K[])[]): this;
| this.select(['$score'] as any); | ||
| this.select('$score'); |
There was a problem hiding this comment.
Could you give more context about why this changed from Array<string> to string? Parse.Query.select usually takes an array, but from the description I understand it also accepts a single string with comma separation. I think we should remove this type ambiguity and make select only accept an array of strings. If you agree we could pin the change for the next major release. We should then keep using the array type wherever possible because we'll have to revert to it.
There was a problem hiding this comment.
Sorry for the late reply; there isn't really any need for this change (it could be just ['$score'] the same as before), but there's really no real behavioural difference underneath I think; internally it just calls Array.isArray(key) check before, but then it does the key.join() immediately after + does a split(',') afterwards, so internally it supports all 3 types I mentioned (single array of keys, keys as string args, keys as string with comma splitting), and even likely supports a mix-and-match of all 3 types as multiple args due to how it's implemented in the addAscending and addDescending call sites.
The typings I think what people are definitely using (at least in my team) are:
.addAscending('key1', 'key2')(the variadic arguments version)- or the
.addAscending(['key1', 'key2'])(the array args version)
I don't recall seeing the comma separated string thing documented, but it's definitely working and has been there a long time. I don't think we should remove this to be honest. I do have an idea for how to make the comma-separated typed multiple keys thing work strongly typed, but I was thinking of doing a separate PR for this since it seemed like a bit of TS magic. Just making keys typed the conventional way seemed straightforward.
There was a problem hiding this comment.
@mtrezza sorry for the ping; do you have any suggestions to what I should edit?
3 participants
Pull Request
Issue
Typings for
ascending,descending,addAscending, andaddDescendingin the current version are typed as...string[], which misses the original object key checks which were possible in the original DefinitelyTyped typings.This change allows for the autocompletion & key inference type checks to work once again (this includes base attributes such as
objectId,createdAt,updatedAttoo).The comma-string keys (which are technically supported, JS-wise, but were not supported in the original DefinitelyTyped typings) are not included in this scope yet, but should be possible to add in a future PR by using modern features.
Approach
Used key inference in the same fashion as in the prior DefinitelyTyped typings.
Tasks
Summary by CodeRabbit