Make iteration query methods respect returned promises #1112
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This function is documented as allowing the callback to return a promise, and when the callback returns a promise we should wait for that promise to resolve before continuing, and stop iteration if the promise rejects. If the callback returned a promise, the previous implementation would return an array of promises, and run the callback on all of them. The new implementation wraps the returned value in `Promise.resolve`, coercing it into a promise, so that we can safely extract a value from it before pushing the value into the results array. Returning this promise in the `.each()` callback also provides the behavior of waiting / stopping based on the resolution of a promise from the callback.
This function is documented as allowing the callback to return a promise, and when the callback returns a promise we should wait for that promise to resolve before continuing, and stop iteration if the promise rejects. The previous implementation was a bit naive in that it would load the entire set of objects matching the query into memory, and then call `.reduce()` on it. The new implementation avoids this by applying the callback on each object in turn. This lets us then return the result of the callback to the `.each()` call, which handles waiting for promise resolution / rejection. Things here get a little tricky in that the behavior of `reduce` is special when no initial value is provided: we take the first item, don't pass it into the callback, and treat that item as the initial value. We need to keep that treatment, which entails a check for a special case: we're looking at the first item AND the initial value was undefined.
This function is documented as allowing the callback to return a promise, and when the callback returns a promise we should wait for that promise to resolve before continuing, and stop iteration if the promise rejects. When the callback returns a promise, the old behavior would have been to push the object into the results array, because a promise is a truthy value. Now we wait for the promise to resolve before checking the value.
Codecov Report
@@ Coverage Diff @@
## master #1112 +/- ##
==========================================
+ Coverage 92.24% 92.25% +0.01%
==========================================
Files 54 54
Lines 5222 5232 +10
Branches 1170 1172 +2
==========================================
+ Hits 4817 4827 +10
Misses 405 405
Continue to review full report at Codecov.
|
acinader
reviewed
Mar 11, 2020
This more closely mirrors the behavior of Array.reduce(), which throws a TypeError in this situation. Implementation notes: - We return a rejected promise instead of throwing synchronously, because we need to wait for the result set to materialize before we know if you're reducing on an empty set - The `.each()` callback is never invoked for an empty result set, so we need to have this empty check happen _after_ we've finished iteration. Fortunately we're already keeping track of the iteration index in this scope, so we can reuse that for this check. Thanks for the feedback @acinader!
acinader
approved these changes
Mar 11, 2020
There was a problem hiding this comment.
@noahsilas this is really great. Thank you! Hope you'll find other ways to contribute :).
|
@TomWFox Is this documented? |
|
@samuelantonioli They are documented in the API https://parseplatform.org/Parse-SDK-JS/api/2.12.0/Parse.Query.html#map |
This was referenced Mar 30, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
These methods (
ParseQuery.map,ParseQuery.filter, andParseQuery.reduce) are documented as behaving similarly toParseQuery.each(which they rely on under the hood): if the callback they are passed returns a promise, we should:These methods weren't living up to that contract. Here's a series of adjustments to each one so that they follow those guidelines.