-
Notifications
You must be signed in to change notification settings - Fork 604
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
[2308] Refactor Adaptive store to use DI #2312
Changes from 5 commits
13f3b39
a1bfef6
f46a18d
9c5f974
50b1f2b
f09facb
d976a4d
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -2,11 +2,15 @@ import { computed } from '@ember/object'; | |||||
import { inject as service } from '@ember/service'; | ||||||
import { getOwner } from '@ember/application'; | ||||||
import Base from 'ember-simple-auth/session-stores/base'; | ||||||
import LocalStorage from 'ember-simple-auth/session-stores/local-storage'; | ||||||
import Cookie from 'ember-simple-auth/session-stores/cookie'; | ||||||
|
||||||
const LOCAL_STORAGE_TEST_KEY = '_ember_simple_auth_test_key'; | ||||||
|
||||||
const injectStore = (store) => { | ||||||
return computed(function() { | ||||||
return getOwner(this).lookup(`session-store:${store}`); | ||||||
}); | ||||||
}; | ||||||
|
||||||
const proxyToInternalStore = function() { | ||||||
return computed({ | ||||||
get(key) { | ||||||
|
@@ -53,6 +57,8 @@ export default Base.extend({ | |||||
*/ | ||||||
localStorageKey: 'ember_simple_auth-session', | ||||||
|
||||||
localStorage: injectStore('local-storage'), | ||||||
|
||||||
/** | ||||||
The domain to use for the cookie if `localStorage` is not available, e.g., | ||||||
"example.com", ".example.com" (which includes all subdomains) or | ||||||
|
@@ -102,7 +108,21 @@ export default Base.extend({ | |||||
_cookieExpirationTime: null, | ||||||
cookieExpirationTime: proxyToInternalStore(), | ||||||
|
||||||
_sameSite: null, | ||||||
sameSite: proxyToInternalStore(), | ||||||
|
||||||
_cookies: service('cookies'), | ||||||
cookie: injectStore('cookie'), | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. see above |
||||||
|
||||||
_isLocalStorageAvailable: computed({ | ||||||
marcoow marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
get() { | ||||||
return testLocalStorageAvailable(); | ||||||
}, | ||||||
|
||||||
set(key, value) { | ||||||
return value; | ||||||
} | ||||||
}), | ||||||
|
||||||
init() { | ||||||
this._super(...arguments); | ||||||
|
@@ -112,28 +132,25 @@ export default Base.extend({ | |||||
this._fastboot = owner.lookup('service:fastboot'); | ||||||
} | ||||||
|
||||||
this._isLocalStorageAvailable = this.hasOwnProperty('_isLocalStorageAvailable') ? this._isLocalStorageAvailable : testLocalStorageAvailable(); | ||||||
|
||||||
let store; | ||||||
if (this.get('_isLocalStorageAvailable')) { | ||||||
store = this.get('localStorage'); | ||||||
const options = { key: this.get('localStorageKey') }; | ||||||
options._isFastBoot = false; | ||||||
store = this._createStore(LocalStorage, options); | ||||||
store.setProperties(options); | ||||||
} else { | ||||||
const options = this.getProperties('cookieDomain', 'cookieName', 'cookieExpirationTime', 'cookiePath'); | ||||||
options._fastboot = this.get('_fastboot'); | ||||||
options._cookies = this.get('_cookies'); | ||||||
store = this.get('cookie'); | ||||||
const options = this.getProperties('sameSite', 'cookieDomain', 'cookieName', 'cookieExpirationTime', 'cookiePath'); | ||||||
|
||||||
store = this._createStore(Cookie, options); | ||||||
store._initialize(options); | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does
Suggested change
not work here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I could check that out, although The proxy properties that were receiving some default values on initialization weren't causing any issues before, but right now since we can't really pass any properties during init, but we set them after instead - its causing e.g |
||||||
this.set('cookieExpirationTime', store.get('cookieExpirationTime')); | ||||||
} | ||||||
|
||||||
this.set('_store', store); | ||||||
this._setupStoreEvents(store); | ||||||
}, | ||||||
|
||||||
_createStore(storeType, options) { | ||||||
let owner = getOwner(this); | ||||||
const store = storeType.create(owner.ownerInjection(), options); | ||||||
|
||||||
_setupStoreEvents(store) { | ||||||
store.on('sessionDataUpdated', (data) => { | ||||||
this.trigger('sessionDataUpdated', data); | ||||||
}); | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,25 +1,13 @@ | ||
import Adaptive from 'ember-simple-auth/session-stores/adaptive'; | ||
import createCookieStore from './create-cookie-store'; | ||
import { merge, assign as emberAssign } from '@ember/polyfills'; | ||
|
||
const assign = emberAssign || merge; | ||
|
||
export default function createAdaptiveStore( | ||
cookiesService, | ||
options = {}, | ||
props = {} | ||
owner | ||
) { | ||
let cookieStore = createCookieStore( | ||
cookiesService, | ||
assign({}, options, { _isFastboot: false }) | ||
); | ||
props._createStore = function() { | ||
cookieStore.on('sessionDataUpdated', data => { | ||
this.trigger('sessionDataUpdated', data); | ||
}); | ||
|
||
return cookieStore; | ||
}; | ||
owner.register('session-store:adaptive', Adaptive.extend(Object.assign({ | ||
_isLocalStorageAvailable: false, | ||
}, options))); | ||
|
||
return Adaptive.extend(props).create(options); | ||
return owner.lookup('session-store:adaptive'); | ||
} | ||
marcoow marked this conversation as resolved.
Show resolved
Hide resolved
|
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,98 +1,104 @@ | ||
import { run } from '@ember/runloop'; | ||
import { | ||
describe, | ||
beforeEach, | ||
afterEach, | ||
it | ||
} from 'mocha'; | ||
import { describe, it } from 'mocha'; | ||
import { expect } from 'chai'; | ||
import sinonjs from 'sinon'; | ||
import Adaptive from 'ember-simple-auth/session-stores/adaptive'; | ||
import LocalStorage from 'ember-simple-auth/session-stores/local-storage'; | ||
import itBehavesLikeAStore from './shared/store-behavior'; | ||
import itBehavesLikeACookieStore from './shared/cookie-store-behavior'; | ||
import FakeCookieService from '../../helpers/fake-cookie-service'; | ||
import createAdaptiveStore from '../../helpers/create-adaptive-store'; | ||
import { setupTest } from 'ember-mocha'; | ||
|
||
describe('AdaptiveStore', () => { | ||
let sinon; | ||
let store; | ||
|
||
beforeEach(function() { | ||
sinon = sinonjs.createSandbox(); | ||
}); | ||
|
||
afterEach(function() { | ||
store.clear(); | ||
sinon.restore(); | ||
}); | ||
|
||
setupTest(); | ||
describe('when localStorage is available', function() { | ||
beforeEach(function() { | ||
store = Adaptive.extend({ | ||
_createStore(storeType, options) { | ||
return LocalStorage.create({ _isFastBoot: false }, options); | ||
} | ||
}).create({ | ||
_isLocalStorageAvailable: true | ||
}); | ||
}); | ||
|
||
itBehavesLikeAStore({ | ||
store() { | ||
store(sinon, owner) { | ||
let store; | ||
let cookieService; | ||
owner.register('service:cookies', FakeCookieService); | ||
cookieService = owner.lookup('service:cookies'); | ||
sinon.spy(cookieService, 'read'); | ||
sinon.spy(cookieService, 'write'); | ||
store = createAdaptiveStore(cookieService, { | ||
_isLocal: false, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't seem to exist anywhere actually? 🤔 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, that's true, I will remove it, also noticed that some of the |
||
_isLocalStorageAvailable: true, | ||
}, owner); | ||
return store; | ||
} | ||
}); | ||
}); | ||
|
||
describe('when localStorage is not available', function() { | ||
let cookieService; | ||
beforeEach(function() { | ||
cookieService = FakeCookieService.create(); | ||
sinon.spy(cookieService, 'read'); | ||
sinon.spy(cookieService, 'write'); | ||
store = createAdaptiveStore(cookieService, { | ||
_isLocal: false, | ||
_cookies: cookieService, | ||
}); | ||
}); | ||
|
||
itBehavesLikeAStore({ | ||
store() { | ||
store(sinon, owner) { | ||
let store; | ||
let cookieService; | ||
owner.register('service:cookies', FakeCookieService); | ||
cookieService = owner.lookup('service:cookies'); | ||
sinon.spy(cookieService, 'read'); | ||
sinon.spy(cookieService, 'write'); | ||
store = createAdaptiveStore(cookieService, { | ||
_isLocal: false, | ||
_isLocalStorageAvailable: false, | ||
_cookieName: 'test:session', | ||
}, owner); | ||
return store; | ||
} | ||
}); | ||
}); | ||
|
||
itBehavesLikeACookieStore({ | ||
createStore(cookieService, options = {}) { | ||
options._isLocalStorageAvailable = false; | ||
return createAdaptiveStore(cookieService, options, { | ||
_cookies: cookieService, | ||
_fastboot: { isFastBoot: false }, | ||
}); | ||
}, | ||
renew(store, data) { | ||
return store.get('_store')._renew(data); | ||
}, | ||
sync(store) { | ||
store.get('_store')._syncData(); | ||
}, | ||
spyRewriteCookieMethod(store) { | ||
sinon.spy(store.get('_store'), 'rewriteCookie'); | ||
return store.get('_store').rewriteCookie; | ||
} | ||
describe('CookieStore', function() { | ||
describe('Behaviour', function() { | ||
itBehavesLikeACookieStore({ | ||
store(sinon, owner, storeOptions) { | ||
owner.register('service:cookies', FakeCookieService); | ||
let cookieService = owner.lookup('service:cookies'); | ||
sinon.spy(cookieService, 'read'); | ||
sinon.spy(cookieService, 'write'); | ||
let store = createAdaptiveStore(cookieService, Object.assign({ | ||
_isLocal: false, | ||
_isLocalStorageAvailable: false, | ||
_cookieName: 'test:session', | ||
}, storeOptions), owner); | ||
return store; | ||
}, | ||
renew(store, data) { | ||
return store.get('_store')._renew(data); | ||
}, | ||
sync(store) { | ||
store.get('_store')._syncData(); | ||
}, | ||
spyRewriteCookieMethod(sinon, store) { | ||
sinon.spy(store.get('_store'), 'rewriteCookie'); | ||
return store.get('_store').rewriteCookie; | ||
} | ||
}); | ||
}); | ||
|
||
it('persists to cookie when cookie attributes change', function() { | ||
let now = new Date(); | ||
it('persists to cookie when cookie attributes change', async function() { | ||
let sinon = sinonjs.createSandbox(); | ||
let store; | ||
let cookieService; | ||
let now; | ||
|
||
this.owner.register('service:cookies', FakeCookieService); | ||
cookieService = this.owner.lookup('service:cookies'); | ||
sinon.spy(cookieService, 'read'); | ||
sinon.spy(cookieService, 'write'); | ||
run(() => { | ||
store.persist({ key: 'value' }); | ||
now = new Date(); | ||
store = createAdaptiveStore(cookieService, { | ||
_isLocal: false, | ||
_isLocalStorageAvailable: false, | ||
_cookieName: 'test:session', | ||
}, this.owner); | ||
|
||
store.setProperties({ | ||
cookieName: 'test:session', | ||
cookieExpirationTime: 60 | ||
}); | ||
}); | ||
await store.persist({ key: 'value' }); | ||
|
||
expect(cookieService.write).to.have.been.calledWith( | ||
'test:session-expiration_time', | ||
|
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.
Could this be set in
init
instead?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.
I think so, yes 👍