Handle cookies #19
Handle cookies #19
Conversation
| fastbootService.setProperties({ | ||
| _request: request, | ||
| _response: response, | ||
| cookies: cookies |
There was a problem hiding this comment.
Should cookies also be private for now (i.e. _cookies)?
|
LGTM, but I'd like to see this kept as private for now while we settle in on things (same as |
| } | ||
|
|
||
| fastbootService.setProperties({ |
There was a problem hiding this comment.
This still has the problem that cookies are only accessible after the application instance has booted (e.g. you cannot use cookies in initializers/instance initializers). Not sure whether using cookies in (instance) initializer should be considered an anti pattern but I guess it might be surprising to people that they can only access cookies after all instance initializers have run?
There was a problem hiding this comment.
@marcoow Yes, this is definitely a tradeoff of this implementation. I went spelunking in the Ember Application and ApplicationInstance code and it seems there is not an elegant way of setting cookies prior to running instance initializers.
That said, I just re-read the code now and I think I misread last night. I thought that registry creation and running of instance initializers all happened in _bootSync. But actually it looks like __registry__ exists on the application instance before the boot process begins.
I think that means that we could register an object in the registry before the instance initializers are kicked off. It involves reaching pretty deep into private API, so we should think about what we're trying to express here and see if there's an elegant way of exposing it as public API.
There was a problem hiding this comment.
I think restoring authentication state (or user settings etc.) from cookies in (instance) initializers is probably still a pretty common thing to do. In fact, Ember Simple Auth did just that until version 1.0 as well.
I think the ApplicationInstance returned from buildInstance (https://github.com/ember-fastboot/ember-fastboot-server/pull/19/files#diff-ba82abedace362f3becf198dd49dacb4R60) already has the register method so that it should be possible to register the request and response as -request:main and -response:main and inject these into the fastboot service then?
There was a problem hiding this comment.
@marcoow Yes, I think you're right. I'm in the middle of updating the PR now to attempt this.
|
@rwjblue I think it's OK to make
Given that we're pre-1.0, I think we could get away with changing it if we had to. But given how simple it is, I think it's probably OK. |
|
@tomdale - My personal preference is to err on the side of private and make it public once we are certain. I am happy to defer to you on this. |
|
Seems good to me. BTW, I'm starting to feel the desire for some tests in this repo.... |
|
Looks great! I think in the long term there would have to be a public interface to |
|
@rwjblue FWIW all of these new features I'm working on come with tests in the That said, yeah, I agree it's problematic. We should try to think of something. We can at minimum probably write a stub Ember app generator that generates the minimum scaffolding required to trick the server into thinking it's running an Ember app. |
|
@tomdale: I published (a very early version of) an addon that abstracts cookie access so it works both in the browser as well as with Fastboot here: https://github.com/simplabs/ember-cookies |
|
@marcoow Awesome! |
This slightly changes #17 to use the FastBoot service, and does the cookie parsing on the server side.
/cc @rwjblue @marcoow