Refactor session handling

[interpretor]

This combines #36, #41 and #47 and refactors the handling of (modified) session.

Instead of listening to event.res.on('finish'), it uses the resEndProxy from #41 to save changes made to the session.
This has a few advantages, because it is called at the very end of all event handlers, but before sending the response to the client:

• Setting cookies at the very end of all event handlers is possible. This e.g. is necessary to implement saveUninitialized.
• All storage operations will exit before the client gets his response. This prevents race conditions.

To reduce storage operations, I extracted the setStorageSession() from newSession() and created setSession(). newSession() will now just initialize a new session and publish it at the event.context, but will be saved only when every event handler has finished (with calling setSession()). Before, setStorageSession() has been called twice: First while creating a new Session instance, then at the event.res.on('finish') event handler.

[BracketJohn]

Hey @interpretor!

Thanks for the contribution - usually I would prefer to merge the PRs individually with one change (feat, fix, docs, ...) per PR. Is there a strong reason to doing all of this in one go?

[interpretor]

Hey @BracketJohn,

if you want you can merge the PRs #36, #41 and #47 individually and then merge this PR on top, that should work. The restructuring of the session handling here bases on the individual PRs, so I had to merge them in my dev branch first. It also removes redundant hooks (event.res.on('finish') to save changes, resEndProxy only for saveUninitialized). The individual PRs don't change anything at the current behavior of the module, as they are active only when setting the new options.

[valiafetisov]

I suggest to move copying logic this into the getSession so ensureSession either creates a new one or returns a copy of the old

[interpretor]

Moving the copying logic to getSession would not work, as the oldSession object has to be different from the one saved to the event context. We could return a copy of the session after saving a reference to the event context in ensureSession.

[valiafetisov]

Overwriting standard node js methods is obviously not recommended. Is there a better way doing that?

• What is the exact type error that is thrown here?
• Also, I'm not sure if res.end can be async, since it is expected to return this according to the docs I don't think it's being awaited. Was it at least tested with long timeouts?
  • also the express session doesn't make it async

[interpretor]

It seems that there is no alternative, as already mentioned in #41. There is no hook at the end of all event handlers, and no event that fires just before sending the response.

• res.end returns always a ServerResponse, and here it returns a Promise<ServerResponse>. Also there are multiple overloads, and res.end also accepts a callback. In express session this leads to problems, so they decided to ditch the callback, as it is very rarely used.
• The types don't allow res.end being async, but it works perfectly. I tested with long timeouts with various conditions, so it must be awaited, but I didn't check it in the node source code.

[valiafetisov]

Why do you need to typecast it?

[interpretor]

res.end.call() returns any, not ServerResponse

[valiafetisov]

I think it's get confusing here, as you got too many session names: session, oldSession, newSession and storedSession (where oldSession is in actuality can be newly created or existing one)

Please propose a better naming for those

[valiafetisov]

• Same here, you need to provide better explanation
• Also expressjs/session recommends false by default

[interpretor]

expressjs/session recommends false, but the current behavior of @sidebase/nuxt-session is true, so I've chosen to not change the default behavior. It is also recommended to set resave to false.

We can change it if you want.

[greym0uth]

Is there any help I can provide on this to help push it through? Currently the library clears sessions if any network requests happen directly after the session is saved (i.e a redirect) as the getSession returns the old session and overwrites the previously updated session. Tried with this implementation and it solves the issue, also the resave option is very valuable to have available.

[greym0uth]

@zoey-kaiser @BracketJohn how does this look to you?

[BracketJohn]

Hey @greym0uth 👋

Thanks for reaching out + pushing this. I agree that having this PR merged would be very valuable. As it is quite a complex change and @interpretor + @valiafetisov have already done quite a lot of iterations, I do want @valiafetisov to finish his review and approve. Then we can merge without further reviews from my side + do a release right away.

@valiafetisov can you look into this on Monday or Tuesday to see if there are any major blockers left?

Thanks @greym0uth and @interpretor for your patience - we've sadly been super occupied with other libs, modules, community work, ... :/

[interpretor]

Sadly I've been experiencing issues while testing with this PR and SSR because of making res.end() async. I'll push a commit once refactured.

[valiafetisov]

The code looks good except for the hack on which it depends. This line:

• Overwrites internal node js method response.end
  • It's dangerous, but potentially acceptable on it's own, as express session also does it
• Overwrites sync method response.end with async method
  • I suspected that it's not going to work, since we can not expect that node.js internal logic will await a method that suppose to be synchronous
    • I now treat this comment as indication that I was right 🙂

Conclusion: I don't recommend merging this PR since it uses undocumented node.js behaviour, which might break core functionality of this package. Looking forward to the refactoring hinted by @interpretor to reevaluate this intermediate conclusion ✨

[greym0uth]

Overwrites sync method response.end with async method

What if instead of overriding it with a async function we expose an async save() function on the context.session object (essentially doing the same thing as express-session). That way if the session needs to be saved, during something like a redirect scenario, it can be saved and awaited manually before the redirect response is sent. If this sounds promising I'll spin up a PR so we can have a separate thread around that.