-
Notifications
You must be signed in to change notification settings - Fork 615
Port structural changes from Multi-Tab #84
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
Conversation
6cbe2c9
to
8f6c8cc
Compare
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.
LGTM with optional mumble. Thanks for going back and doing this!
viewChanges, | ||
fromCache, | ||
mutatedKeys, | ||
true); |
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.
mumble mumble
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.
rumble
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.
Note that this changed the unit tests quite a bit, as Web doesn't verify the excludeMetadataChanges
flag. Time for a backport.
btw, if you merge the latest master there should be a slightly higher chance CI will pass. |
37ad13d
to
ed71aff
Compare
ed71aff
to
56ea93a
Compare
/test connected-check |
@schmidt-sebastian: The following test failed, say
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
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.
I wasn't expecting you to fix the excludesMetadataChanges stuff. Thanks!
/test connected-check |
This PR contains a port of the structural changes from the Multi-Tab PR (firebase/firebase-js-sdk#1000)
It's might be worth discussing just getting rid of the TargetIdGenerator at this point...