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.
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
feat(datastore): Dispatch outboxStatus, subscriptionsEstablished, syncQueriesStarted events #721
feat(datastore): Dispatch outboxStatus, subscriptionsEstablished, syncQueriesStarted events #721
Changes from 20 commits
19ade20
ee22220
5c578a9
e59be63
e6db87a
beaf801
2fc86e6
c12e462
6d27bad
0a03b27
d222390
f2263af
e4315f7
9de6be5
9c5f606
d962a58
2a91473
de6e778
effabe9
fe6fb22
720cbd9
f534255
7645ba2
9c88647
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Clarify wording: "Dispatched when: 1) the DataStore starts; 2) each time a local mutation is enqueued into the outbox; 3) each time a local mutation is finished processing."
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.
Updated
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.
"Dispatched when DataStore has finished establishing its subscriptions to all syncable models"
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.
Updated
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.
"Dispatched when DataStore starts establishing its subscriptions to all syncable models"
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.
For
syncQueriesStarted
, it is notDispatched when DataStore starts establishing its subscriptions to all syncable models
but isDispatched when DataStore starts to sync Queries
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.
Unnecessary import
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.
Removed
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.
"
true
if there are no events in the outbox at the time the event was dispatchedThere 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.
Unnecessary import
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.
Removed
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.
"A list of all model names for which DataStore has started establishing subscriptions"
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.
Updated
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.
How do you know the outbox is empty here? Shouldn't you be querying the mutation events from storage, or checking the
operationCount
of the outbox's operation queue?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.
Right, it makes sense.
Incorrectly, I thought because
subscription.request(.max(1))
ensure the queue to have only onemutationEvent
at a time so I can assert queue is empty when the event is finish processing. I miss the fact that events are stored inside the mutationEventDatabase.Updated.
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.
As i understand it, we were emitting outboxStatus events to this logic (this is probably wrong then)
isEmpty
isEmpty
to false since we know we are processing oneoutboxStatus
event withisEmpty = true
example output
outBoxStatus - isEmpty = false - on start, check mutationEvent from storage, say there 5 mutation events
outboxStatus - isEmpty = false - queue mutationEvent 1
outboxStatus - isEmpty = true - finish processing mutationEvent 1
outboxStatus - isEmpty = false - queue mutationEvent 2
outboxStatus - isEmpty = true - finish processing mutationEvent 2
outboxStatus - isEmpty = false - queue mutationEvent 3
outboxStatus - isEmpty = true - finish processing mutationEvent 3
outboxStatus - isEmpty = false - queue mutationEvent 4
outboxStatus - isEmpty = true - finish processing mutationEvent 4
outboxStatus - isEmpty = false - queue mutationEvent 5
outboxStatus - isEmpty = true - finish processing mutationEvent 5
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.
or is the expected behavior just a ping of the outbox status? for example
outBoxStatus - isEmpty = false - on start, check mutationEvent from storage, say there 5 mutation events
outboxStatus - isEmpty = false - queue mutationEvent 1
outboxStatus - isEmpty = false - finish processing mutationEvent 1
outboxStatus - isEmpty = false - queue mutationEvent 2
outboxStatus - isEmpty = false - finish processing mutationEvent 2
outboxStatus - isEmpty = false - queue mutationEvent 3
outboxStatus - isEmpty = false - finish processing mutationEvent 3
outboxStatus - isEmpty = false - queue mutationEvent 4
outboxStatus - isEmpty = false - finish processing mutationEvent 4
outboxStatus - isEmpty = false - queue mutationEvent 5
outboxStatus - isEmpty = true - finish processing mutationEvent 5
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.
@lawmicha the second behavior is the expected one.
For reference, this is the JS implementation:
https://github.com/aws-amplify/amplify-js/blob/a7feacea4ed506340d250249d0b15286fe3ef5fa/packages/datastore/src/sync/index.ts#L358-L365
It checks the queue
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.
s/quering/querying/
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.
updated