-
-
Notifications
You must be signed in to change notification settings - Fork 17.3k
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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'); | ||
var redis = require('redis'); | ||
var db = redis.createClient(); | ||
|
||
|
@@ -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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is not fire and forget with an 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 |
||
await online.add(req.headers['user-agent']); | ||
} catch (err) { | ||
next(err); | ||
} | ||
next(); | ||
}); | ||
|
||
|
@@ -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'); | ||
} | ||
})(); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,44 @@ | ||
'use strict'; | ||
|
||
const { createClient } = require('redis'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
|
||
/** | ||
* 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()); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure where this came from (was lifted from the 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 |
||
} catch (err) { | ||
console.error('Error setting user activity:', err); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Noticing now that you have both this and the call to |
||
} | ||
|
||
// 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; |
There was a problem hiding this comment.
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.