fix: Type regressions in ascending/descending fields - #2919
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. |
📝 WalkthroughWalkthroughTightened ParseQuery type signatures to include the special full-text ranking key Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 99.98% 99.98%
=======================================
Files 64 64
Lines 6339 6339
Branches 1522 1538 +16
=======================================
Hits 6338 6338
Misses 1 1 ☔ 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;
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/ParseQuery.ts (1)
1771-1784:⚠️ Potential issue | 🟡 MinorAlign
addAscendingwithascendingfor'$score'support.
ascendingaccepts'$score', but it delegates toaddAscending, whose public signature rejects'$score'. This creates an avoidable API/type inconsistency for equivalent runtime behavior.Suggested fix
- 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 {Use this read-only verification to confirm the signature gap and test coverage status:
#!/bin/bash # Verify '$score' typing across sorting methods and type tests. rg -n "ascending<|addAscending<|descending<|addDescending<|select<" src/ParseQuery.ts types/ParseQuery.d.ts rg -n "\$score" types/tests.ts -C2 rg -n "addAscending\\('\\$score'\\)|addDescending\\('\\$score'\\)" types/tests.ts -C2🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ParseQuery.ts` around lines 1771 - 1784, addAscending's generic signature currently excludes '$score' while ascending accepts it and delegates to addAscending, causing a typing mismatch; update addAscending (and symmetrically addDescending if present) to include '| '$score'' in its generic constraint (match ascending/descending signatures) so both runtime and type APIs align, then run the provided grep/type-test commands to verify '$score' is accepted in addAscending/addDescending and tests/types compile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/ParseQuery.ts`:
- Around line 1771-1784: addAscending's generic signature currently excludes
'$score' while ascending accepts it and delegates to addAscending, causing a
typing mismatch; update addAscending (and symmetrically addDescending if
present) to include '| '$score'' in its generic constraint (match
ascending/descending signatures) so both runtime and type APIs align, then run
the provided grep/type-test commands to verify '$score' is accepted in
addAscending/addDescending and tests/types compile.
|
I think this is ready to merge |
|
@coderabbitai resume |
✅ Actions performedReviews resumed. |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/ParseQuery.ts (1)
1830-1869: Optional: document that$scoreis intentionally excluded from descending.The descending pair correctly excludes
'$score'from its key constraint (full-text ranking is only meaningful in ascending order for this API). Consider a one-line JSDoc note mirroring theascendingdoc so consumers understand this is intentional rather than an omission, e.g.:📝 Proposed JSDoc clarification
/** * Sorts the results in descending order by the given key. - * + * Note: `$score` is not supported here; full-text score ranking must use `ascending('$score')`. * `@param` {(string|string[])} keys The key to order by, which is a * string of comma separated values, or an Array of keys, or multiple keys.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/ParseQuery.ts` around lines 1830 - 1869, Add a one-line JSDoc note to the descending (and/or addDescending) method(s) clarifying that '$score' is intentionally excluded from the allowed keys because full-text ranking only applies in ascending order for this API; update the comment block above descending (and mirror in addDescending if present) to reflect this intentional behavior so consumers don't think it's an omission, referencing the descending and addDescending methods.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/ParseQuery.ts`:
- Around line 1830-1869: Add a one-line JSDoc note to the descending (and/or
addDescending) method(s) clarifying that '$score' is intentionally excluded from
the allowed keys because full-text ranking only applies in ascending order for
this API; update the comment block above descending (and mirror in addDescending
if present) to reflect this intentional behavior so consumers don't think it's
an omission, referencing the descending and addDescending methods.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8fc96cb5-38ab-461e-b8e6-99e5c84b0b5b
📒 Files selected for processing (2)
src/ParseQuery.tstypes/ParseQuery.d.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- types/ParseQuery.d.ts
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