fix: update online example to support Redis v4+ and remove outdated dependency#6286
fix: update online example to support Redis v4+ and remove outdated dependency#6286vinybrun wants to merge 1 commit intoexpressjs:masterfrom
Conversation
wesleytodd
left a comment
wesleytodd
left a comment
There was a problem hiding this comment.
In general I am happy to see these get updated. Unfortunately there are some things in this PR which likely need to be fixed before we can merge.
| online.add(req.headers['user-agent']); | ||
| app.use(async (req, res, next) => { | ||
| try { | ||
| // fire-and-forget |
There was a problem hiding this comment.
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.
|
|
||
| var express = require('../..'); | ||
| var online = require('online'); | ||
| var online = require('./online-tracker'); |
There was a problem hiding this comment.
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.
| @@ -0,0 +1,44 @@ | |||
| 'use strict'; | |||
|
|
|||
| const { createClient } = require('redis'); | |||
There was a problem hiding this comment.
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.
| async function add(user) { | ||
| const now = Date.now(); | ||
| try { | ||
| await redisClient.hSet('online_users', user, now.toString()); |
There was a problem hiding this comment.
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.
| await redisClient.hSet('online_users', user, now.toString()); | ||
| } catch (err) { | ||
| console.error('Error setting user activity:', err); | ||
| } |
There was a problem hiding this comment.
Noticing now that you have both this and the call to add in a try/catch, which is unnecessary.
|
Okay, appreciate the inputs. I'll work on that changes and re-request review once it's done. |
This comment was marked as spam.
This comment was marked as spam.
1 similar comment
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as spam.
3 participants
This PR updates the Search example to address issues caused by breaking changes in Redis v4+.
The online example was broken due to Redis updates and its reliance on the outdated online package by TJ Holowaychuk, which is no longer maintained and has only 24 weekly downloads.
I considered removing the example or replacing
onlinewith another library but opted to add a new file that substitutes the dependency.Key changes:
onlineby creating a new file,online-tracker.js, which replicates the original functionality.try/catchfor error handling.initializeRedisto make sure that it completes before the server starts.Note: the original
onlinemodule was optimized for performance. The substitute file prioritizes simplicity to make it easier to understand, considering it is now part of an example rather than a standalone module.Tested locally with:
Let me know if any changes are needed!