Skip to content
This repository has been archived by the owner on Dec 12, 2023. It is now read-only.

Add resave option #47

Open
wants to merge 7 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -220,7 +220,9 @@ Here's what the full _default_ module configuration looks like:
// The request-domain is strictly used for the cookie, no sub-domains allowed
domain: null,
// Sessions aren't pinned to the user's IP address
ipPinning: false
ipPinning: false,
// Sessions are saved to the store, even if they were never modified during the request
resave: true
},
api: {
// The API is enabled
Expand Down
7 changes: 3 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,7 @@
"argon2": "^0.30.2",
"dayjs": "^1.11.6",
"defu": "^6.1.0",
"fast-deep-equal": "^3.1.3",
"h3": "^1.0.1",
"unstorage": "^1.0.1"
},
Expand Down
3 changes: 2 additions & 1 deletion src/module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,8 @@ const defaults: FilledModuleOptions = {
options: {}
},
domain: null,
ipPinning: false as boolean|SessionIpPinningOptions
ipPinning: false as boolean|SessionIpPinningOptions,
resave: true
},
api: {
isEnabled: true,
Expand Down
13 changes: 10 additions & 3 deletions src/runtime/server/middleware/session/index.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { deleteCookie, eventHandler, H3Event, parseCookies, setCookie } from 'h3'
import { nanoid } from 'nanoid'
import dayjs from 'dayjs'
import equal from 'fast-deep-equal'
import { SameSiteOptions, Session, SessionOptions } from '../../../../types'
import { dropStorageSession, getStorageSession, setStorageSession } from './storage'
import { processSessionIp, getHashedIpAddress } from './ipPinning'
Expand Down Expand Up @@ -129,17 +130,23 @@ const ensureSession = async (event: H3Event) => {
}

export default eventHandler(async (event: H3Event) => {
const sessionOptions = useRuntimeConfig().session.session as SessionOptions

// 1. Ensure that a session is present by either loading or creating one
await ensureSession(event)
const session = await ensureSession(event)
// 2. Save current state of the session
const source = { ...session }

// 2. Setup a hook that saves any changed made to the session by the subsequent endpoints & middlewares
// 3. Setup a hook that saves any changed made to the session by the subsequent endpoints & middlewares
event.res.on('finish', async () => {
// Session id may not exist if session was deleted
const session = await getSession(event)
if (!session) {
return
}

await setStorageSession(session.id, event.context.session)
if (sessionOptions.resave || !equal(event.context.session, source)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This conflicts with #36, as the updated createdAt would not be saved.
So this should be updated when #36 is merged.

Suggested change
if (sessionOptions.resave || !equal(event.context.session, source)) {
if (sessionOptions.resave || sessionOptions.rolling || !equal(event.context.session, source)) {

Copy link
Contributor Author

@interpretor interpretor Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As this is not straightforward, I would suggest to merge only #48 instead of this and #41.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @interpretor, here you suggest to merge #48 and #41 (and abandon current one #47), while in #48 (comment) you suggest to merge #36, #41 and #47.

Can you please bring your reasoning on those changes and suggest from which one should I start a review? Perhaps #36 or #41?

Copy link
Contributor Author

@interpretor interpretor Dec 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @valiafetisov, I think you misunderstood me. Here I suggest to merge #48, as it is based on #36, #41 and #47. #36 is already merged, and I don't recommend to merge #41 or #47 individually. Instead, I recommend to merge only #48 as it does handle the options from #41 and #47 better.

So it would be best to start the review from #48.

await setStorageSession(session.id, event.context.session)
}
})
})
7 changes: 7 additions & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -82,6 +82,13 @@ export interface SessionOptions {
* @type {SessionIpPinningOptions|boolean}
*/
ipPinning: SessionIpPinningOptions|boolean,
/**
* Forces the session to be saved back to the session store, even if the session was never modified during the request.
* @default true
* @example false
* @type boolean
*/
resave: boolean
}

export interface ApiOptions {
Expand Down