Open sripberger opened 5 years ago
The app reference is required, for use in context.js::emit()
The emit
class method is called from valid()
, which validates a session as expired or valid.
As for the storage handling, the storage is managed by the session object, primarily the Context
. While storing session data in a cookie is optional (for so many reasons very bad), the preferred choice would be to store it in some sort of persistent storage, such as *SQL, Redis, NoSQL, etc. You set this by passing through a valid store
object when you instantiate the session middleware.
Example
let session = new session({
store: () => {
_store = [];
function get(key) {
return _store.find(e => e.key === key);
}
...
return {get: get, set: set, destroy: destroy};
},
...
});
I do agree that the app
object shouldn't be deep referenced in the way that it is, just for using the emit
function. The issue reporter is looking for functionality where the session isn't implicitly bound to the request, so that not every request has a session, or is a session, therefore setting unnecessary cookies.
I appreciate the detailed response, of course, though I'd point out that the change which added the app reference to the middleware creation signature predates those emit
calls by two years. As stated in the referenced PR #59, original reason was a performance optimization. My guess is that the events were emitted through that reference later as a sort of "might as well use this" kind of thing.
In Koa, though, the ctx object itself contains a reference to the app which could easily have been used for this purpose:
emit(event, data) {
setImmediate(() => {
this.ctx.app.emit(`session:${event}`, data);
});
}
The problem with the performance optimization in #32 is that it makes a bunch of changes to the app object immediately upon the creation of the middleware, before any ctx objects exist, so it has to reference the app separately from those. Either that, or resort to some trickery that makes said changes lazily when the first request is received after startup.
This latter approach would not deal with the problem I am having, though, which is that I simply don't want the session
getter to exist in the app.context
object, which acts as the prototype for all ctx objects in the entire app. Not all parts of my app will be designed to use this cookie-based session data, and I want ctx.context
to return undefined
in those parts where it should not be used.
As for the storage thing, I agree that storing session data in cookies is generally bad, but for some context-- What I'm doing is re-writing some legacy REST endpoints in Node. The goal is to reproduce their functionality exactly -- so that we don't break our front ends which are unfortunately coupled to them rather extensively-- then add more a sensible graphql endpoint at a different path so we can re-write our front ends around that at a later date.
The legacy endpoints use an encrypted cookie to store a CSRF token in the user's browser. The browser never reads them-- only the sever does, so I suppose I could store them on the back end, but the front end assumes that they will never expire until the user closes their browser-- an event which the server can have no knowledge of whatsoever.
The reason it can't expire earlier is because the front end is a single-page app that gets the CSRF token from a global variable which is injected into a script tag when serving the page. The app itself never at any point requests new tokens. So if they have an expiration time, the app will break until the user refreshes the page when that expiration time passes.
So unfortunately it seems like we have to keep using the cookie for now.. So I'm aiming to reproduce it (but again, only for the legacy endpoints) with a signed cookie handled by this middleware (or another similar one).
Ideally, because this functionality is legacy and kind of gross to me, it will not leak into other parts of the app in any way, so that new front ends won't be sending and receiving this (for their purposes) completely unused cookie data, and so that middlewares and handlers in any new endpoints do not mistakenly read or write from ctx.session
, which is not supposed to be a thing for said endpoints.
I hope that makes sense?
Given this further discussion of the issue, I think it makes sense to rename this to make it a bit more clear.
It's also something that I've noticed, that there's no way to 'bind' the middleware to the app contextually, it's enforced. I also get your use case, and it makes sense, because I've used those cases before.
A few other libraries have their own prescribed idea of how to manage sessions, be it entirely at the whim of the developer when and where, or wrap the app like this one.
Good point on the pre-ctx object. I'll need to dig deeper into the koajs
code to understand that a bit better.
@sripberger is 100% right the middleware should not be coupled to the app, it's really easy to change the emit logic to use ctx.app
. Otherwise, it's confusing, how the session can be accessed via ctx.session
on routes where session was not even installed. what are performance benefits anyway, the current logic defines session symbol on the context constructor but how much gain is there from that
it would be a breaking change though
https://github.com/koajs/session/blob/f81d7130ff4fd44ad97bd0b807034bd46964db8a/test/cookie.test.js#L668-L695
@sripberger you can use this forke https://github.com/idiocc/session you're welcome
@sripberger you can use this forke https://github.com/idiocc/session you're welcome
Thanks!
It turns out I'm no longer using this package at all, since I was able to convince my company to start with a brand new product-- complete with brand new clients-- rather than doing this lengthy and error-prone transition to keep support for the legacy clients. So there's no longer any need for session data cookies that are used in one part of the app, but not in another.
I appreciate that someone shares my thoughts about this situation, though. :+1:
As discussed in #59, a reference to the Koa app is currently accepted so the middleware initializer can modify it for performance reasons. This makes sense, but it sort of couples the middleware to the app, making it tricky to use with other middleware consumers such as koa-router.
For example, if I want to use
koa-router
to define different behavior for different url paths-- some paths using sessions, others not. Even if I only attach the middleware for some paths, thectx.session
andctx.sessionOpts
getters will still exist on all context objects all paths, even though they shouldn't.In fact, omitting the middleware doesn't even seem to remove much of the functionality at all. Without it, I could still get and commit session data from cookies as normal. Looking at the implementation, it seems like the middleware itself only really handles external storage initialization and the
autoCommit
option:https://github.com/koajs/session/blob/10bb12246699101a0c87a2f3e2e09b1a79e10e33/index.js#L37-L50
I suppose the answer here is to just not access those properties where they're not supposed to exist, but it feels messy and likely to confuse other developers who might work on my code who aren't familiar with the internals of
koa-session
.I'd like to use
koa-session
since it is clearly the most popular and well-maintained module in this space, but at the moment this is a huge blocker for me. I'd be willing to take the performance hit in order to avoid these problems, though I'm aware not everyone feels that way.Would you guys be open to the possibility of making the
app
argument optional in the initializer, falling back to the slower way of doing things if it is not provided? I imagine it will be a decent amount of work to make this change, but I'm happy to do it if you'd be receptive to it.If not I'll just save my time and use
koa-session2
. :\