Trigger database syncing with Background Sync #820

gnowoel · 2016-11-21T17:10:06Z

Fixes #731.

Do initial sync in the main thread. Subsequent syncing is triggered with a Background Sync event and only happens in the Service Worker.

cc @HospitalRun/core-maintainers

jkleinsc · 2016-11-22T17:15:46Z

@gnowoel thanks for your work on this PR. I think we need to hold off on further work on this until this PR for PouchDB is completed: pouchdb/pouchdb#5903. I believe this maybe why you are having trouble doing the initial sync.

Basically, PouchDB wasn't testing fetch properly in their test suite, so it appears there may be some issues with using fetch with PouchDB, which is needed to use PouchDB within a Service Worker.

Also, in looking at your code, listening for changes in the Service Worker will not work because the service worker can be killed at any time by the browser, so we can't rely on the changes listener to be long running.

I haven't had time to test the concept in full but I think the solution for HospitalRun should be as follows in browsers that support background sync and service workers

The UI thread just has a remote DB
The Service Worker intercepts all network requests to remote DB (the code in HR already does this).
On network requests that are updating data, register a background sync event if the network request fails.
Server side Push API is used to push changes from server side to local db.

gnowoel · 2016-11-23T05:40:35Z

@jkleinsc My motivation of the PR was to fix the upfront bug, while laying ground work for possible future update. I totally respect your decision to hold it off, but I really think we could perhaps improve this PR first. Here're the reasons just for your consideration.

... listening for changes in the Service Worker will not work...

Actually, we listen for database changes in the main thread (in the application adapter). The Server Worker on the other hand only responds to the sync event fired on database change.

The UI thread just has a remote DB

We created a PouchDB instance in the UI thread. This is for doing the initial sync and watching database changes. Considering the initial sync in fact only happens once (because the data would be persisted in IndexedDB) and watching database changes is a relatively lightweight operation, the performance impact should be acceptable.

On network requests that are updating data, register a background sync event if the network request fails.

I didn't do intensive testing, but this seems to be working already. If there're any bugs, I'd be happy to fix them.

Server side Push API is used to push changes from server side to local db.

It's a great idea for sure. However, as I briefly mentioned above, while it's not ideal, watching database changes in the UI thread should be acceptable. So, it's more like an enhancement than a necessity for me. Could we leave the watching code in the UI thread for now, and worry about how to integrate Push API (which requires more work because it needs service-side cooperation) for later?

I think we need to hold off on further work on this until this PR for PouchDB is completed...

If the bug is fixed, maybe we can move the initial syncing code into the Service Worker. But it also sounds more like an enhancement to me. The initial sync only happens initially, when we start the app in the browser for the very first time. Once the databases has been fully synced for once, all subsequent calling to the one-shot sync function would do about nothing.

Suggestion

While I totally respect your decision to hold it off, I would suggest that we fix the bug first. If we stop here, we get something that works already. If we want to improve it later by introducing advanced techniques like Pushing API, the current implementation shouldn't get in the way.

Please correct me if I'm completely wrong.

jkleinsc · 2016-11-23T16:34:20Z

@gnowoel I misread your code. I mistakenly thought the changes listeners were in the service worker. Let me take a closer look at your code and what you are proposing.

Can you check on one thing on your end? It looks like Travis CI is failing and I suspect if you run ember test locally you will see the errors as well. The full output from Travis is available here:
https://travis-ci.org/HospitalRun/hospitalrun-frontend/builds/177716747

The error I am seeing is:
'Error: Failed to create an instance of 'adapter:application'. Most likely an improperly defined class or an invalid module export. at http://localhost:7357/assets/vendor.js, line 12015\n' }

gnowoel · 2016-11-24T13:48:11Z

@jkleinsc I missed the test part. Sorry about that.

The failure in the CI is due to undefined databases. We created databases when initializing the database service and I guess this means it's triggered by the first page request. However, there won't be a page request when running the app in a CI.

I updated the code to just skip the initial sync unless database are created.

By the way, do you think it's a good idea to implement poor man's Periodic Sync with setInterval(), by repeatedly firing lightweight Background Sync events from the UI thread?

jkleinsc

