Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Optimize session deletion, recreation, and persistence #10

Merged
merged 2 commits into from
Apr 21, 2024

Conversation

callmetwan
Copy link

@callmetwan callmetwan commented Apr 7, 2024

This work optimizes control flow by only rotating the session key or persisting data if the session is not deleted. The flow before this work would recreate the session key/persist session data every time, even if it would end up being deleted. This is important if storing the session in a database, as that means there is at least one additional round trip. For larger systems this could mean cutting the number of database interactions in half.

Tangential, the bun dev script has been split into two to reflect the two types of servers you can run.

Before merging I would love it if someone else could test this as well. Not being overly familiar with the library and there being no unit tests makes me nervous I missed something 😅

@callmetwan callmetwan marked this pull request as draft April 7, 2024 21:45
@callmetwan callmetwan marked this pull request as ready for review April 7, 2024 21:45
Comment on lines +103 to +105
const shouldDelete = session.getCache()._delete;
const shouldRotateSessionKey = c.get("session_key_rotation") === true;
const storeIsCookieStore = store instanceof CookieStore;
Copy link
Author

Choose a reason for hiding this comment

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

The original conditionals were logical but a bit challenging for me to grok. I'm a big fan of abstracting conditionals into well named variables. I personally lean towards verbosity over succinctness to make the intention really clear.

That being said, happy to make changes if there is different stylistic preference on how to write these conditionals.

@callmetwan
Copy link
Author

A few other notes:

  • There wasn't a style guide. I used prettier to format just my changes. Happy to turn use another formatting tool, I just didn't see anything required/referenced in the repo
  • I lean towards verbosity over succinctness in variable naming to very clearly illustrate intent.
  • I left larger docblock style comments to also illustrate intent. I specifically leave out variable name references in comments so the comment doesn't atrophy/make the code reading experience confusing down the line.

Copy link
Author

Choose a reason for hiding this comment

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

None of these changes are intended to alter the functionality, just re-organize the code so we aren't unnecessarily updating the session store if we don't need to.

@jcs224
Copy link
Owner

jcs224 commented Apr 9, 2024

Looks like something broke the cookie driver.

image

@callmetwan
Copy link
Author

I'll take a look later this week

@callmetwan
Copy link
Author

Hmmm, running it locally all the tests pass. Here is the node cookie test for example
image

It seems to be failing on the incorrect password step but I verified in Chrome and FF (on a Mac) that they work as expected. Is there a way to re-run the tests in GH actions? I looked but didn't see any options.

@jcs224
Copy link
Owner

jcs224 commented Apr 9, 2024

Weird. Did you change the STORE environment variable to cookie before running the test?

I'll run the action again.

@jcs224
Copy link
Owner

jcs224 commented Apr 9, 2024

Still failing, I'll check this out in the next couple days.

@callmetwan
Copy link
Author

Ah, I didn’t realize I needed to pass an encryption key. I’ll remind myself to add this to the testing section of the README

@callmetwan
Copy link
Author

Okay, think I fixed it! Had a ! in the wrong place

@jcs224
Copy link
Owner

jcs224 commented Apr 10, 2024

Nice! I might play with it a little before I merge it in, but this looks good, thanks!

@callmetwan
Copy link
Author

Please do, I want to be 100% sure I didn't mess anything up unintentionally! And glad to do it :)

@jcs224 jcs224 merged commit 5e87063 into jcs224:main Apr 21, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants