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.

[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.