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

Improve performance of set method #423

Merged
merged 1 commit into from
Mar 10, 2025
Merged

Improve performance of set method #423

merged 1 commit into from
Mar 10, 2025

Conversation

ozcanovunc
Copy link
Contributor

@ozcanovunc ozcanovunc commented Mar 2, 2025

Hi,

In our system, we store additional keys in Redis and need to sync certain data within session storage. To achieve this, we require a callback that triggers whenever a session is saved to Redis, allowing us to perform operations on our custom keys.

Currently, as a workaround, we have overridden the set method of the Redis store:

const store = new RedisStore({ ... });

const originalSet = store.set;
store.set = async function (...args) {
  const session = args?.[1];

  // Custom logic

  return originalSet.apply(store, args);
};

However, this approach is fragile and requires validation each time a new version of connect-redis is released.

To make this process more robust and maintainable, we propose adding a built-in onSet callback to connect-redis. This would provide a cleaner, more reliable way to execute custom logic when a session is saved.

Let me know if you need refinements!
Thanks in advance 🚀

Above was the original request. Currently this PR aims to call the serializer.stringify when necessary.

@ozcanovunc
Copy link
Contributor Author

@wavded 👀

1 similar comment
@ozcanovunc
Copy link
Contributor Author

@wavded 👀

@wavded
Copy link
Collaborator

wavded commented Mar 6, 2025

@ozcanovunc Sorry I've been busy and I haven't had a chance to look in depth on this. I did have a few initial questions though. One, it seems like this is a general express session feature (firing an event when updating the session) would that be better served in the main express-session repo to include there? Two, are there no events on the Redis clients themselves to capture writes happening? Three, if we kept it in here, it may make sense to follow the event emitter pattern as we inherit it from the session Store instead of using one-off event functions.

@ozcanovunc
Copy link
Contributor Author

Hi @wavded no worries, your comment got me thinking, I can achieve my goal using serializer parameter. I updated this PR to optimize the set method a little.

@ozcanovunc ozcanovunc changed the title Improve Session Sync by Introducing an onSet Callback for Redis Store Improve performance of set method Mar 8, 2025
@wavded wavded self-assigned this Mar 10, 2025
@wavded wavded merged commit 3b28243 into tj:master Mar 10, 2025
3 checks passed
@wavded wavded added the chore label Mar 10, 2025
@wavded
Copy link
Collaborator

wavded commented Mar 10, 2025

Fair enough, I will merge that in.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants