-
Notifications
You must be signed in to change notification settings - Fork 76
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
Make adapter call a hook when encountering a change for a record that is not yet loaded. #108
Make adapter call a hook when encountering a change for a record that is not yet loaded. #108
Conversation
* If you want to change this, subclass this method, and push the data into the store. e.g. | ||
* | ||
* let store = this.get('store'); | ||
* let recordTypeName = this.getRecordTypeName(this.store.modelFor(obj.type)); |
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.
store.modelFor(obj.type)
without this
?
…a record that is not yet loaded. - The hook is no-op by default, to match present behavior and performance profile - Added a test demonstrating use of the hook to load the new data into the ember-data store
a7569e7
to
58e3e81
Compare
Hmmm, reading this and trying to understand it (forgive me; I am really weak on EmberData specifics, although I grok Pouch and relational-pouch 😃). I guess what I'm wondering is why we have this early return in the first place. Doesn't this mean that changes from a remote CouchDB will never be loaded into the Ember store? Why don't we just make your commented code the default behavior? (Side note: it seems like we have a very robust syncing system between PouchDB and CouchDB, but a rather weak one between PouchDB and the in-memory store. In particular, the fact that this syncing logic isn't taking |
@nolanlawson here is the discussion: #50 (comment) |
@nolanlawson @fsmanuel yes, my understanding based on the README and the referenced thread was that pushing every new record into the store on arrival was an undesirable for performance reasons. We have a low-volume use case and wanted to make it easier to implement that approach. |
@nolanlawson, I know you're busy so here's a summary: the ember-data store is the in-memory cache of records the application has touched during the current session. For records which are already cached, our default change watcher updates the cached copy when they change and removes them from the cache when they are removed from PouchDB. It does not automatically cache new records that arrive because that causes terrible performance during the initial sync with medium-and-up data sizes. (Re: revisions. The change watcher does not update cached records unless ember-data's state machine believes that the cached record is up to date with PouchDB; see this line. If an in-memory copy has been modified but not saved and simultaneously a sync brings in a new rev of that record, the application's attempt to save it will cause a conflict. The application can then resolve the conflict as it sees fit. (Similarly for |
@lukemelia / @chrislopresto: thanks for this. I have an idea about how to make the adapter act more naturally in this area, but I haven't had a chance to implement/propose it yet. Your change would not interfere with what I'm thinking of and it looks like a good interim step. |
Thanks for the summary @rsutphin. With respect to the discussion of conflict handling. This seems like it could also be initially addressed by calling a hook with the relevant details that has no-op behavior. i.e. but the power in the hands of the end-user while providing conservative defaults. |
@rsutphin Ah okay, that makes a lot more sense then. I look forward to the more complete in-memory mirroring solution (I take it something like this would reproduce exactly the performance problem you describe, for the initial load of a large database?), but for now, if you're +1 on this PR, then I am as well. I'll let you merge it when you're ready. :) |
👍 is there anything that needs to be done to get this merged? |
I just wanted to give other contributors time to look at it. I'm +1; I'll merge later today when I get a chance. |
Make adapter call a hook when encountering a change for a record that is not yet loaded.
Pouch adapter now calls a hook method when encountering a change for a record that is not yet loaded.
@lukemelia / @chrislopresto