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

Client: automatic data structure sync i.e. .onValue(...) #76

Closed
wants to merge 3 commits into from
Closed

Client: automatic data structure sync i.e. .onValue(...) #76

wants to merge 3 commits into from

Conversation

marshall007
Copy link
Contributor

Implements #52. I went ahead and did this because in order to implement the pagination API (#31), we need to keep track of changefeed values to compute the page boundaries. There are a few open questions:

  1. For ordered feeds, should onValue preserve sorting? Probably, and this is necessary for pagination anyway, but Subscription currently knows nothing about the underlying query.
  2. Should onValue subscribers be notified while we're receiving initial values? Tentatively, my vote would be no, but that's not how it's currently implemented.
  3. The logic is not very expensive, but it might be nice to avoid it if there are no onValue subscribers. The problem would be if you attach a subscription.onValue(...) listener later on, the internal data structure wouldn't necessarily contain all/any initial values.
  4. onValue currently emits a shallow copy of it's internal array. Does this need to be a deep copy? That will be more expensive, so it's something to think about.

@marshall007 marshall007 changed the title Client: automatic data structure sync (subscription.onValue(...)) Client: automatic data structure sync i.e. .onValue(...) Jan 13, 2016
@marshall007
Copy link
Contributor Author

While testing this I updated the vue-todo-app example see marshall007/fusion@293d66a as something to poke around with.

@marshall007
Copy link
Contributor Author

Oh and one other thing, onValue listeners are called with (rows, index, row) meaning respectively: current state, position of the row that was just removed/changed, and the row that caused this change to be emitted.

@deontologician deontologician self-assigned this Jan 14, 2016
@deontologician
Copy link
Contributor

I'm in the process of something similar and I've run up against the same questions:

For ordered feeds, should onValue preserve sorting? Probably, and this is necessary for pagination anyway, but Subscription currently knows nothing about the underlying query.

I think it needs to know the sorting from the query. The current implementation of .order.limit changefeeds assumes we keep the collection ordered on the client, it doesn't provide a way to know where the changes go in the resulting set. The huge issue with this is that we have to be able to sort exactly the same way the server does, which isn't anything like how javascript sorts things natively. I was in the process of implementing this sort for the client

Should onValue subscribers be notified while we're receiving initial values? Tentatively, my vote would be no, but that's not how it's currently implemented.

I was leaning towards no. The idea with the onValue / values observable is that it takes care of the low-level details and just tells you what the resultset looks like. It's actually fine if the array gradually fills up to the size limit and your app renders things as they come in.

The problem would be if you attach a subscription.onValue(...) listener later on, the internal data structure wouldn't necessarily contain all/any initial values.

I was leaning towards not tacking it onto the subscription for this reason. Since this is inherently stateful, you really need to be able to ensure you've got the full results, and as you said, you don't want to save the state just in case the user needs it later. My thought was to add a method to .observe (still not on next :/) that gives you an observable using scan to give you the entire ordered results on every change emitted.

onValue currently emits a shallow copy of it's internal array. Does this need to be a deep copy?That will be more expensive, so it's something to think about.

There are a good number of libraries around nowadays that use the convention of documenting that data structures should be considered immutable, but not enforcing it by being defensive with freeze or deep copies for efficiency's sake. I think that's a reasonable strategy to take with these things

@deontologician
Copy link
Contributor

I think for the time being, I'm going to leave this unmerged, but I realize you need it or something like it for implementing the pagination api. My suggestion is to work with this on your private branch, and when I finish the observable state sync stuff, we can figure out how to put the pagination stuff on top of it

@deontologician
Copy link
Contributor

This has now been implemented in #102

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

Successfully merging this pull request may close these issues.

None yet

2 participants