-
-
Notifications
You must be signed in to change notification settings - Fork 600
fix: Parse.Query.findAll returns empty array calling withCount
#2854
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: alpha
Are you sure you want to change the base?
Changes from 1 commit
84c8526
a880e85
42bddd6
3cfda88
fb6fc02
d2c1bc9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -675,7 +675,7 @@ class ParseQuery<T extends ParseObject = ParseObject> { | |||||||||||||||||||||||||||||||
| * @returns {Promise} A promise that is resolved with the results when | ||||||||||||||||||||||||||||||||
| * the query completes. | ||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||
| find(options?: QueryOptions): Promise<T[]> { | ||||||||||||||||||||||||||||||||
| find(options?: QueryOptions): Promise<T[] | { results: T[]; count: number }> { | ||||||||||||||||||||||||||||||||
| const findOptions = ParseObject._getRequestOptions(options); | ||||||||||||||||||||||||||||||||
| this._setRequestTask(findOptions); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
|
|
@@ -736,12 +736,15 @@ class ParseQuery<T extends ParseObject = ParseObject> { | |||||||||||||||||||||||||||||||
| * @returns {Promise} A promise that is resolved with the results when | ||||||||||||||||||||||||||||||||
| * the query completes. | ||||||||||||||||||||||||||||||||
| */ | ||||||||||||||||||||||||||||||||
| async findAll(options?: BatchOptions): Promise<T[]> { | ||||||||||||||||||||||||||||||||
| let result: T[] = []; | ||||||||||||||||||||||||||||||||
| async findAll(options?: BatchOptions): Promise<T[] | { results: T[], count: number}> { | ||||||||||||||||||||||||||||||||
| let results: T[] = []; | ||||||||||||||||||||||||||||||||
| await this.eachBatch((objects: T[]) => { | ||||||||||||||||||||||||||||||||
| result = [...result, ...objects]; | ||||||||||||||||||||||||||||||||
| results = [...results, ...objects]; | ||||||||||||||||||||||||||||||||
| }, options); | ||||||||||||||||||||||||||||||||
| return result; | ||||||||||||||||||||||||||||||||
| if (this._count) { | ||||||||||||||||||||||||||||||||
| return { results, count: results.length }; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
| return results; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
Comment on lines
+739
to
748
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
If 🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
| /** | ||||||||||||||||||||||||||||||||
|
|
@@ -930,10 +933,14 @@ class ParseQuery<T extends ParseObject = ParseObject> { | |||||||||||||||||||||||||||||||
| return !finished; | ||||||||||||||||||||||||||||||||
| }, | ||||||||||||||||||||||||||||||||
| async () => { | ||||||||||||||||||||||||||||||||
| const [results] = await Promise.all([ | ||||||||||||||||||||||||||||||||
| const [response] = await Promise.all([ | ||||||||||||||||||||||||||||||||
| query.find(findOptions), | ||||||||||||||||||||||||||||||||
| Promise.resolve(previousResults.length > 0 && callback(previousResults)), | ||||||||||||||||||||||||||||||||
| ]); | ||||||||||||||||||||||||||||||||
| let results: any = response; | ||||||||||||||||||||||||||||||||
| if (results.results) { | ||||||||||||||||||||||||||||||||
| results = results.results; | ||||||||||||||||||||||||||||||||
| } | ||||||||||||||||||||||||||||||||
|
Comment on lines
+936
to
+943
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unnecessary Two concerns here:
Proposed fix const query = ParseQuery.fromJSON(this.className, this.toJSON());
+ query._count = false;
query.ascending('objectId');
query._limit = options.batchSize || 100;And for the unwrapping: - let results: any = response;
- if (results.results) {
- results = results.results;
- }
+ const results: T[] = Array.isArray(response)
+ ? response
+ : (response as { results: T[] }).results;📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||||||||||||
| if (results.length >= query._limit) { | ||||||||||||||||||||||||||||||||
| if (findOptions.json) { | ||||||||||||||||||||||||||||||||
| query.greaterThan('objectId', (results[results.length - 1] as any).objectId); | ||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||
Uh oh!
There was an error while loading. Please reload this page.
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.
Union return type is a breaking change for TypeScript consumers.
Changing
find()fromPromise<T[]>toPromise<T[] | { results: T[]; count: number }>forces every caller to narrow the type before accessing array methods. This was already flagged in the PR discussion by@mtrezzaas conceptually problematic and inconsistent across SDKs. Consider a separate method (e.g.,findWithCount()) or always returning a wrapper to avoid a discriminated union.🤖 Prompt for AI Agents
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.
I think we could actually introduce a method
findWithCount()for this, to not mess up the types while providing some relieve for developers - what do you think @dplewis?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.