Skip to content
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

fix(database): retrieve initial list content once #820

Merged
merged 2 commits into from
Feb 15, 2017

Conversation

cartant
Copy link
Contributor

@cartant cartant commented Feb 10, 2017

Closes #819

Checklist

Description

This PR simplifies the list implementation so that only the reducers associated with the child_added/remove/changed events manipulate the list's internal array. The value event is only used to determine when the initial load has been done.

The on('value') and on('child_added', ...) calls are made within the same turn through the event loop, so only a single copy of the initial content is retrieved.

The comments made in the commit that introduced this retrieve-the-initial-content-twice behaviour mention SDK quirks and nuances. By using only the child_XXX events and by not worrying whether the child_added event adds the last key before or after the value event fires, this PR's implementation should be sufficiently robust to cope with said quirks and nuances.

@davideast
Copy link
Member

Your approach is much simpler! Do know though, that the data is still only retrieved once over the network. The second set of listeners retrieve from the cache.

@cartant
Copy link
Contributor Author

cartant commented Feb 11, 2017

I agree that it would make sense for the second set of listeners to receive cached data, but why does it show up in the WebSocket traffic in the Dev Tools? Is this an SDK issue? Is it not using the cache it ought to be?

@cartant
Copy link
Contributor Author

cartant commented Feb 11, 2017

Actually, I suspect the second set of listeners would receive cached data if the first listener was attached with on, but it's attached with once. So perhaps the first listener is already 'disconnected' (so to speak) by the time the promise resolves and the second set of listeners establish a new 'connection' - which sees them retrieve data via the WebSocket?

@cartant
Copy link
Contributor Author

cartant commented Feb 13, 2017

@davideast I've had a bit of a poke around with this and there's another plunk here that demonstrates the differences in behaviour between overlapping and non-overlapping listeners.

It seems that if the listeners don't overlap, data for the second listener is re-retrieved via the WebSocket - which is the behaviour that I see with the current FirebaseListObservable and its initial content.

@davideast
Copy link
Member

davideast commented Feb 15, 2017

@cartant I went and spoke with a member on the Database team and your suspicions are correct. A once() will force a network call each and every time because internally it calls .off() which "disconnects". While on() calls will attempt to read from the cache if available because it is an active listener.

This should PR should fix these issues. Thank you :)

@davideast davideast merged commit 5c5ff7b into angular:master Feb 15, 2017
@cartant
Copy link
Contributor Author

cartant commented Feb 16, 2017

@davideast Thanks for looking further into it. What you say makes sense and fits with the behaviour I see. I'm looking forward to the performance improvements that should come with the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants