Don't update query target metadata for updates #1063
Conversation
schmidt-sebastian
force-pushed
the
multitab-addupdate
branch
from
39e3203 to
d56990a
Compare
| } | ||
| }); | ||
| }); | ||
| return this.saveQueryData(transaction, queryData); |
There was a problem hiding this comment.
You will still potentially need to update the metadata. Once QueryCache starts tracking the highest sequence number, it is likely that an update will trigger a metadata write.
There was a problem hiding this comment.
I updated the PR to allow future Greg to set the sequence number as part of the setLastRemoteSnapshotVersion call. Let me know if that will work as is for GC.
I am also still accepting nominations for the API name voting challenge.
| transaction: PersistenceTransaction, | ||
| snapshotVersion: SnapshotVersion | ||
| highestListenSequenceNumber: number, | ||
| lastRemoteSnapshotVersion?: SnapshotVersion |
There was a problem hiding this comment.
Why is this optional? Do we ever not supply it?
There was a problem hiding this comment.
Local Store doesn't always call setLastRemoteSnapshotVersion. Do you still want to update the listen sequence number?
There was a problem hiding this comment.
I see. I do still want to update the sequence number. I guess optional is ok, although it looks like in that case we could also pass in the highest snapshot version and have it be a no-op. I don't feel too strongly either way though.
There was a problem hiding this comment.
We can refine the API when we start porting to Web, but I believe the highest snapshot version that you know of could be SnapshotVersion.MIN (unless you pass the version that you just got from getLastRemoteSnapshotVersion).
There was a problem hiding this comment.
Yeah, I meant the one you just read. But yeah, we can update later if need be.
This PR ports some of what I think I learned by reviewing Greg's latest Android PR to the Web. This save an additional read on every query update in Multi-Tab.
Greg - is this correct?