ember-pouch live updating stopped working

[broerse]

On: https://github.com/broerse/ember-cli-blog/tree/master/app/adapters the live updating stopped working. If you now change something in one browser you don't see the change until you switch to an other route and back. I tested with ember-pouch 1.2.3 and 1.2.4 and ember-data beta15 & beta16 but snapshots don't seem to be the problem.

[broerse]

Update:

• When a new record is created in browser 1 it is synced to the remote CouchDB server. It is not synced back to Ember-data from browser 2. (You can see this in the ember inspector) It is synced back to the IndexDB from browser 2.
• When you then create a record in browser 2 this is also synced to the remote CouchDB server but the record from step 1 does not show up in Ember-Data but shows up in the IndexDB.

So it seems syncing between Pouch and Ember-Data is not working 100% anymore.

[rsutphin]

@broerse, I think you might be seeing a side-effect of #43. That PR changed the default change watcher so that it only updates records that are already loaded into ember-data's store (i.e., with a find). It no longer loads every record it sees into the store. (This is necessary to support replicating with large databases where you don't need all the data available to Ember immediately.)

If you do want to have every record (or every record of a particular type) automatically loaded as it comes in, I suggest adding a change watcher that's specific to your app's needs.

[broerse]

@rsutphin Perhaps but it breaks how it worked before. Perhaps we need a default change watcher that does work like it was before and extend if we want something else. (Just like Ember ;-)) I understand why this is basically a good idea.
I think most people starting with ember-pouch want sync. like firebase and don't have large databases. Can you show me how to add the "old" behavior change watcher?
@nolanlawson do we need a major version update and some doc's for this break?

[nolanlawson]

My bad, I didn't realize it was a breaking change. It's already published, so that ship has kind of sailed 😓.

Looking at #43, I don't really understand how it introduced a breaking change. In both case, we are doing changes({since: 'now'}), which means that it shouldn't trigger for pre-existing documents. @rsutphin I also don't quite follow your explanation.

In general I think the userbase for ember-pouch is low enough that we can feel comfortable moving fast and breaking things. Although if you both think it's serious, I can unpublish from npm.

@broerse can you rearchitect your app to use the new system, or do you absolutely positively need the old functionality?

[broerse]

On my phone. No problem breaking things. I can comment tomorrow.

[rsutphin]

One person's feature is another person's bug, I suppose. With the old change watcher, an ember-pouch-using application would become nonresponsive for minutes or longer when you replicated in more than a thousand records or so. That seemed like undesirable default behavior.

I agree that we should add documentation for how to write a change watcher that automatically finds newly added records. Here's a sketch of one (not tested):

// Put in your subclass of ember-pouch's adapter 
immediatelyLoadAllChangedRecords: function() {
  this.db.changes({
    since: 'now',
    live: true,
    returnDocs: false
  }).on('change', function (change) {
    var obj = this.db.rel.parseDocID(change.id);
    // skip changes for non-relational_pouch docs. E.g., design docs.
    if (!obj.type || obj.type === '') { return; }
    var store = this.container.lookup('store:main');
    store.find(obj.type);
  }
}.on('init')

@nolanlawson, the difference is in how the pre- and post-#43 change watchers interact with ember-data's in-memory store. The ember-data store is the place where the objects that are bound into controllers/views/etc reside. Changes to objects in the store are automatically reflected in UIs that use their attributes. The store caches any object it loads indefinitely, and will not go back to the adapter for it unless explicitly directed (via store.fetch). (It helped me when trying to grok this to remind myself that ember-data's main use case is for dealing with remote APIs, so it makes different tradeoffs than traditional ORMs.) Records which are added to PouchDB are not automatically added to the store by ember-data.

In the pre-#43 change watcher, as soon as a PouchDB record was seen to have changed, all records of that type would be retrieved and put into the ember-data store (ref). In the post-#43 change watcher, only records that are already in the store are updated (ref). The post-#43 behavior allows the application to control which records are loaded and when, which is important for performance with large datasets and/or slow browsers (e.g., mobile).

Apologies if that's still unclear. I'm happy to answer other questions.

[broerse]

@rsutphin I can't get this working. Is it possible to do a PR on https://github.com/broerse/ember-cli-blog ?
@nolanlawson why not make a storesync flag for both scenarios db.sync(remote, {live: true, retry: true, storesync: true }); to hide this relatively complex fixing, for default behavior, from beginners like me. Or keep the old behavior and let non beginners subclass adapters ;-)

