Store attemptedTransition target url as a cookie in fastboot mode #1136
Conversation
DrewCarlson
force-pushed
the
fastboot-redirect
branch
from
8be9159
to
1a99a02
Compare
| @@ -58,6 +58,10 @@ export default Mixin.create({ | |||
| return fastboot ? fastboot.get('isFastBoot') : false; | |||
| }), | |||
|
|
|||
| _cookies: computed(function() { | |||
| return getOwner(this).lookup('service:cookies'); | |||
| }), | |||
There was a problem hiding this comment.
We should lookup the service inside of the sessionAuthenticated method so that we don't add any unnecessary properties via the mixin.
There was a problem hiding this comment.
I had _cookies and _secureCookies as members strictly for mocking in tests. Is there a reference in the test suite for mocking the lookup function?
There was a problem hiding this comment.
Yeah, that makes testing easier for sure. There should be examples in the code base where the container is mocked though.
| @@ -33,12 +33,25 @@ export default Mixin.create({ | |||
| */ | |||
| session: service('session'), | |||
|
|
|||
| _cookies: computed(function() { | |||
| return getOwner(this).lookup('service:cookies'); | |||
| }), | |||
There was a problem hiding this comment.
We should lookup the service inside of the sessionAuthenticated method so that we don't add any unnecessary properties via the mixin.
| } | ||
|
|
||
| return window.location.protocol === 'https:'; | ||
| }).volatile(), |
There was a problem hiding this comment.
We should move this into the sessionAuthenticated method so that we don't add any unnecessary properties via the mixin. Also this will only ever be used when running in FastBoot so window.location.protocol can be ignored.
| if (!this.get('_isFastBoot')) { | ||
| if (this.get('_isFastBoot')) { | ||
| this.get('_cookies').write('ember_simple_auth-redirectTarget', transition.intent.url, { | ||
| path: `/${authenticationRoute}`, |
There was a problem hiding this comment.
authenticationRoute will be the Ember route and would contain dots instead of slashes (e.g. app.internal.login instead of app/internal/login). Is there any way we can get the URL path for the Ember route? If not I think we'll have to set the path to /.
|
|
||
| beforeEach(() => { | ||
| session = InternalSession.create({ store: EphemeralStore.create() }); | ||
| cookiesMock = { | ||
| read() {}, | ||
| write() {}, |
There was a problem hiding this comment.
write shouldn't actually be needed here.
DrewCarlson
force-pushed
the
fastboot-redirect
branch
from
f348583
to
48ea5d1
Compare
|
@marcoow I don't see a simple way to resolve the route's path from name. Let me know if you have any thoughts or other requests. |
DrewCarlson
force-pushed
the
fastboot-redirect
branch
from
48ea5d1
to
e4a3edd
Compare
|
Yeah, it's probably fine to simply set the path to |
|
awesome! |
Fixes #1127