Core Data: Add batched variants for start and finish resolution actions #31005
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
fluiddot
force-pushed
the
add/start-finish-batch-resolutions
branch
from
4f266e3 to
a40911a
Compare
|
Size Change: +146 B (0%) Total Size: 1.46 MB
ℹ️ View Unchanged
|
gziolo
reviewed
Apr 21, 2021
| * | ||
| * @return {Object} Action object. | ||
| */ | ||
| export function startResolutions( selectorName, args ) { |
There was a problem hiding this comment.
Would it be possible to refactor existing actions (finishResolution and finishResolution) to accept not only a single resolution but also multiple resolutions when necessary? This way we wouldn't have to create new methods.
There was a problem hiding this comment.
How would we determine what the caller intends to do? args is already an array, and for finishResultions it's an array of arrays.
In my view two separate functions are easier to understand. And easier to provide types for.
There was a problem hiding this comment.
I thought about that option but I didn't want to overuse the args parameter, besides since args can be anything it can be hard to infer when it's being dispatched with single/multiple resolutions. One option we might consider is to add an extra boolean parameter that specifies if the args contain single/multiple resolutions.
There was a problem hiding this comment.
Right, it's very hard to detect whether it's single or multiple items because args can be any number of params of any type.
In startResolutions and finishResolutions we should use a different name than args to distinguish between single and multiple items in both functions. The type is going to be more complex as well.
I think it's better to add new functions if the only alternative is a boolean param.
|
Thanks @fluiddot for working on this. The PR is simple and straightforward and I think it's OK to merge. Let's discuss the API tradeoffs. |
|
I had only minor comment related to documentation: * @param {...*} args Array of arguments to associate for uniqueness, each item
* is associated to a resolution.It applies for both methods Otherwise, you can proceed, nice refactor 👍🏻 |
gziolo
approved these changes
Apr 22, 2021
gziolo
changed the title
gziolo
added
[Type] New API
I'm ok to rephrase the documentation of these actions but I'm wondering if it's necessary in case we consider this param the same as the argument object of JS functions. If this is the case, I think it's implicit that this param is an array, what do you think?
Sure! I'm thinking to rename it to |
|
@fluiddot, I'm leaving it up to you to decide. Technically speaking |
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.
Description
Related to issue: #29928
Batched variants have been added for start (
START_RESOLUTION) and finish (FINISH_RESOLUTION) resolution actions. These actions will be useful when we want to resolve a selector with multiple results in one dispatch, like the case exposed in thegetEntityRecordsresolver, which has also been updated to use these new actions.How has this been tested?
I tested these changes by following the same test instructions described on the PR that introduced the start and finish resolution actions in the
getEntityRecordsresolver.Screenshots
N/A
Types of changes
New feature
Checklist:
*.native.jsfiles for terms that need renaming or removal).