@gnowoel thanks for the updates. I finally had some time to sit down and take a closer look and I have a decent amount of comments. Basically the concepts are sound, but there are some improvements that can be made using Ember conventions around service injections as well as tapping into functions that ember-pouch provides us. Let me know if this all makes sense:

jkleinsc · 2016-11-29T21:51:56Z

app/adapters/application.js

+  });
+}
+
+if (localMainDB && remoteDB) {


Instead of running this code when this code is parsed, it should be run when the DB is set on the adapter. This adapter extends the ember-pouch adapter, source code for that is here:
https://github.com/nolanlawson/ember-pouch/blob/master/addon/adapters/pouch.js.
There is a function in the adapter, changeDb that we should be calling to set the database from the database service, but right now we are instead using a computed property to define the db with the following line:
db: Ember.computed.reads('database.mainDB'),

What we should do instead is in app/services/database do the following:

Inject the store service into the database service: store: Ember.inject.service('store')

In the setup function after this line: this.set('mainDB', db);, add the following code:

let adapter = this.get('store').adapterFor('application'); adapter.changeDb(db);

Once this is setup, we can then extend onChange function in app/adapters/application.js to run our code. For example:

onChange(db) { foregroundSync().then(() => { watchLocalDB(); this._super(db); //Super will watch remote db }); }

jkleinsc · 2016-11-29T21:56:42Z

app/adapters/application.js

+
+function backgroundSync() {
+  console.log('registering background sync');
+  return navigator.serviceWorker.ready


This code should verify navigator.serviceWorker is available, eg if (navigator.serviceWorker) { ...

jkleinsc · 2016-11-29T21:57:08Z

app/adapters/application.js

+  console.log('registering background sync');
+  return navigator.serviceWorker.ready
+    .then(function(reg) {
+      return reg.sync.register('sync');


Same thing here, check to see if reg.sync is available to call.

jkleinsc · 2016-11-29T21:59:09Z

app/adapters/application.js

@@ -9,6 +9,62 @@ const {
  }
 } = Ember;

+const { localMainDB, remoteDB } = window;
+
+function foregroundSync() {


My only concern here is this will be slow for large databases and I would prefer to offload that to a separate thread. Small dbs would work ok with this though.

jkleinsc · 2016-11-29T21:59:34Z

app/adapters/application.js

+  });
+}
+
+function watchRemoteDB() {


This adapter is already watching the remoteDB, so we just need to extend the onChange function. For example:

onChange(change) { backgroundSync(); this._super(change); }

jkleinsc · 2016-11-29T22:02:16Z

app/services/database.js

@@ -162,6 +165,7 @@ export default Ember.Service.extend(PouchAdapterUtils, {
  _createRemoteDB(remoteUrl, pouchOptions) {
    return new Ember.RSVP.Promise(function(resolve, reject) {
      let remoteDB = new PouchDB(remoteUrl, pouchOptions);
+      window.remoteDB = remoteDB;


Using a global is unneeded. The application adapter has access to the database service, so it can use that to get the remoteDb via:

this.get('database').get('mainDB')

jkleinsc · 2016-11-29T22:02:55Z

app/adapters/application.js

@@ -9,6 +9,62 @@ const {
  }
 } = Ember;

+const { localMainDB, remoteDB } = window;


These values should be pulled from the database service. For example:

let remoteDB = this.get('database').get('mainDB');

jkleinsc · 2016-11-29T22:05:40Z

app/services/database.js

@@ -176,6 +180,7 @@ export default Ember.Service.extend(PouchAdapterUtils, {
  _createLocalDB(localDBName, pouchOptions) {
    return new Ember.RSVP.Promise(function(resolve, reject) {
      let localDB = new PouchDB(localDBName, pouchOptions);
+      window.localMainDB = localDB;


Like remoteDB, Using a global is unneeded, but we should set the localDB as a property on the database service so that the application adapter has access to it:

this.set('localDb', localDb);

Then in the application adapter you can access via:

let localDB = this.get('database').get('localDB');

jkleinsc · 2016-11-29T22:08:23Z

@gnowoel as far as setInterval, I think that is unnecessary for now.

tangollama · 2017-04-23T18:08:46Z

@gnowoel, do you think you'll be willing / able to make the recommended changes from @jkleinsc?

gnowoel · 2017-04-24T03:39:08Z

@tangollama Sorry for the long delay. I didn't forget it, but I was trapped by my own life's struggle. Your reminder reminds me that there's no better time than now. And yes, I'm willing to finish what I started, at least give it another try before it's too late.

jkleinsc · 2017-04-24T14:26:09Z

@gnowoel My apologies here. Sorry to hear about your own life's struggle - I hope everything is going ok. @tangollama and I weren't in sync on this. I have been doing some work on this whole issue and given that we are trying to get a 1.0 release out this month, I need to take primary responsibility for this issue, so please feel released from taking care of this. I am going to close this PR, but please feel free to contribute in the future!

gnowoel · 2017-04-24T16:39:13Z

@jkleinsc Thanks for your kind words. It means a lot to me!

I was trying to update PR today, based on your very inspiring comment above. It's only partially completed, but along the way, I've found a few things that might be useful.

The local and remote PouchDB instances are only available after logging in, which makes it difficult to setup database syncing in the application adapter.
The application adapter seems to be initialized for every request, but we only want to setup syncing once.
The db of the application adapter would change based on the connection status, so it might be a challenge to correctly use onChange() to trigger a background sync.

I ended up defining the sync-related methods in a service, and then injecting it in the module that handles authentication. That way, I can setup syncing just by adding a then() block:

authenticate(credentials).then(() => this.get('sync').setup());

I'm not sure if it's a good idea. Perhaps Ember initializers are the way to go. I don't know. But I'm sure your solution will be much better.

Thanks again!

(I'd like to attach my latest update below. It's just a prototype. So, please ignore it if it's useless.)

gnowoel · 2017-04-24T16:43:59Z

Oops. The code is not showing. Guess it's because the issue is closed. Never mind :) In case you want to take a look, please see here.

Trigger database syncing with Background Sync

875f497

gnowoel mentioned this pull request Nov 21, 2016

Syncing for offline use not working #731

Closed

Don't do initial sync unless databases are created

2b3b1b3

Wait the promise to resolve in the sync handler

f71fc7c

jkleinsc suggested changes Nov 29, 2016

View reviewed changes

jkleinsc closed this Apr 24, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Trigger database syncing with Background Sync #820

Trigger database syncing with Background Sync #820

gnowoel commented Nov 21, 2016 •

edited

Loading

jkleinsc commented Nov 22, 2016

gnowoel commented Nov 23, 2016 •

edited

Loading

jkleinsc commented Nov 23, 2016

gnowoel commented Nov 24, 2016 •

edited

Loading

jkleinsc left a comment

jkleinsc Nov 29, 2016

jkleinsc Nov 29, 2016

jkleinsc Nov 29, 2016

jkleinsc Nov 29, 2016

jkleinsc Nov 29, 2016

jkleinsc Nov 29, 2016

jkleinsc Nov 29, 2016

jkleinsc Nov 29, 2016

jkleinsc commented Nov 29, 2016

tangollama commented Apr 23, 2017

gnowoel commented Apr 24, 2017

jkleinsc commented Apr 24, 2017

gnowoel commented Apr 24, 2017

gnowoel commented Apr 24, 2017

Trigger database syncing with Background Sync #820

Trigger database syncing with Background Sync #820

Conversation

gnowoel commented Nov 21, 2016 • edited Loading

jkleinsc commented Nov 22, 2016

gnowoel commented Nov 23, 2016 • edited Loading

jkleinsc commented Nov 23, 2016

gnowoel commented Nov 24, 2016 • edited Loading

jkleinsc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jkleinsc commented Nov 29, 2016

tangollama commented Apr 23, 2017

gnowoel commented Apr 24, 2017

jkleinsc commented Apr 24, 2017

gnowoel commented Apr 24, 2017

gnowoel commented Apr 24, 2017

gnowoel commented Nov 21, 2016 •

edited

Loading

gnowoel commented Nov 23, 2016 •

edited

Loading

gnowoel commented Nov 24, 2016 •

edited

Loading