Custom Session Stores #190
Conversation
davidpatrick
force-pushed
the
custom-session-stores
branch
from
2b64657 to
7ffdc03
Compare
|
This pull request introduces 2 alerts when merging 7ffdc03 into bcc141a - view on LGTM.com new alerts:
|
davidpatrick
force-pushed
the
custom-session-stores
branch
from
7ffdc03 to
df6e880
Compare
| const { get: getConfig } = require('../lib/config'); | ||
| const { create: createServer } = require('./fixture/server'); | ||
| const redis = require('redis-mock'); | ||
| const RedisStore = require('connect-redis')({ Store: class Store {} }); |
There was a problem hiding this comment.
This is bit of a pain, most session stores expect the express-session instance to define the store interface
There was a problem hiding this comment.
This is a pretty awkward import. We could alternatively have them pass in the connector (default export) with store options, and then expose then instantiated the store.
lib/appSession.js
Outdated
| const { end: origEnd } = res; | ||
| res.end = async function resEnd(...args) { | ||
| await store.set(existingSessionValue, req, res, { | ||
| iat, | ||
| }); | ||
| origEnd.call(res, ...args); | ||
| }; |
There was a problem hiding this comment.
How are storage errors propagated?
There was a problem hiding this comment.
I'm assuming you'd log them on the store instance (or the client instance that the store is using, eg https://github.com/tj/connect-redis#how-to-log-redis-errors)
I don't think express-sessions has a better solution than this
There was a problem hiding this comment.
Hmmm... actually, maybe we do need to do something with the next https://github.com/expressjs/session/blob/master/index.js#L337
I thought res.end was too late to call it but maybe not...
| @@ -1,9 +1,9 @@ | |||
| const Joi = require('@hapi/joi'); | |||
| const clone = require('clone'); | |||
There was a problem hiding this comment.
@adamjmcgrath what is the reason for this clone in the first place? I can't remember.
@davidpatrick what is the reason for this deletion?
There was a problem hiding this comment.
I deleted it because I didn't want to clone the store instance (in case user's added event listeners to it etc.)
I believe it was there to prevent unwanted mutations to the user's config, but objects passed into joi are effectively immutable and mutating the store instance is desirable (adding event listeners etc.) - so I don't think it's necessary (I removed it for the Next SDK as well)
|
In the example i'd like to see how to use this feature with the existing list of |
👍 We can change the example to use something like https://www.npmjs.com/package/memorystore? (don't want the user to have to run something like redis or memcached to get an example running) |
There was a problem hiding this comment.
I added a failing test for the storage error handling 2f213cb
adamjmcgrath
force-pushed
the
custom-session-stores
branch
from
1906696 to
2f213cb
Compare
* Custom Session Stores (#190) * Custom Session Stores * Updates * Add custom store tests * Update custom store tests * missed lock file * clearCookie needs domain and path * updates * storage errors test case * add storage error propagation * Add memorystore example and `auth.Store` helper * Add docs/example, move config option to session config Co-authored-by: adamjmcgrath <adam.mcgrath@auth0.com> * Release 2.3.0-beta.0 (#196) * Release 2.3.0 Co-authored-by: David Patrick <david.patrick@auth0.com>
Introduce the ability for a user to specify a custom session store for the session data to be stored. Custom stores will need to be compatible with express session middleware https://github.com/expressjs/session#compatible-session-stores
See: #143