Clear cached time when setting expiration to null #1463

stevenwu · 2017-10-20T04:39:08Z

@marcoow the issue is that calculateExpirationTime always uses the cached expiration time if there is one.

I've decided to clear the cache when user nulls expiration time, however we get Error: Assertion Failed: Attempting to lookup an injected property on an object without a container, ensure that the object was instantiated via a container.. Thoughts?

#854

marcoow

LGTM; I guess the error is most likely caused by something in the tests not being set up correctly (as the property function now needs the cookies service while it previously didn't).

marcoow · 2017-10-20T07:52:36Z

addon/session-stores/cookie.js

-    if (value < 90) {
+    // When nulling expiry time on purpose, we need to clear the cached value.
+    // Otherwise, `_calculateExpirationTime` will reuse it.
+    if (this.get('_cookies') && value === null) {


The check for this.get('_cookies') should not actually be necessary.

marcoow · 2017-10-20T07:52:57Z

addon/session-stores/cookie.js

+      this.get('_cookies').clear(`${this.get('cookieName')}-expiration_time`);
+    }
+
+    if (value < 90 && value !== null) {


I'd make this an else if for clarity.

stevenwu · 2017-10-25T06:25:49Z

The cookie service is called during init

https://github.com/stevenwu/ember-simple-auth/blob/f8f3a11b5345cc4a9ea21ff544316c9d4f90faef/addon/session-stores/cookie.js#L162-L165

so we need to set the container in tests before that. How do we do that?

marcoow · 2017-10-25T07:45:53Z

This creates a cookie store that has the _cookies service set during init. I think something like that should work?

stevenwu · 2017-11-12T23:17:07Z

I'm unable to run fastboot tests locally, even on master.

  1) Graceful regression tests "before all" hook:
     the object {
  "code": 1
  "errors": [
    "Cannot find module 'ember-cli-preprocess-registry/preprocessors'\n"
    "Error: Cannot find module 'ember-cli-preprocess-registry/preprocessors'\n    at Function.Module._resolveFilename (module.js:536:15)\n    at Function.Module._load (module.js:466:25)\n    at Module.require (module.js:579:17)\n    at require (internal/module.js:11:18)\n    at Object.<anonymous> (/private/var/folders/jw/xxf7ygd91039vt0ysv21fsww0000gp/T/d-1171012-57398-3opn1v.hho57/pristine/node_modules/ember-cli-fastboot/index.js:18:11)\n    at Module._compile (module.js:635:30)\n    at Object.Module._extensions..js (module.js:646:10)\n    at Module.load (module.js:554:32)\n    at tryModuleLoad (module.js:497:12)\n    at Function.Module._load (module.js:489:3)\n"
  ]
  "output": []
  "signal": [null]
} was thrown, throw an Error :)
  Error: the object {
    "code": 1
    "errors": [
      "Cannot find module 'ember-cli-preprocess-registry/preprocessors'\n"
    ]
    "output": []
    "signal": [null]
  } was thrown, throw an Error :)
      at tryCatch (node_modules/rsvp/dist/rsvp.js:525:12)
      at invokeCallback (node_modules/rsvp/dist/rsvp.js:538:13)
      at publish (node_modules/rsvp/dist/rsvp.js:508:7)
      at publishRejection (node_modules/rsvp/dist/rsvp.js:443:3)
      at flush (node_modules/rsvp/dist/rsvp.js:2400:5)

  2) Integration tests "before all" hook:
     Error: Could not find fixtures directory. Make sure you have a fixtures directory in your `test/` directory. You may encounter this issue if you have npm linked this package; copy it to your node_modules directory instead.
      at findFixturesPath (node_modules/ember-cli-addon-tests/lib/utilities/copy-fixture-files.js:15:11)
      at copyFixtureFiles (node_modules/ember-cli-addon-tests/lib/utilities/copy-fixture-files.js:23:20)
      at node_modules/ember-cli-addon-tests/lib/models/addon-test-app.js:30:9
      at tryCatch (node_modules/rsvp/dist/rsvp.js:525:12)
      at invokeCallback (node_modules/rsvp/dist/rsvp.js:538:13)
      at publish (node_modules/rsvp/dist/rsvp.js:508:7)
      at flush (node_modules/rsvp/dist/rsvp.js:2400:5)
      at _combinedTickCallback (internal/process/next_tick.js:131:7)
      at process._tickCallback (internal/process/next_tick.js:180:9)

marcoow · 2017-11-13T15:59:26Z

Yeah, I can reproduce the FastBoot tests error - I'll fix that.

marcoow · 2017-11-13T19:51:50Z

Hm, I cannot reproduce this anymore now - both

git clean -dfx
yarn --pure-lockfile
npm run test:fastboot

as well as

git clean -dfx
yarn --no-lockfile
npm run test:fastboot

work for me. Can you check whether git clean -dfx does fix it for you @stevenwu?

stevenwu · 2017-11-13T20:23:46Z

I still have the error, even on a fresh clone. I'm on node 8.9.1 and npm 5.5.1.

marcoow · 2017-11-13T20:24:52Z

Hm, that's very strange. What's your global ember-cli version?

stevenwu · 2017-11-13T20:27:04Z

2.16.2

stevenwu · 2017-11-13T20:29:03Z

I get this though, if I run yarn --pure-lockfile and not yarn

~/temp/ember-simple-auth master
❯ ember --version
module.js:538
    throw err;
    ^

Error: Cannot find module 'broccoli-merge-trees'
    at Function.Module._resolveFilename (module.js:536:15)
    at Function.Module._load (module.js:466:25)
    at Module.require (module.js:579:17)
    at require (internal/module.js:11:18)
    at Object.<anonymous> (/Users/steve/temp/ember-simple-auth/node_modules/ember-cli/lib/broccoli/merge-trees.js:2:28)
    at Module._compile (module.js:635:30)
    at Object.Module._extensions..js (module.js:646:10)
    at Module.load (module.js:554:32)
    at tryModuleLoad (module.js:497:12)
    at Function.Module._load (module.js:489:3)

Test results will be the same.

marcoow · 2017-11-14T07:54:45Z

Did you clean your node_modules, dist and tmp folders (git clean -dfx will completely clean your repo).

stevenwu · 2017-11-15T18:25:01Z

I can run the tests with node 8 and npm 4, so I think there's an issue with npm 5.

marcoow · 2017-11-15T19:11:58Z

Hm, ok - as that's almost certainly not an issue with ESA let's ignore it for now as you can run the tests now.

stevenwu · 2017-11-17T07:46:22Z

addon/session-stores/cookie.js

+    // When nulling expiry time on purpose, we need to clear the cached value.
+    // Otherwise, `_calculateExpirationTime` will reuse it.
+    if (value === null) {
+      this.get('_cookies').clear(`${this.get('cookieName')}-expiration_time`);


@marcoow I don't know why this is causing

1) Graceful regression tests does not error when using the adaptive store: AssertionError: expected undefined to deeply equal [ Array(1) ]

which asserts

expect(response.headers['set-cookie']).to.deep.equal(['ember_simple_auth-session=%7B%22authenticated%22%3A%7B%7D%7D; path=/'])

Try reproducing this outside of the tests - run ember s and have a look at the response.

marcoow

Seems good - the failing FastBoot test seems strange though…

marcoow · 2017-11-17T12:58:19Z

addon/session-stores/cookie.js

-    if (value < 90) {
+    // When nulling expiry time on purpose, we need to clear the cached value.
+    // Otherwise, `_calculateExpirationTime` will reuse it.
+    if (value === null) {


I'd use isNone here.

marcoow · 2017-11-24T17:00:51Z

The failing FastBoot test is due to the store creation logic being pretty broken - in particular with the adaptive store that the graceful regression test uses. I tried to do a quick "fix" but that should all be properly refactored. I'll do that in a separate PR.

marcoow · 2017-12-07T19:51:58Z

tests/unit/session-stores/adaptive-test.js

@@ -26,7 +25,7 @@ describe('AdaptiveStore', () => {
    beforeEach(function() {
      store = Adaptive.extend({
        _createStore(storeType, options) {
-          return this._super(storeType, assign(options, { _isFastBoot: false }));
+          return LocalStorage.create({ _isFastBoot: false }, options);


This is all very bad of course but will be fixed in #1490

marcoow reviewed Oct 20, 2017

View reviewed changes

stevenwu changed the title ~~[wip] Clear cached time when setting expiration to null~~ Clear cached time when setting expiration to null Oct 27, 2017

stevenwu added 5 commits November 15, 2017 10:27

Clear cached time when setting expiration to null

f73c033

Fix cookie name in test

1b9992d

Fix adaptive store test

217fb7a

Set fastboot flag

11f6616

Refactor adaptive stubs to match previous behaviour

cb493e3

stevenwu force-pushed the fix-cached-expiration branch from ba95782 to cb493e3 Compare November 15, 2017 18:28

stevenwu commented Nov 17, 2017

View reviewed changes

marcoow reviewed Nov 17, 2017

View reviewed changes

marcoow added 2 commits December 7, 2017 20:13

prefer isNone over null comparison

3facd4b

fix fastboot tests

73b9bd9

marcoow reviewed Dec 7, 2017

View reviewed changes

marcoow approved these changes Dec 7, 2017

View reviewed changes

marcoow merged commit f867f24 into mainmatter:master Dec 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Clear cached time when setting expiration to null #1463

Clear cached time when setting expiration to null #1463

stevenwu commented Oct 20, 2017

marcoow left a comment

marcoow Oct 20, 2017

marcoow Oct 20, 2017

stevenwu commented Oct 25, 2017

marcoow commented Oct 25, 2017

stevenwu commented Nov 12, 2017

marcoow commented Nov 13, 2017

marcoow commented Nov 13, 2017

stevenwu commented Nov 13, 2017

marcoow commented Nov 13, 2017

stevenwu commented Nov 13, 2017

stevenwu commented Nov 13, 2017 •

edited

Loading

marcoow commented Nov 14, 2017

stevenwu commented Nov 15, 2017

marcoow commented Nov 15, 2017

stevenwu Nov 17, 2017 •

edited

Loading

marcoow Nov 17, 2017

marcoow left a comment

marcoow Nov 17, 2017

marcoow commented Nov 24, 2017

marcoow Dec 7, 2017

Clear cached time when setting expiration to null #1463

Clear cached time when setting expiration to null #1463

Conversation

stevenwu commented Oct 20, 2017

marcoow left a comment

Choose a reason for hiding this comment

marcoow Oct 20, 2017

Choose a reason for hiding this comment

marcoow Oct 20, 2017

Choose a reason for hiding this comment

stevenwu commented Oct 25, 2017

marcoow commented Oct 25, 2017

stevenwu commented Nov 12, 2017

marcoow commented Nov 13, 2017

marcoow commented Nov 13, 2017

stevenwu commented Nov 13, 2017

marcoow commented Nov 13, 2017

stevenwu commented Nov 13, 2017

stevenwu commented Nov 13, 2017 • edited Loading

marcoow commented Nov 14, 2017

stevenwu commented Nov 15, 2017

marcoow commented Nov 15, 2017

stevenwu Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

marcoow Nov 17, 2017

Choose a reason for hiding this comment

marcoow left a comment

Choose a reason for hiding this comment

marcoow Nov 17, 2017

Choose a reason for hiding this comment

marcoow commented Nov 24, 2017

marcoow Dec 7, 2017

Choose a reason for hiding this comment

stevenwu commented Nov 13, 2017 •

edited

Loading

stevenwu Nov 17, 2017 •

edited

Loading