redis / node-redis

Redis Node.js client
https://redis.js.org/
MIT License
16.86k stars 1.87k forks source link

Migration Guide and Breaking Changes is not full enough #1765

Open npdev453 opened 2 years ago

npdev453 commented 2 years ago

So, only way for now (without modifying thousands of files) is patch client object like this, or remap keys with .toLowerCase():

    client.hgetall = client.hGetAll;
    client.hget = client.hGet;
    client.hset = client.hSet;
    client.hmset = client.hSet;
    client.setex = client.setEx;
    ...
 client.hSet('key', {a: 1, b:2}, callback); 

< ./node_modules/@node-redis/client/dist/lib/commander.js:72
        yield `$${byteLength.toString()}${DELIMITER}`;
                             ^

TypeError: Cannot read properties of undefined (reading 'toString')
    at encodeCommand (./node_modules/@node-redis/client/dist/lib/commander.js:72:30)
    at encodeCommand.next (<anonymous>)
    at RedisSocket.writeCommand (./node_modules/@node-redis/client/dist/lib/client/socket.js:56:20)
    at Commander._RedisClient_tick (./node_modules/@node-redis/client/dist/lib/client/index.js:415:64)
    at Commander._RedisClient_sendCommand (./node_modules/@node-redis/client/dist/lib/client/index.js:396:82)
    at Commander.sendCommand (./node_modules/@node-redis/client/dist/lib/client/index.js:349:93)
    at Commander.<computed> (./node_modules/@node-redis/client/dist/lib/client/index.js:383:14)

But previously, on 3.x its works:

client.hmset('key', {a: 1, b: 2}, callback); 

< HMSET key a 1 b 2

Functionality hiddenly broken, and no alternative provided.

Current solution is writing a wrap for hmset like this:

client.hmset = (hkey, parms, callback) => {
    client.hSet(hkey, Object.entries(parms).flat(), callback)
}

Also, I think that users can additionally import something like LegacyTypesHelper module that will override signatures until legacyMode enabled.



Finally, I should say that is very sad example of "how modules shouldn't make breaking updates", especially of so popular module. Feels like a spit into consumers.

AnnAngela commented 2 years ago

I agree with you, and I have encountered the same first problem as you these days right after my pr.

To be honest, I think that as a package with millions of downloads every week, if developers want to release a new major version, they should complete the change log and migration guide before proceeding. Because this will cause a big impact, but only developers can quickly find all changes, it is difficult for us as users to find all changes.

wavded commented 2 years ago

I maintain the connect-redis package. v4 is breaking enough that it is no longer interoperable with packages like ioredis and redis-mock (which are also both supported). I thought using legacyMode would allow a period for connect-redis users to migrate and support both Redis v3 or v4 as well the other redis clients but it doesn't really work as this case points out, there are methods missing (.e.g. mget) and signatures are incomplete. At this point I think we have to support and recommend v3 until we have something more interoperable. Are there plans to flesh out legacyMode further?

ebebbington commented 2 years ago

Also doesn't mention:

npdev453 commented 2 years ago

Also doesn't mention:

  • What happened to import { RedisClient }

Thanks, forgot about it. Already added to the list

jimsnab commented 2 years ago

The migration guide should explain how to modify RedisClient for purposes of unit test mocks.

(Was it really necessary to prevent a RedisClient object from being modified or extended?)

jimsnab commented 2 years ago

Another thing for the migration guide. SCAN args have changed, requiring client.SCAN(cursor, { MATCH: keyPattern, COUNT: pageSize }), pageSize is a Number. It used to take client.SCAN(cursorString, 'MATCH', keyPattern, 'COUNT', pageSize.toString()).

And the response has changed. Instead of an array of strings [<cursor>, <keys>], it now returns a structure {cursor: <number>, keys: <keys>}.

npdev453 commented 2 years ago

The migration guide should explain how to modify RedisClient for purposes of unit test mocks.

(Was it really necessary to prevent a RedisClient object from being modified or extended?)

Could you please explain in details? You are free to modify client object after calling const client = createClient();

jimsnab commented 2 years ago

My unit tests had class MockRedisClient extends redis.RedisClient {.

This has to be completely reworked.

jayeclark commented 2 years ago

Anyone know where can I find the old V3 docs? While the changelog and migration guide are very helpful at telling me how things work in the new version compared to the previous one, I'm having trouble finding the documentation on what hasn't changed. There are a lot of gaps to fill if you aren't someone who is migrating to V4 but instead are starting off with V4.

Also the docs here need to be updated or - probably better - clearly identified as relevant to V3 - I've only been tinkering the repo for a couple hours so far, and almost every example there that I had to use is now out of date. 😄

leibale commented 2 years ago

@jayeclark the old docs can be found in the v3.1 branch. I'll make sure that https://docs.redis.com/latest/rs/references/client_references/client_nodejs/ will be updated. If you can prepare a list of what is missing in the docs it'll be very helpful.

jayeclark commented 2 years ago

@leibale oh, that makes perfect sense - I can’t believe I didn’t think to look there. 🤦 Sure, I can make a list as I go through - may not be comprehensive but it should be a good start especially for newer users.

OxleyS commented 2 years ago

Using this workaround for expressing the client type:

type RedisClient = ReturnType<typeof createClient>;

does not seem to give a very good type. It's sort of like it treats nonexistent commands as any. Here's a Typescript playground to demonstrate the issue.

andrefedev commented 2 years ago

Using this workaround for expressing the client type:

type RedisClient = ReturnType<typeof createClient>;

does not seem to give a very good type. It's sort of like it treats nonexistent commands as any. Here's a Typescript playground to demonstrate the issue.

I was having trouble finding the type of the client and this helped me solve my problem, but it shouldn't be the way to go, rather there should be a clean type client that can be imported and used.