Skip to content

💥 Bump redis version to 5.0.0 #24

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.

Sign up for GitHub

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

Merged
merged 6 commits into from
May 20, 2025
Merged

💥 Bump redis version to 5.0.0 #24

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
7f1b1ed
💥 Bump redis version to 5.0.0
dawidreedsy May 19, 2025
282a37e
Change version to 5.1.0
dawidreedsy May 20, 2025
ab8252d
Remove changes to node versions in tests
dawidreedsy May 20, 2025
da7857c
Fix issue with hanging tests
dawidreedsy May 20, 2025
51ef0cc
🚨 Fix linter errors
dawidreedsy May 20, 2025
abc1d92
Add deprecation comment
dawidreedsy May 20, 2025
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
7 changes: 6 additions & 1 deletion .github/workflows/test.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,14 +10,17 @@ on:

jobs:
test:
name: Node.js ${{ matrix.node }}
name: Node.js ${{ matrix.node }} - Redis ${{ matrix.redis }}
runs-on: ubuntu-latest
strategy:
matrix:
node:
- 16
- 18
- 20
redis:
- 4
- 5
services:
redis:
image: redis
Expand All @@ -31,6 +34,8 @@ jobs:
node-version: ${{ matrix.node }}
- name: Install
run: npm install
- name: Install correct redis version
run: npm install redis@${{ matrix.redis }}
- name: Lint
run: npm run lint
- name: Test
Expand Down
29 changes: 24 additions & 5 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,10 +42,19 @@ RedisPubSub.prototype.close = function(callback) {
};

RedisPubSub.prototype._close = function() {
return this._closing = this._closing || this._connect().then(Promise.all([
this.client.quit(),
this.observer.quit()
]));
var pubsub = this;

if (!this._closing) {
this._closing = this._connect()
.then(function() {
return Promise.all([
close(pubsub.client),
close(pubsub.observer)
]);
});
}

return this._closing;
};

RedisPubSub.prototype._subscribe = function(channel, callback) {
Expand Down Expand Up @@ -90,5 +99,15 @@ function connect(client) {

var PUBLISH_SCRIPT =
'for i = 2, #ARGV do ' +
'redis.call("publish", ARGV[i], ARGV[1]) ' +
'redis.call("publish", ARGV[i], ARGV[1]) ' +
'end';

function close(client) {
if (client.close) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think technically .quit() is only deprecated for Redis server >=7.2:

As of Redis version 7.2.0, this command is regarded as deprecated.

It can be replaced by just closing the connection when migrating or writing new code.

I think I'd read this notice as anyone using Redis server <7.2 should still use .quit(), regardless of Node.js client version(?).

Not sure how we want to handle this?

Copy link
Contributor Author

@dawidreedsy dawidreedsy May 20, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This is the note in the Redis docs:

Note: Clients should not use this command. Instead, clients should simply close the connection when they're not used anymore. Terminating a connection on the client side is preferable, as it eliminates TIME_WAIT lingering sockets on the server side.

The redis-node code does seem to just close the socket connection. I guess it is the same thing as closing connection to internet.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@alecgibson @ericyhwang
This is a breaking change anyway:

Maybe we can just drop support for node-redis < 5.0.0 then we can just use close.

Copy link
Contributor

Choose a reason for hiding this comment

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

Okay turns out redis@5 doesn't support Redis server <7.2, so I guess it doesn't matter if older server users should still use .quit(), because those consumers should still be on redis@4.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we shouldn't make this change breaking. It's nice to let people use the older version if possible, especially because it makes the migration path easier to keep the shim around. And since it's super easy for us, I don't see why not.

return client.close();
}

// The quit is deprecated for node redis >= 5.0.0
// This call should be removed after we stop supporting redis < 5.0.0
return client.quit();
}
4 changes: 2 additions & 2 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
{
"name": "sharedb-redis-pubsub",
"version": "5.0.1",
"version": "5.1.0",
"description": "Redis pub/sub adapter adapter for ShareDB",
"main": "index.js",
"dependencies": {
"redis": "^4.0.0",
"redis": "^4.0.0 || ^5.0.0",
"sharedb": "^1.0.0 || ^2.0.0 || ^3.0.0 || ^4.0.0 || ^5.0.0"
},
"devDependencies": {
Expand Down