From ddf0db03383a0c3e719accfe02570f5293fc2647 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Mon, 31 Aug 2015 14:09:57 -0400 Subject: [PATCH] Refactor notifications `localStorage` cache into adapter pattern. Sometimes you want stale data right away, then refresh it async. This adds `findStale` to the store for that case. If it returns an object with `hasResults` you can get the `results` and display them. It also returns a `refresh()` method to freshen up the stale data. To enable `localStorage` support for stale data, just include the mixin `StaleLocalStorage` into an adapter for that model. This commit includes a sample of doing that for `Notifications`. --- .../discourse/adapters/notification.js.es6 | 4 ++ .../discourse/adapters/rest.js.es6 | 5 +++ .../discourse/components/user-menu.js.es6 | 45 +++++-------------- .../discourse/lib/avatar-template.js.es6 | 9 ++-- .../javascripts/discourse/lib/hash.js.es6 | 11 +++++ .../discourse/lib/stale-result.js.es6 | 12 +++++ .../mixins/stale-local-storage.js.es6 | 32 +++++++++++++ .../javascripts/discourse/models/store.js.es6 | 28 +++++++++--- app/assets/javascripts/main_include.js | 2 + test/javascripts/models/store-test.js.es6 | 12 +++++ 10 files changed, 113 insertions(+), 47 deletions(-) create mode 100644 app/assets/javascripts/discourse/adapters/notification.js.es6 create mode 100644 app/assets/javascripts/discourse/lib/hash.js.es6 create mode 100644 app/assets/javascripts/discourse/lib/stale-result.js.es6 create mode 100644 app/assets/javascripts/discourse/mixins/stale-local-storage.js.es6 diff --git a/app/assets/javascripts/discourse/adapters/notification.js.es6 b/app/assets/javascripts/discourse/adapters/notification.js.es6 new file mode 100644 index 0000000000..f8298a4307 --- /dev/null +++ b/app/assets/javascripts/discourse/adapters/notification.js.es6 @@ -0,0 +1,4 @@ +import RestAdapter from 'discourse/adapters/rest'; +import StaleLocalStorage from 'discourse/mixins/stale-local-storage'; + +export default RestAdapter.extend(StaleLocalStorage); diff --git a/app/assets/javascripts/discourse/adapters/rest.js.es6 b/app/assets/javascripts/discourse/adapters/rest.js.es6 index 1a9fa03274..347f27b8a1 100644 --- a/app/assets/javascripts/discourse/adapters/rest.js.es6 +++ b/app/assets/javascripts/discourse/adapters/rest.js.es6 @@ -1,3 +1,4 @@ +import StaleResult from 'discourse/lib/stale-result'; const ADMIN_MODELS = ['plugin', 'site-customization', 'embeddable-host']; export function Result(payload, responseJson) { @@ -53,6 +54,10 @@ export default Ember.Object.extend({ return ajax(this.pathFor(store, type, findArgs)).catch(rethrow); }, + findStale() { + return new StaleResult(); + }, + update(store, type, id, attrs) { const data = {}; const typeField = Ember.String.underscore(type); diff --git a/app/assets/javascripts/discourse/components/user-menu.js.es6 b/app/assets/javascripts/discourse/components/user-menu.js.es6 index 23200374c7..b99626795a 100644 --- a/app/assets/javascripts/discourse/components/user-menu.js.es6 +++ b/app/assets/javascripts/discourse/components/user-menu.js.es6 @@ -31,50 +31,27 @@ export default Ember.Component.extend({ } }, - loadCachedNotifications() { - var notifications; - try { - notifications = JSON.parse(localStorage["notifications"]); - notifications = notifications.map(n => Em.Object.create(n)); - } catch (e) { - notifications = null; - } - return notifications; - }, - - // TODO push this kind of functionality into Rest thingy - cacheNotifications(notifications) { - const keys = ["id", "notification_type", "read", "created_at", "post_number", "topic_id", "slug", "data"]; - const serialized = JSON.stringify(notifications.map(n => n.getProperties(keys))); - const changed = serialized !== localStorage["notifications"]; - localStorage["notifications"] = serialized; - return changed; - }, - refreshNotifications() { - if (this.get('loadingNotifications')) { return; } - var cached = this.loadCachedNotifications(); - - if (cached) { - this.set("notifications", cached); - } else { - this.set("loadingNotifications", true); - } - // TODO: It's a bit odd to use the store in a component, but this one really // wants to reach out and grab notifications const store = this.container.lookup('store:main'); - store.find('notification', {recent: true}).then((notifications) => { + const stale = store.findStale('notification', {recent: true}); + + if (stale.hasResults) { + this.set('notifications', stale.results); + } else { + this.set('loadingNotifications', true); + } + + stale.refresh().then((notifications) => { this.set('currentUser.unread_notifications', 0); - if (this.cacheNotifications(notifications)) { - this.setProperties({ notifications }); - } + this.set('notifications', notifications); }).catch(() => { this.set('notifications', null); }).finally(() => { - this.set("loadingNotifications", false); + this.set('loadingNotifications', false); }); }, diff --git a/app/assets/javascripts/discourse/lib/avatar-template.js.es6 b/app/assets/javascripts/discourse/lib/avatar-template.js.es6 index 16b70cadc7..542e979596 100644 --- a/app/assets/javascripts/discourse/lib/avatar-template.js.es6 +++ b/app/assets/javascripts/discourse/lib/avatar-template.js.es6 @@ -1,4 +1,5 @@ -/*eslint no-bitwise:0 */ +import { hashString } from 'discourse/lib/hash'; + let _splitAvatars; function defaultAvatar(username) { @@ -7,11 +8,7 @@ function defaultAvatar(username) { _splitAvatars = _splitAvatars || defaultAvatars.split("\n"); if (_splitAvatars.length) { - let hash = 0; - for (let i = 0; i { + localStorage.setItem(this.storageKey(type, findArgs), JSON.stringify(results)); + return results; + }); + } +} diff --git a/app/assets/javascripts/discourse/models/store.js.es6 b/app/assets/javascripts/discourse/models/store.js.es6 index dc6acae822..851b712a61 100644 --- a/app/assets/javascripts/discourse/models/store.js.es6 +++ b/app/assets/javascripts/discourse/models/store.js.es6 @@ -61,14 +61,28 @@ export default Ember.Object.extend({ }); }, + _hydrateFindResults(result, type, findArgs) { + if (typeof findArgs === "object") { + return this._resultSet(type, result); + } else { + return this._hydrate(type, result[Ember.String.underscore(type)], result); + } + }, + + // See if the store can find stale data. We sometimes prefer to show stale data and + // refresh it in the background. + findStale(type, findArgs) { + const stale = this.adapterFor(type).findStale(this, type, findArgs); + if (stale.hasResults) { + stale.results = this._hydrateFindResults(stale.results, type, findArgs); + } + stale.refresh = () => this.find(type, findArgs); + return stale; + }, + find(type, findArgs) { - const self = this; - return this.adapterFor(type).find(this, type, findArgs).then(function(result) { - if (typeof findArgs === "object") { - return self._resultSet(type, result); - } else { - return self._hydrate(type, result[Ember.String.underscore(type)], result); - } + return this.adapterFor(type).find(this, type, findArgs).then((result) => { + return this._hydrateFindResults(result, type, findArgs); }); }, diff --git a/app/assets/javascripts/main_include.js b/app/assets/javascripts/main_include.js index e48e17368f..69396e6c98 100644 --- a/app/assets/javascripts/main_include.js +++ b/app/assets/javascripts/main_include.js @@ -9,6 +9,8 @@ //= require ./ember-addons/decorator-alias //= require ./ember-addons/macro-alias //= require ./ember-addons/ember-computed-decorators +//= require ./discourse/lib/hash +//= require ./discourse/lib/stale-result //= require ./discourse/lib/load-script //= require ./discourse/lib/notification-levels //= require ./discourse/lib/app-events diff --git a/test/javascripts/models/store-test.js.es6 b/test/javascripts/models/store-test.js.es6 index 266eb06fa2..6f39c2186a 100644 --- a/test/javascripts/models/store-test.js.es6 +++ b/test/javascripts/models/store-test.js.es6 @@ -63,6 +63,17 @@ test('find with query param', function() { }); }); +test('findStale with no stale results', (assert) => { + const store = createStore(); + const stale = store.findStale('widget', {name: 'Trout Lure'}); + + assert.ok(!stale.hasResults, 'there are no stale results'); + assert.ok(!stale.results, 'results are present'); + return stale.refresh().then(function(w) { + assert.equal(w.get('firstObject.id'), 123, 'a `refresh()` method provides results for stale'); + }); +}); + test('update', function() { const store = createStore(); return store.update('widget', 123, {name: 'hello'}).then(function(result) { @@ -134,3 +145,4 @@ test('findAll embedded', function(assert) { assert.equal(fruits.objectAt(2).get('farmer.name'), 'Luke Skywalker'); }); }); +