[rsutphin]

Sorry you're having trouble, @broerse. I'm happy to try to answer questions, but I don't have time to try to set up your application myself to fix it. What seems to be the problem?

why not make a storesync flag for both scenarios db.sync(remote, {live: true, retry: true, storesync: true });

db.sync is part of PouchDB, not ember-pouch itself, so I don't think a fix could go there. I think that maybe having an option in the adapter to enable this behavior would be worthwhile, but I also don't know how useful it is to have this sort of automatic loading of everything all the time. It would really only work well in testing/demo-scale datasets, then break when you got to production sizes, requiring you to go back and update everything to only load the data you need. Seems better to do that up front.

Or keep the old behavior and let non beginners subclass adapters ;-)

I know that if I had seen the pre-#43 behavior when I was a beginner, I would have probably dropped PouchDB instead of trying to fix it. It was just an accident of the way our application was configured (we had accidentally prevented the change watcher from running) that I didn't run into the terrible replication performance until I had been using ember-data, ember-pouch, and PouchDB for a while. At that point I had enough experience with all of them to find and fix the problem.

FWIW, you are already subclassing the adapter. You have to subclass it to set the db property. So you're closer than you think.

[broerse]

When new records come in I get Uncaught TypeError: Cannot read property 'lookup' of undefined
Perhaps 'store:main' is not correct?

[rsutphin]

Sounds like this.container is undefined. I see the problem — try this instead:

immediatelyLoadAllChangedRecords: function() {
  this.db.changes({
    since: 'now',
    live: true,
    returnDocs: false
  }).on('change', function (change) {
    var obj = this.db.rel.parseDocID(change.id);
    // skip changes for non-relational_pouch docs. E.g., design docs.
    if (!obj.type || obj.type === '') { return; }
    var store = this.container.lookup('store:main');
    store.find(obj.type);
  }.bind(this))
}.on('init')

Added .bind(this)) on the 2nd to last line.

[broerse]

Thanks! This solved it. Will update my example today.

[broerse]

With @rsutphin 's help:

https://github.com/broerse/ember-cli-blog/blob/master/app/adapters/application.js

@nolanlawson Something like this should be in the docs. @lokeshj is working on this doc so perhaps wait for that.

[nolanlawson]

@broerse Awesome. Having your sample app linked from the docs is also a big help for people, I imagine.:)

[broerse]

