[2308] Refactor Adaptive store to use DI #2312

BobrImperator · 2021-07-13T21:38:49Z

registers session-stores in Ember's DI system.
refactors AdaptiveStore to use registered CookieStore and LocalStorageStore.
modifies test to use setupTest so there's access to ember's container
the side effect of setupTest is that it seems to require the test to be asynchronous, so all test functions are marked as async so they return a promise implicitly
changes how CookieStore is initialized by implementing _initialize method that handles setting of properties that would normally be provided during creation of CookieStore instance.
changed e.g cookieName property to _cookieName in some tests because using 'non private' property would override the underlying computed property which acts as a proxy, because of this behaviour, the private 'default' values are changed and not the public ones.

marcoow

great work – this is one of the messiest parts of the codebase 🙌

marcoow · 2021-07-23T13:15:52Z

packages/ember-simple-auth/addon/session-stores/adaptive.js

@@ -53,6 +57,8 @@ export default Base.extend({
  */
  localStorageKey: 'ember_simple_auth-session',

+  localStorage: injectStore('local-storage'),


Could this be set in init instead?

this.localStorage = getOwner(this).lookup(`session-store:local-storage`);

I think so, yes 👍

marcoow · 2021-07-23T13:16:14Z

packages/ember-simple-auth/addon/session-stores/adaptive.js

  _cookies: service('cookies'),
+  cookie: injectStore('cookie'),


marcoow · 2021-07-23T13:17:48Z

packages/ember-simple-auth/addon/session-stores/adaptive.js


-      store = this._createStore(Cookie, options);
+      store._initialize(options);


Does

Suggested change

store._initialize(options);

store.setProperties(options);

not work here?

I could check that out, although _initalize currently needs to treat the cookieName property in a special way.

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 cookieName to be cached with a wrong value.

marcoow · 2021-07-23T13:21:28Z

packages/ember-simple-auth/tests/unit/session-stores/adaptive-test.js

+        sinon.spy(cookieService, 'read');
+        sinon.spy(cookieService, 'write');
+        store = createAdaptiveStore(cookieService, {
+          _isLocal: false,


This doesn't seem to exist anywhere actually? 🤔

Yeah, that's true, I will remove it, also noticed that some of the _isFastBoot properties were miswritten, we have _isFastBoot and _isFastboot currently somewhere i believe.

marcoow · 2021-07-23T13:24:14Z

packages/ember-simple-auth/tests/unit/session-stores/shared/cookie-store-behavior.js

@@ -311,7 +302,8 @@ export default function(options) {
      );
    });

-    it('clears cached expiration times when setting expiration to null', function() {
+    // eslint-disable-next-line require-await
+    it('clears cached expiration times when setting expiration to null', async function() {


Is this async in fact necessary?

Yeah, otherwise the mocha test is failing, it's throwing some error saying done() wasn't called after the test finished and it's failing because of that. The test works correctly, the expectations inside in fact are working, though for some reason I wasn't able to debunk why the test has to be async.

It started happening after I've used setupTest, so I believe that it's the cause.

This sounds like we're working around something – this test itself has nothing asynchronous. We should look into this a bit closer – happy to pair on it next week.

OK 👍
I was able to remove it from one of the places, I figured that it was caused by beforeEach calling an async method on store.
Though can't find the cause for the other one so far, although I wouldn't be surprised if it actually be caused by mocha setupTest which expects the hook callback to be thenable https://github.com/emberjs/ember-mocha/blob/master/addon-test-support/ember-mocha/setup-test.js#L21

Ok, let's figure out how we can fix this one as well.

Yeah, for now I've removed the eslint disable line, so it fails PR until it's fixed.

marcoow · 2021-07-23T13:24:53Z

packages/ember-simple-auth/tests/unit/session-stores/shared/cookie-store-behavior.js

    });

-    it('restores expiration time from cookie', function() {
+    // eslint-disable-next-line require-await
+    it('restores expiration time from cookie', async function() {


Is this async necessary? There's nothing asynchronous going on in the test?

packages/ember-simple-auth/addon/session-stores/adaptive.js

marcoow · 2021-07-26T06:11:02Z

packages/ember-simple-auth/addon/session-stores/adaptive.js

+      localStorage.setProperties(options);
+
+      store = localStorage;
+      this.localStorage = localStorage;


Is this used anywhere?

👍 I was under the impression that we want to expose the services as properties as well.

I'll remove that next time.

marcoow · 2021-07-26T06:11:47Z

packages/ember-simple-auth/addon/session-stores/adaptive.js

-      store = this._createStore(Cookie, options);
-      this.set('cookieExpirationTime', store.get('cookieExpirationTime'));
+      store = cookieStorage;
+      this.cookieStorage = cookieStorage;


Is this used anywhere?

marcoow · 2021-07-26T06:16:59Z

packages/ember-simple-auth/tests/unit/session-stores/shared/cookie-store-behavior.js

@@ -311,7 +302,8 @@ export default function(options) {
      );
    });

-    it('clears cached expiration times when setting expiration to null', function() {
+    // eslint-disable-next-line require-await
+    it('clears cached expiration times when setting expiration to null', async function() {


Ok, let's figure out how we can fix this one as well.

packages/ember-simple-auth/addon/session-stores/cookie.js

packages/ember-simple-auth/tests/helpers/create-adaptive-store.js

BobrImperator force-pushed the 2308-adaptive-store-DI branch 6 times, most recently from 56c6516 to d753b58 Compare July 13, 2021 22:29

BobrImperator changed the title ~~[DRAFT] 2308 adaptive store di~~ [2308] Refactor Adaptive store to use DI Jul 13, 2021

BobrImperator mentioned this pull request Jul 15, 2021

Drop node 10 in favor of 12 #2313

Merged

BobrImperator force-pushed the 2308-adaptive-store-DI branch from c4ae621 to d753b58 Compare July 15, 2021 17:11

BobrImperator added 5 commits July 15, 2021 19:11

feat: Add session-stores to DI system

13f3b39

get cookies

a1bfef6

wrap async sideffects in runloop

f46a18d

remove create-cookie-store helper

9c5f974

set cookieExpirationTime when cookie store is initialized

50b1f2b

BobrImperator force-pushed the 2308-adaptive-store-DI branch from d753b58 to 50b1f2b Compare July 15, 2021 17:11

BobrImperator requested a review from a team July 15, 2021 17:12

marcoow reviewed Jul 23, 2021

View reviewed changes

marcoow reviewed Jul 26, 2021

View reviewed changes

BobrImperator added 2 commits July 30, 2021 13:18

refactor to inject on init

f09facb

fix test relying on async setup

d976a4d

BobrImperator force-pushed the 2308-adaptive-store-DI branch from a942305 to d976a4d Compare July 30, 2021 11:18

marcoow reviewed Aug 6, 2021

View reviewed changes

packages/ember-simple-auth/addon/session-stores/cookie.js Show resolved Hide resolved

packages/ember-simple-auth/tests/helpers/create-adaptive-store.js Show resolved Hide resolved

marcoow approved these changes Aug 6, 2021

View reviewed changes

BobrImperator merged commit e797c0a into mainmatter:master Aug 13, 2021

BobrImperator added the internal label Sep 8, 2021

BobrImperator mentioned this pull request Apr 4, 2022

[2365] Remove _initialize method #2366

Merged

This was referenced Apr 29, 2022

sameSite isn't proxied to cookie store when using adaptive store #2230

Closed

add docs and test for sameSite attribute in AdaptiveStorage #2239

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[2308] Refactor Adaptive store to use DI #2312

[2308] Refactor Adaptive store to use DI #2312

BobrImperator commented Jul 13, 2021 •

edited

Loading

marcoow left a comment

marcoow Jul 23, 2021

BobrImperator Jul 23, 2021

marcoow Jul 23, 2021

marcoow Jul 23, 2021

BobrImperator Jul 23, 2021

marcoow Jul 23, 2021

BobrImperator Jul 23, 2021

marcoow Jul 23, 2021

BobrImperator Jul 23, 2021 •

edited

Loading

marcoow Jul 23, 2021

BobrImperator Jul 23, 2021

marcoow Jul 26, 2021

BobrImperator Jul 26, 2021

marcoow Jul 23, 2021

marcoow Jul 26, 2021

BobrImperator Jul 26, 2021

marcoow Jul 26, 2021

marcoow Jul 26, 2021


		store = this._createStore(Cookie, options);
		store._initialize(options);

[2308] Refactor Adaptive store to use DI #2312

[2308] Refactor Adaptive store to use DI #2312

Conversation

BobrImperator commented Jul 13, 2021 • edited Loading

marcoow 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

BobrImperator Jul 23, 2021 • edited Loading

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

Choose a reason for hiding this comment

BobrImperator commented Jul 13, 2021 •

edited

Loading

BobrImperator Jul 23, 2021 •

edited

Loading