partial request deduping #897
Merged
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
dedupes 2 major cases: paths followed via refs in cache, and paths with ranges in the end remaining: paths with more properties after the range, and not followed via refs in client cache
about sending requests in order (cannot make them sequential, must be parallel)
jhusain
approved these changes
Oct 17, 2017
test/data/LocalDataSource.js
Outdated
| if (wait > 0) { | ||
| setTimeout(exec, wait); | ||
| } else { | ||
| if (wait === false) { |
There was a problem hiding this comment.
Agree with the decision to make "0" mean setTimeout(0). However would've used null/undefined to indicate no wait. That way the member could just be optional, and if not included, there would be no async.
There was a problem hiding this comment.
thanks! points from discussion with @jhusain :
it did already was no wait if you missed the property (because wait gets defaulted to false earlier), but that did leave out the case where wait is explicitly sent in as undefined. agreed that missing the property should behave the same as setting it to undefined, so now:
using undefined to indicate no wait. not caring about null or other possible types at this point (this is test helper code).
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.
update:
pull request is ready for review and merging! all edge cases covered.
update:
pull request is up for review now. all edge case bugs fixed. adding some tests now.
dedupes 2 major cases: paths followed via refs in cache, and paths with ranges in the end
pull request is still in progress, but still want to get opinion on the approach
work remaining: dedupe paths with more properties after the range, and not followed via refs in client cache (like
['videosById', { from: 0, to: 10 }, 'name' ]wherevideosByIdis not in cachefixes #779