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

fix: update online example to support Redis v4+ and remove outdated dependency #6286

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all 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
54 changes: 40 additions & 14 deletions examples/online/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -4,15 +4,15 @@
// https://redis.io/

// then:
// $ npm install redis online
// $ npm install redis
// $ redis-server

/**
* Module dependencies.
*/

var express = require('../..');
var online = require('online');
var online = require('./online-tracker');
Copy link
Member

Choose a reason for hiding this comment

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

Just remove this entire file and pull the logic into here. It is an example, and examples this small should be easy to follow in a single file.

var redis = require('redis');
var db = redis.createClient();

Expand All @@ -27,9 +27,13 @@ var app = express();
// activity tracking, in this case using
// the UA string, you would use req.user.id etc

app.use(function(req, res, next){
// fire-and-forget
online.add(req.headers['user-agent']);
app.use(async (req, res, next) => {
try {
// fire-and-forget
Copy link
Member

Choose a reason for hiding this comment

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

It is not fire and forget with an await and try/catch. The original design of this was because they didn't want to fail the request if the write failed and still fall back to the response handling.

That said, as an example goes I would prefer we just handle this type of error more robustly to show how. So I would suggest you change the catch to not pass the error to next, just console.error(err) it.

await online.add(req.headers['user-agent']);
} catch (err) {
next(err);
}
next();
});

Expand All @@ -47,15 +51,37 @@ function list(ids) {
* GET users online.
*/

app.get('/', function(req, res, next){
online.last(5, function(err, ids){
if (err) return next(err);
res.send('<p>Users online: ' + ids.length + '</p>' + list(ids));
});
app.get('/', async (req, res, next) => {
try {
const activeUsers = await online.last(5);
res.send('<p>Users online:' + activeUsers.length + '</p>' + list(activeUsers));
} catch (err) {
next(err);
}
});

/* istanbul ignore next */
if (!module.parent) {
app.listen(3000);
console.log('Express started on port 3000');

/**
* Redis Initialization
*/
async function initializeRedis() {
try {
// Connect to Redis
await db.connect();
} catch (err) {
console.error('Error initializing Redis:', err);
process.exit(1);
}
}

/**
* Start the Server
*/

(async () => {
await initializeRedis(); // Initialize Redis before starting the server
if (!module.parent) {
app.listen(3000);
console.log('Express started on port 3000');
}
})();
44 changes: 44 additions & 0 deletions examples/online/online-tracker.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
'use strict';

const { createClient } = require('redis');
Copy link
Member

Choose a reason for hiding this comment

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

You are not using this in this file. Take my other suggestion and move this logic into the index.js and you can just remove this line. Additionally, I dont think any of this code works because you are not calling your exported createOnlineTracker anywhere.


/**
* Factory function to create an OnlineTracker instance
*
* @param {Object} redisClient - The Redis client instance.
* @returns {Object} An object with methods for user activity tracking.
*/

function createOnlineTracker(redisClient) {
// Method to add a user's activity
async function add(user) {
const now = Date.now();
try {
await redisClient.hSet('online_users', user, now.toString());
Copy link
Member

Choose a reason for hiding this comment

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

I am not sure where this came from (was lifted from the online package maybe?), but either way I think this is not the optimal way to achieve this. New versions of redis support expiry on hash keys via hexpire which is likely the optimal way to do this. Either way, this is an forever growing hash which is never good.

If you think jumping to a new feature is a bad idea (which is a valid opinion I think), then you could add some code to remove the items which are expried in the last method.

} catch (err) {
console.error('Error setting user activity:', err);
}
Copy link
Member

Choose a reason for hiding this comment

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

Noticing now that you have both this and the call to add in a try/catch, which is unnecessary.

}

// Method to get the list of users active in the last N minutes
async function last(minutes) {
const threshold = Date.now() - minutes * 60 * 1000;
try {
const users = await redisClient.hGetAll('online_users');
return Object.entries(users || {}).filter(([_, lastActive]) => {
return parseInt(lastActive, 10) >= threshold;
}).map(([id]) => id);
} catch (err) {
console.error('Error retrieving active users:', err);
throw err;
}
}

return {
add,
last,
};
}

// Export the factory function
module.exports = createOnlineTracker;