Making stores async #717

rmachielse · 2015-10-16T12:43:44Z

Replaces #714

This pull request makes stores async by default.
That will allow people to build stores that persist on asynchronous storage (like chrome app storage).

rmachielse · 2015-10-21T08:09:04Z

Hi @marcoow, I rebased and added backwards compatibility with older stores, so I think this branch is ready.

marcoow · 2015-10-21T09:06:16Z

Yeah, saw that - thanks! This will most likely go into 1.1.

LongLiveCHIEF · 2015-11-19T12:49:06Z

👍

marcoow · 2015-11-19T19:06:34Z

@rmachielse: can you rebase this on the latest master?

rmachielse · 2015-11-19T19:43:31Z

@marcoow Done!

marcoow · 2015-11-20T08:03:30Z

tests/unit/internal-session-test.js

-        return session.authenticate('authenticator').then(() => {
-          let properties = store.restore();
-          delete properties.authenticator;
+      it('persists its content in the store', (done) => {


You can simply return a promise from the test instead of using done - I think that's a bit cleaner and also consistent with the rest of the code base then.

I replaced done with return blocks where possible

marcoow · 2015-11-20T11:37:13Z

addon/internal-session.js

+    let result = this.store[method].apply(this.store, params);
+    if (typeof result === 'undefined' || typeof result.then === 'undefined') {
+      Ember.deprecate(`Ember Simple Auth: Synchronous stores have been deprecated. Make sure your custom store's ${method} method returns a promise.`, false);
+      return Ember.RSVP.Promise.resolve(result || {});


Not sure the || {} is actually correct - persist doesn't return anything so this should also not resolve with a value in that case.

Well, || {} it is not really necessary, but it might prevent older custom stores that return nothing in some cases from breaking

If a custom store returns nothing from restore than the error is in the store - this shouldn't be fixed in the internal session. Also now e.g. persist would resolve with {} which doesn't really make sense especially when it actually persisted sth. different than {}.

Alright, I'll just remove that part here

rmachielse · 2015-11-20T12:01:25Z

@marcoow I updated for most of your comments. I'll add some more tests as well

marcoow · 2015-11-20T12:16:59Z

@rmachielse: great!

rmachielse · 2015-11-30T12:50:32Z

@marcoow I've been working on making the session-stores clear in two steps, to make it recoverable in case the invalidation fails. However, this adds much more complexity. No solution will ever be 100% correct, because if session-store#clear is being rejected, we don't know what that means about wether the data is still there or not. I think it would be better to keep the previous behavior, but to throw an exception when unexpected behavior occurs.

You can see my changes on this in the last commit I pushed. What do you think?

marcoow · 2015-11-30T18:05:59Z

Yeah, I think you're right. I think what we should do is to return RSVP.all(cleanStore, invalidateWithAuthenticator) and then the application code would have to figure out what to do when the promise rejects (probably ignore it as there's no way to handle it really).

rmachielse · 2015-12-04T19:12:46Z

@marcoow I've updated for your recent concerns again. The tests fail because of a failure on the latest beta. I think this will happen for master too. Want me to look into it?

marcoow · 2015-12-07T09:25:45Z

@rmachielse: thanks - I'm looking into fixing the build for master. You can rebase this branch then once that's fixed.

marcoow · 2015-12-07T09:34:36Z

addon/session-stores/cookie.js

+      this.restore().then((restoredContent) => {
+        this._lastData = restoredContent;
+        resolve();
+      }, resolve);


Resolving just hides the error in this case. I think the best option here is to just not use restore to set _lastData but simply set it directly so that there would be only one async operation going on here. This of course potentially requires other methods to be updated as well.

I can agree on that, I have updated

marcoow · 2015-12-07T09:37:43Z

Left 2 comments again. Sorry for this taking forever to merge bug it's actually quite a big change and we need to make sure we get it right. Looking forward to merging this soon!

rmachielse · 2015-12-12T15:42:11Z

@marcoow I have updated again. No problem, I'm glad you take this project serious. If we can get this released this year I'm happy!

marcoow · 2015-12-14T17:35:29Z

addon/internal-session.js

+          isAuthenticated: false,
+          authenticator: null
+        });
+        Ember.set(this.content, 'authenticated', {});


I think there must be a this.endPropertyChanges(); after this line as well as otherwise it would remain in the state set by beginPropertyChanges above.

Sorry, missed those, I updated both cases

marcoow · 2015-12-14T17:42:19Z

@rmachielse: just had 2 last comments ;)

I'm currently running the dummy app though and recognized that session syncing doesn't seem to work anymore - don't know why yet, I'm debugging it.

marcoow · 2015-12-14T17:45:05Z

It looks like a promise ends up being stored in the session:

marcoow · 2015-12-14T17:48:02Z

That seems to be a problem with session restoration. It happens as soon as I open a second tab and also seems to be somehow recursive. It seems to keep adding serialized promises into the session until the browser crashes.

marcoow · 2015-12-14T17:51:37Z

{"_id":7492,"_state":1,"_result":{"_id":7488,"_state":1,"_result":{"_id":7484,"_state":1,"_result":{"_id":7484,"_state":1,"_result":{"_id":7480,"_state":1,"_result":{"_id":7476,"_state":1,"_result":{"_id":7472,"_state":1,"_result":{"_id":7468,"_state":1,"_result":{"_id":7464,"_state":1,"_result":{"_id":7460,"_state":1,"_result":{"_id":7456,"_state":1,"_result":{"_id":7452,"_state":1,"_result":{"_id":7448,"_state":1,"_result":{"_id":7444,"_state":1,"_result":{"_id":7440,"_state":1,"_result":{"_id":7436,"_state":1,"_result":{"_id":7432,"_state":1,"_result":{"_id":7428,"_state":1,"_result":{"_id":7424,"_state":1,"_result":{"_id":7420,"_state":1,"_result":{"_id":7416,"_state":1,"_result":{"_id":7412,"_state":1,"_result":{"_id":7408,"_state":1,"_result":{"_id":7404,"_state":1,"_result":{"_id":7400,"_state":1,"_result":{"_id":7396,"_state":1,"_result":{"_id":7392,"_state":1,"_result":{"_id":7388,"_state":1,"_result":{"_id":7384,"_state":1,"_result":{"_id":7380,"_state":1,"_result":{"_id":7376,"_state":1,"_result":{"_id":7372,"_state":1,"_result":{"_id":7368,"_state":1,"_result":{"_id":7364,"_state":1,"_result":{"_id":7360,"_state":1,"_result":{"_id":7356,"_state":1,"_result":{"_id":7352,"_state":1,"_result":{"_id":7348,"_state":1,"_result":{"_id":7344,"_state":1,"_result":…

funny bug (it's actually a lot more - this is just a short excerpt)…

rmachielse · 2015-12-14T17:59:20Z

@marcoow I am able to reproduce this, looking into it now!

…ving async store tests

rmachielse · 2015-12-14T19:02:12Z

@marcoow I found the bug, it turned out I forgot to update _bindToStorageEvents in the addon/session-stores/local-storage.js. Sorry, it should be fixed now!

Making stores async

marcoow · 2015-12-15T09:07:17Z

@rmachielse: thanks a lot for the effort, I'm really happy with the outcome!

rmachielse · 2015-12-15T09:19:32Z

Thank you a lot! Let me know when there's anything else I can do!

sly7-7 · 2016-12-13T11:15:39Z

I guess this is not a big deal, but this results in the inspector promises tab going crazy. (the sync_data function beeing called recursively) (btw, instead of calling cancel + later, calling throttle or debounce do the same job, right ?)

marcoow · 2016-12-13T11:43:56Z

@sly7-7: good point - want to submit a PR that replaces cancel/later with debounce?

sly7-7 · 2016-12-13T14:13:35Z

here you go #1131 😄

rmachielse force-pushed the async-stores branch from 9cec3ff to e3fbf6d Compare October 21, 2015 08:08

marcoow added the enhancement label Oct 21, 2015

marcoow added this to the 1.x milestone Oct 21, 2015

rmachielse force-pushed the async-stores branch from e3fbf6d to a11b7ef Compare November 19, 2015 19:39

marcoow reviewed Nov 20, 2015
View reviewed changes

rmachielse force-pushed the async-stores branch from 2548472 to 1911837 Compare November 20, 2015 11:34

marcoow reviewed Nov 20, 2015
View reviewed changes

rmachielse force-pushed the async-stores branch from 1911837 to c121165 Compare November 20, 2015 12:00

rmachielse force-pushed the async-stores branch from c121165 to 8ac0bc8 Compare November 20, 2015 15:05

rmachielse force-pushed the async-stores branch from 8ac0bc8 to b15c19b Compare November 30, 2015 11:20

rmachielse force-pushed the async-stores branch 2 times, most recently from ddd1ca0 to 7b7a45d Compare December 4, 2015 19:08

marcoow reviewed Dec 7, 2015
View reviewed changes

rmachielse force-pushed the async-stores branch 3 times, most recently from 5eec16e to 53451a5 Compare December 12, 2015 15:39

marcoow reviewed Dec 14, 2015
View reviewed changes

rmachielse added 2 commits December 14, 2015 18:51

Making stores async

6c31a28

Adding backwards compatibility with synchronous stores

1915445

Wrapping async store methods + adding sync store deprecations + impro…

6e55271

…ving async store tests

rmachielse force-pushed the async-stores branch from 53451a5 to 6e55271 Compare December 14, 2015 18:59

marcoow added a commit that referenced this pull request Dec 15, 2015

Merge pull request #717 from rmachielse/async-stores

d8fe402

Making stores async

marcoow merged commit d8fe402 into mainmatter:master Dec 15, 2015

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making stores async #717

Making stores async #717

rmachielse commented Oct 16, 2015

rmachielse commented Oct 21, 2015

marcoow commented Oct 21, 2015

LongLiveCHIEF commented Nov 19, 2015

marcoow commented Nov 19, 2015

rmachielse commented Nov 19, 2015

marcoow Nov 20, 2015

rmachielse Nov 20, 2015

marcoow Nov 20, 2015

rmachielse Nov 20, 2015

marcoow Nov 20, 2015

rmachielse Nov 20, 2015

rmachielse commented Nov 20, 2015

marcoow commented Nov 20, 2015

rmachielse commented Nov 30, 2015

marcoow commented Nov 30, 2015

rmachielse commented Dec 4, 2015

marcoow commented Dec 7, 2015

marcoow Dec 7, 2015

rmachielse Dec 12, 2015

marcoow commented Dec 7, 2015

rmachielse commented Dec 12, 2015

marcoow Dec 14, 2015

rmachielse Dec 14, 2015

marcoow commented Dec 14, 2015

marcoow commented Dec 14, 2015

marcoow commented Dec 14, 2015

marcoow commented Dec 14, 2015

rmachielse commented Dec 14, 2015

rmachielse commented Dec 14, 2015

marcoow commented Dec 15, 2015

rmachielse commented Dec 15, 2015

sly7-7 commented Dec 13, 2016

marcoow commented Dec 13, 2016

sly7-7 commented Dec 13, 2016

Making stores async #717

Making stores async #717

Conversation

rmachielse commented Oct 16, 2015

rmachielse commented Oct 21, 2015

marcoow commented Oct 21, 2015

LongLiveCHIEF commented Nov 19, 2015

marcoow commented Nov 19, 2015

rmachielse commented Nov 19, 2015

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

rmachielse commented Nov 20, 2015

marcoow commented Nov 20, 2015

rmachielse commented Nov 30, 2015

marcoow commented Nov 30, 2015

rmachielse commented Dec 4, 2015

marcoow commented Dec 7, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoow commented Dec 7, 2015

rmachielse commented Dec 12, 2015

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoow commented Dec 14, 2015

marcoow commented Dec 14, 2015

marcoow commented Dec 14, 2015

marcoow commented Dec 14, 2015

rmachielse commented Dec 14, 2015

rmachielse commented Dec 14, 2015

marcoow commented Dec 15, 2015

rmachielse commented Dec 15, 2015

sly7-7 commented Dec 13, 2016

marcoow commented Dec 13, 2016

sly7-7 commented Dec 13, 2016