I know this is more a Pouch thing but .on('change' also seems to fire when I change something myself. It would be nice if I can specify it to only fire when the change is coming from the remote and is not made by me. Is this already possible with Pouch?

[nolanlawson]

@broerse All changes go through the changes feed, regardless of source. This has been a big ask from users, but I've typically recommended just adding an "author" field (or something) to the documents and then manage that yourself.

[broerse]

"author" is not unique but I understand what you want. It is extra data for fixing something. Perhaps it is better to do some kind of "rev" check. If the rev ember-data knows is the same as the .on('change' we don't have to do anything.

[rsutphin]

If the rev ember-data knows is the same as the .on('change' we don't have to do anything.

This seems reasonable. Want to write a PR for it? The change would be around here:

https://github.com/nolanlawson/ember-pouch/blob/master/addon/adapters/pouch.js#L55-L60

[broerse]

@rsutphin I don't know if I can. I will try. For starters ember test does complete all test OK but on github everything is OK. How do I run tests? Or should I not have PhantomJS 1.9?

[rsutphin]

If the rev ember-data knows is the same as the .on('change' we don't have to do anything.

I made a new issue to track this: #81.

@broerse, you need PhantomJS 2.0 to run the tests. (PouchDB doesn't work with PhantomJS 1.9.) We should probably have better docs, but for now you can look at the travis.yml to see how the tests get run.

I'm going to close this because I think that we've come to a consensus on the original issue.

[broerse]

Reopening because this will not be solved by #81 it seems.

[rsutphin]

@broerse, I understood #81 to be a side issue: avoid unnecessary reloads in the change watcher by not reloading when ember-data already knows about the changed rev. It's orthogonal to the question here, which is whether the change watcher should automatically tell ember-data to load every not-yet-loaded record that it sees.

I thought that we had agreed (back in March) that having the ember-pouch adapter immediately and automatically load every single record that ever came into the database was ill-advised. I gave you an alternative that provides that behavior if your app is going to have few enough records that it can handle loading them all all the time.

What else would you like to see come out of this issue?

[broerse]

I thanks you for the alternative. I use it now but it seems to me like a hack. I like this to be handled by Ember Pouch by default. Perhaps add a Switch if you want the filtered way or the non filtered way. I can see you point wanting to be able to filter data also. Let me think about it.

[srsgores]

I have to side with @broerse here. It makes more sense to allow disabling live loading new records rather than to disable it by default.

[rsutphin]

I use it now but it seems to me like a hack.

It's essentially the same code that was in the library before #43. It's just in your application instead.

It makes more sense to allow disabling live loading new records rather than to disable it by default.

Say that you are coming to this adapter as a new user. You have read about PouchDB's easy ability to sync from a CouchDB server. You have a modestly-sized dataset you want to replicate into your mobile ember app — say 500 records or so. You set up your server, set up the ember app, and set up replication. Consider these two scenarios:

1. The first time you trigger replication, the app completely stops responding for several minutes. Which one of these is your likely response:
   • Let me review the adapter documentation carefully. Maybe there is a flag I need to set if I'm replicating more than a trivial amount of data.
   • Wow, PouchDB is super slow. I guess I'll need to find another solution.
2. When replication is triggered, the data comes in smoothly and without interrupting the application. You notice that, while existing records get updated, new records don't show up on the page you're on when a replication happens — you have to go to a different page and come back. Which of these is your likely response:
   • Let me review the documentation & discussions about the ember-pouch adapter. Maybe someone has a suggestion of how to make new records load also.
   • When I was reading about PouchDB, I saw that it fires change events when it sees new or updated data. Maybe I can figure out a way to use those for this one page I want to have new records show up in automatically.
   • I guess ember-pouch won't work for me. I'm going to find a different solution.

[broerse]

As a beginner you only read the Ember Pouch doc and start using it. I did.

When I was reading about PouchDB, I saw that it fires change events when it sees new or updated data. Maybe I can figure out a way to use those for this one page I want to have new records show up in automatically.

Okay now you try to find a way to set sync for the whole store or for import { Model } from 'ember-pouch'; and it seems that it is not provided by Ember Pouch ??

I will be offline for 3 days so let me think about it some more.

[broerse]

@fsmanuel Just for my pondering about this. Is there a way to set a sync flag per imported { model }?

[fsmanuel]

@broerse you can use an adapter per type and change the watcher behavior.

[broerse]

@fsmanuel I know but if we can create a liveSync flag per model maybe we can we can test for liveSync on a per model and/or a per store basis. How to do the storesync flag (storeLiveSync) above is obvious to me but not how to create a modelLiveSync flag.

[fsmanuel]

@broerse what do you mean by modelLiveSync? Per modelType or per record?

[broerse]

@fsmanuel I like to be able to set the liveSync flag in the (not ready yet) code example below per Store or Per modelType.

    var liveSync = false;
    var recordInStore = store.peekRecord(obj.type, obj.id);
    if (!recordInStore) {
      if (!liveSync) {
        return;
      }
      return Ember.run.next(function() {
        recordInStore = store.peekRecord(obj.type, obj.id);
        if (!recordInStore) {
          return store.findRecord(obj.type, obj.id);
        }
      });
    }

    if (!recordInStore.get('isLoaded') || recordInStore.get('hasDirtyAttributes')) {
      if (!liveSync) {
        return;
      }
      recordInStore.one('didLoad',function() {
        if (recordInStore.get('rev') !== change.changes[0].rev) {
          return recordInStore.reload();
        }
      });
    }

[broerse]

I have to check but is reload: true not the same as I try to fix with the liveSync flag?

store.findAll('user');  //goes to the server the first time
store.findAll('user');  //after that returns from cache, but updates in background
store.findAll('user', { reload: true });  //enforces getting fresh data
store.findAll('user', { backgroundReload: false });  //opts out of background updating

[rsutphin]

I have to check but is reload: true not the same as I try to fix with the liveSync flag?

I don't think so. I think it's the per-query override of the same behavior that the shouldReloadAll method controls at the adapter level.

[rsutphin]

It would be nice if we could use the fact that an application has queried for all records to control whether we automatically load new records as they come in. We would need to know:

• When findAll has been called for a model, and then later
• When unloadAll has been called for the same model

In between those two calls, I think it would make sense to automatically load any new records for that model that were seen in the change watcher. It would be automatic and seamless and ember-y — no user-set flags required.

I know we can do the first one, but when I looked into this a while ago there was no way for the adapter to know about the second one without using private APIs. Anyone know if that has changed recently? (#39 has an older example of using a private API to detect that condition.)

[broerse]

@rsutphin I looked at #39 and it seems to do the rev check before your own record is loaded by ember-data. Where is the unloadAll condition checked in #39 ? It is not clear to me.

I think if we can fix this issue without a user-set flag it would be perfect. I will also take a look if there is changed something recently. @fsmanuel if it can be done without a flag you can discard my question.

Is the current ember-pouch behavior compatible with store.findAll('user', { backgroundReload: true }); ?
Should we listen to store.findAll('user', { backgroundReload: false }); in the change watcher or is this working already, true or false? Should we create new issue for it if not?

[rsutphin]

The watcher in #39 achieves the effect I'm describing via this line:

return unless typeMap.findAllCache?

It is using the presence of this property which is there between when find-all is done and when unload-all is done. (I don't know if this API still exists in current ember-data. Either way, though, it's private and I don't think we should use private APIs.)

Is the current ember-pouch behavior compatible with store.findAll('user', { backgroundReload: true }); ?

AFAIK, yes. I have not seen anything that says that the adapter needs to do anything special for this, nor would I expect it to. It's something that can be done entirely by the store.

Should we listen to store.findAll('user', { backgroundReload: false }); in the change watcher or is this working already, true or false? Should we create new issue for it if not?

Can you be more specific about your concern? I don't understand what you think we should be doing.

[broerse]

Can you be more specific about your concern? I don't understand what you think we should be doing.

Stop the recordInStore.reload(); in the pouch onChange if store.findAll('user', { backgroundReload: false });. Not sure about this. Just learning.

[rsutphin]

I see what you mean.

I think they are separate concepts — ember-data's background reload means "give me whatever data you have cached immediately, then issue a new request to the adapter and update the array when the response is available". The change watcher does not issue new requests — it responds to data as it arrives.

To put it another way, ember-data's background reload is at a point in time, while the change watcher is continuous.

[mattmarcum]

I think I was running into this same issue, just took me awhile to figure it out. I was seeing updates and deletes to already loaded records propagate between nodes just fine, but new records weren't propagating. Took me a while to debug this to the point where I decided to go look at the issues here on github. Here's the thing - I actually see the new records in indexeddb, just not in ember-data. So the record has already been retrieved from the server and saved to indexeddb but not loaded into ember-data. It looks like the reason given for this behavior is because loading all the records might cause too many resources to be consumed...but I mean, its already happening anyways right? I think the correct place to control propagation of records is at the couchdb level with security/access/validation controls right?

tldr: new records are getting retrieved and saved to indexeddb just not pushed into ember-data. Since we're already doing 90% of the work, why not go ahead and do the other 10% by default.

[mattmarcum]

I think at the very least we need to update the README to alert people to this unexpected behavior. I think added a config option to skip this check would be pretty easy too.
https://github.com/nolanlawson/ember-pouch/blob/master/addon/adapters/pouch.js#L58

I'll see if I can get a pr up later tonight after dinner.

[rsutphin]

It looks like the reason given for this behavior is because loading all the records might cause too many resources to be consumed...but I mean, its already happening anyways right?

The resources in question are not storage but memory. Automatically loading every record into the ember-data store's in-memory cache will kill your application once you get above a certain amount of data. Definitely you should use CouchDB to control what records are available to your clients, but it's possible to have an application in which the volume of synchronized data is larger than what you want to (or can) have loaded into memory at any given moment. Sync puts in the local database so that it's available when your app needs it.

Good idea to add this to the README – I've added a new section. Feedback appreciated.

I think added a config option to skip this check would be pretty easy too.

It would be easy, but I think it would be a bad idea. A config option to enable this behavior is a footgun. It would work well during development/initial testing, but as your app got bigger or when you tested it with larger data sets you'd have these puzzling slowdowns during sync. At that point it's easy to blame them on "well, sync is slow, I guess". But you (and your users) would be missing out on an better performance that is possible without much effort, so long as you know about it at the beginning.

[mattmarcum]

I'm not sure if we understand each other when we talk about resource usage. My point is that if one node creates a 1000 records then those 1000 records will get synched via pouchdb and stored in indexeddb - you can inspect indexeddb in chrome dev tools and see the records that are retrieved but not loaded into ember-data. I think this screenshot should illustrate it - the browser on the left created the test7 record. The browser on the right retrieved the test7 record, stored it in indexeddb, but did not load it into ember-data.

So with a badly set up db the browser is still going to download all those records, they just won't get loaded into ember-data. Yes, ember-data does incur additional overhead on top of pouchdb/indexeddb...but hiding new records from ember-data does not solve the problem you're talking about. And honestly, I don't think that's the expected behavior in pouch/couchdb implementation. I'd like ember-pouch just to work the same way that pouchdb is working. Pouchdb is already grabbing these new records and making them available, it's just that ember-pouch is ignoring them.

[rsutphin]

What I am saying is that it's entirely possible, likely even, that some apps will legitimately need to sync 1000 records (or 10000) — it's not a mistake or an error. The app needs them to be available offline, but does not (could not) use all of them at once. Forcing them to all load at once during sync will kill the app.

And I'm further suggesting that lots of apps will be like this over time. Initially you just have a few dozen articles in your knowledge base, but eventually it grows to hundreds. Or you only have a few issues in your backlog, but they get created faster than they get closed. Or you have a contact database with just a few hundred people, but they all have multiple addresses, personal statements, contact preferences, etc. — thousands of records that you don't need open all the time, but want to have offline in case want to look someone up.

Having the default behavior be something that scales much, much worse than what's easily possible if you know the alternative seems bad to me.

Pouchdb is already grabbing these new records and making them available, it's just that ember-pouch is ignoring them.

As I've noted elsewhere in this thread, it's really easy to write your own change listener to load them if you know that that it isn't going to kill your application. Having this behavior be a change listener away gives you the opportunity to consider if automatically loading everything is what you really want, or if it makes more sense for your app to just load everything of one type or whatever.

[mattmarcum]

When you say 'Forcing them to all laod at once during sync...' I'm not sure if I follow your train of thought. From my experiments pouchdb will sync all these records locally - it will download them over the wire and store them in indexeddb and will trigger a change event. At this point you've already incurred a big bandwidth/memory hit, and this is before ember-pouch processes the change event. Your solution implemented in ember-pouch - to ignore records if they aren't already loaded in ember-data's store - doesn't address the real problem you're concerned with. If a developer has made a crappy app that tries to sync 100mb of data on initial load - then that's going to happen in pouchdb regardless of what ember-pouch's adapter does later.

[rsutphin]

When I'm talking about "loading them all at once during sync", I'm talking about the idea of having the sync process, in addition to storing the records in PouchDB, tell ember-data to load every single one into the in-memory cache as well. (This is what ember-pouch did before #43 and what would be necessary to have synced records show up with no intervention in your Ember app's UI.)

Records that are synced and stored have a limited memory impact — they flow through the browser's memory on their way to storage, but then are available to be garbage-collected. Records that are kept in ember-data's cache are not available to be garbage-collected. The memory they take is reserved until you navigate away from the application, or you explicitly unload them.

Taking more memory than you need is bad on general principles. On a desktop browser, it will probably just result in bloat (though someone with a less-than-recent machine or a low-spec Chromebook may disagree). On a mobile device it can result in the browser killing your tab.

Keeping records in ember-data's store cache also hurts performance in another way: As new records are loaded, ember-data does in-memory resolution of relationships. E.g., say you have Person and Sandwich models, and Sandwich belongs to Person. When you load a new person, ember-data will look through all the already-loaded sandwiches to see if the new person belongs to one of them. Not a big deal if you only have a few of each, but if you have thousands of records across a dozen interrelated models, this resolution time adds up.

It takes much, much less than 100MB of data to trigger bad performance this way. As I tried to make clear in my last reply, it happens with totally medium-sized data sets on totally banal application types. It really did happen with the app I use ember-pouch in, which is why I submitted #43 in the first place. Among other things, that app is a sort of contact manager. We ran into this problem on initial sync with just a few hundred contacts, plus their associated detail records. We almost stopped using PouchDB, until I figured out that the problem was actually in ember-pouch.

[mattmarcum]

That's pretty incredible. Both my current and previous employer have ember apps in production that can load 1000+ records into ember-data and not see that kind of performance hit....but we aren't using pouchdb/ember-pouch.

I'd still rather have the ability to turn this check off. There's really no simple solution for implementing a new change listener that isn't just a copy & paste of the change listener already implemented in adapters/pouch.js - minus lines 57-59. Of course once you do that you're thrown into the situation of trying to keep your local onChange function in sync with whatever changes are occurring in ember-pouch. That's a bad situation to foist on end developers.

[rsutphin]

Do they load those 1000+ records over the course of a few seconds? It's more of an instantaneous performance problem — if the sync is able to complete, the app is usually okay (though on mobile it could still have problems due to the total memory consumption).

here's really no simple solution for implementing a new change listener that isn't just a copy & paste of the change listener already implemented in adapters/pouch.js

Your new change listener doesn't need to do everything that the existing one does because the existing one will still be there. All it needs to do is tell ember-data to load new records. To do that, you only have to parse the ID (db.rel.parseDocID) and call store.findRecord with the resulting model and ID.

[dehuszar]

I'm trying to get an effect similar to what's discussed here --namely, when a user loads the site or a route for the first time (and forever more), all the records load without user intervention (the site doesn't contain more than a few dozen records and won't grow much over time).

Following the conversation above, I've tried a few suggestions and examples, but am unable to get the desired result. I'm sure I'm missing a super tiny detail.

Here's my Pouch Adapter:

https://gist.github.com/dehuszar/826ba357121e618da449

[broerse]

Perhaps you should use getOwner and service:store like this:

https://github.com/broerse/ember-cli-blog/blob/master/app/adapters/application.js

[dehuszar]

That worked, thanks!

[broerse]

from v3.2.0 and up you can solve this with:

export default Adapter.extend({
  db: db,

  unloadedDocumentChanged: function(obj) {
    let store = this.get('store');
    let recordTypeName = this.getRecordTypeName(store.modelFor(obj.type));
    this.get('db').rel.find(recordTypeName, obj.id).then(function(doc) {
      store.pushPayload(recordTypeName, doc);
    });
  }
});

Thanks @rsutphin @lukemelia @fsmanuel @nolanlawson

Finally closing the issue!