Skip to content

Migration Guide and Breaking Changes is not full enough #1765

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

Open
2 of 6 tasks
npdev453 opened this issue Dec 6, 2021 · 14 comments
Open
2 of 6 tasks

Migration Guide and Breaking Changes is not full enough #1765

npdev453 opened this issue Dec 6, 2021 · 14 comments

Comments

@npdev453
Copy link

npdev453 commented Dec 6, 2021

  • 1) Many methods now is not available by old names (with legacyMode also too)

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;
    ...
  • 2) hSet does not allow objects as second parameter anymore
 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)
}
  • 3) Connection errors did not crash process with uncatched errors anymore. Strange thing, but seems like something changed in error policy of this module. I haven't figured it out yet, but when connection is lost, algorithm simply stop on redis commandSending forever.

  • 4) (Bonus) Types for legacyMode and callbacks is not provided, but in 3.x its does, and for 2.x too: https://www.npmjs.com/package/@types/redis,
    Dirty way for now is patch node_modules/@node-redis/client/dist/lib/client/index.d.ts and add it to WithCommands type
export declare type RedisClientCommandSignatureLegacy<C extends RedisCommand> = (...args: [...options: Parameters<C['transformArguments']>, callback: (e: Error | null, result: RedisCommandReply<C>) => void]) => void

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


  • 5) Importing of { RedisClient } from "redis" now is not available anymore.
    Current solution is craft it by yourself: (but it seems like a crutch)
import { createClient } from 'redis'
type RedisClient = ReturnType<typeof createClient>

But I think develops can provide more useful type with generics, but not delete it at all.
Many things, like cache adapters, models or abstract classes require that type. Also, will be useful, if develop may declare that
code expected pure RedisClient or RedisClient with RediJson module.

Anyway it should be described in migration guide.

  • 6) 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>}. (thx for reporting @jimsnab)


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
Copy link
Contributor

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
Copy link

wavded commented Dec 7, 2021

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?

wavded added a commit to tj/connect-redis that referenced this issue Dec 7, 2021
V4 is too breaking at the moment to work well with the other supported
clients and the `legacyMode` is incomplete and unreliable as a fallback.

Ref: redis/node-redis#1765
@ebebbington
Copy link

ebebbington commented Dec 7, 2021

Also doesn't mention:

  • What happened to import { RedisClient }

@npdev453
Copy link
Author

npdev453 commented Dec 7, 2021

Also doesn't mention:

  • What happened to import { RedisClient }

Thanks, forgot about it. Already added to the list

@jimsnab
Copy link

jimsnab commented Dec 7, 2021

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
Copy link

jimsnab commented Dec 7, 2021

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
Copy link
Author

npdev453 commented Dec 7, 2021

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
Copy link

jimsnab commented Dec 7, 2021

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

This has to be completely reworked.

leibale added a commit that referenced this issue Dec 13, 2021
* update workflows & README

* add .deepsource.toml

* fix client.quit, add error events on cluster, fix some "deepsource.io" warnings

* Release 4.0.0-rc.1

* add cluster.duplicate, add some tests

* fix #1650 - add support for Buffer in some commands, add GET_BUFFER command

* fix GET and GET_BUFFER return type

* update FAQ

* Update invalid code example in README.md (#1654)

* Update invalid code example in README.md

* Update README.md

Co-authored-by: Leibale Eidelman <leibale1998@gmail.com>

* fix #1652

* ref #1653 - better types

* better types

* fix 5412479

* Update GEOSEARCHSTORE.spec.ts

* fix #1660 - add support for client.HSET('key', 'field', 'value')

* upgrade dependencies, update README

* fix #1659 - add support for db-number in client options url

* fix README, remove unused import, downgrade typedoc & typedoc-plugin-markdown

* update client-configurations.md

* fix README

* add CLUSTER_SLOTS, add some tests

* fix "createClient with url" test with redis 5

* remove unused imports

* Release 4.0.0-rc.2

* add missing semicolon

* replace empty "transformReply" functions with typescript "declare"

* fix EVAL & EVALSHA, add some tests, npm update

* fix #1665 - add ZRANGEBYLEX, ZRANGEBYSCORE, ZRANGEBYSCORE_WITHSCORES

* new issue templates

* add all COMMAND commands

* run COMMAND & COMMAND INFO tests only on redis >6

* Create SECURITY.md

* fix #1671 - add support for all client configurations in cluster

* ref #1671 - add support for defaults

* remove some commands from cluster, npm update, clean code,

* lock benny version

* fix #1674 - remove `isolationPoolOptions` when creating isolated connection

* increase test coverage

* update .npmignore

* Release 4.0.0-rc.3

* fix README

* remove whitespace from LICENSE

* use "export { x as y }" instead of import & const

* move from "NodeRedis" to "Redis"

* fix #1676

* update comments

* Auth before select database (#1679)

* Auth before select database

* fix #1681

Co-authored-by: leibale <leibale1998@gmail.com>

* Adds connect-as-acl-user example. (#1684)

* Adds connect-as-acl-user example.

* Adds blank line at end.

* Set to private.

* Adds examples folder to npmignore.

* Adds Apple .DS_Store file to .gitignore (#1685)

* Adds Apple .DS_Store file.

* Add .DS_Store to .npmignore too

Co-authored-by: Leibale Eidelman <leibale1998@gmail.com>

* move examples

* clean some tests

* clean code

* Adds examples table of contents and contribution guidelines. (#1686)

* Updated examples to use named functions. (#1687)

* Updated examples to user named functions.

* Update README.md

Co-authored-by: Leibale Eidelman <leibale1998@gmail.com>

* update docs, add 6.0.x to the tests matrix, add eslint, npm update, fix some commands, fix some types

Co-authored-by: Simon Prickett <simon@crudworks.org>

* fix tests with redis 6.0.x

* fix ACL GETUSER test

* fix client.quit and client.disconnect

* fix ACL GETUSER

* Adds TypeScript note and corrects a typo.

* Fixes a bug in the Scan Iterator section. (#1694)

* Made examples use local version.

* Add `lua-multi-incr.js` example (#1692)

Also fix syntax error in the lua example in the README

Closes #1689.

* Add(examples): Create an example for blPop & lPush (#1696)

* Add(examples): Create an example for blPop & lPush

Signed-off-by: Aditya Rastogi <adit.rastogi2014@gmail.com>

* Update(examples): fix case, add timeout, update readme

Signed-off-by: Aditya Rastogi <adit.rastogi2014@gmail.com>

Closes #1693.

* Add command-with-modifiers.js example (#1695)

* Adds TypeScript note and corrects a typo.

* Adds command-with-modifiers example. (#1688)

* Adds command-with-modifiers example. (#1688)

* Adds command-with-modifiers example. (#1688)

* Removed callbacks.

Co-authored-by: Simon Prickett <simon@redislabs.com>

Closes #1688.

* Issue # 1697 FIX - creates an example script that shows how to use the SSCAN iterator (#1699)

* #1697 fix for set scan example

* adds the js file

* adds comment

* Minor layout and comment adjustment.

Co-authored-by: srawat2 <shashank19aug>
Co-authored-by: Simon Prickett <simon@redislabs.com>

Closes #1697.

* fix #1706 - HSET return type should be number

* use dockers for tests, fix some bugs

* increase dockers timeout to 30s

* release drafter (#1683)

* release drafter

* fixing contributors

* use dockers for tests, use npm workspaces, add rejson & redisearch modules, fix some bugs

* fix #1712 - fix LINDEX return type

* uncomment TIME tests

* use codecov

* fix tests.yml

* uncomment "should handle live resharding" test

* fix #1714 - update README(s)

* add package-lock.json

* update CONTRIBUTING.md

* update examples

* uncomment some tests

* fix test-utils

* move "all-in-one" to root folder

* fix tests workflow

* fix bug in cluster slots, enhance live resharding test

* fix live resharding test

* fix #1707 - handle number arguments in legacy mode

* Add rejectedUnauthorized and other TLS options (#1708)

* Update socket.ts

* fix #1716 - decode username and password from url

* fix some Z (sorted list) commands, increase commands test coverage

* remove empty lines

* fix 'Scenario' typo (#1720)

* update readmes, add createCluster to the `redis` package

* add .release-it.json files, update some md files

* run tests on pull requests too

* Support esModuleInterop set to false. (#1717)

* Support esModuleInterop set to false.

When testing the upcoming 4.x release, we got a bunch of typescript
errors emitted from this project.

We quickly realized this is because the library uses the esModuleInterop
flag. This makes some imports _slightly_ easier to write, but it comes
at a cost: it forces any application or library using this library to
*also* have esModuleInterop on.

The `esModuleInterop` flag is a bit of a holdover from an earlier time,
and I would not recommend using it in libraries. The main issue is that
if it's set to true, you are forcing any users of the library to also
have `esModuleInterop`, where if you keep have it set to `false` (the
default), you leave the decision to the user.

This change should have no rammifications to users with
`esModuleInterop` on, but it will enable support for those that have it
off.

This is especially good for library authors such as myself, because I
would also like to keep this flag off to not force *my* users into this
feature.

* All tests now pass!

* Move @types/redis-parser into client sub-package

and removed a comma

* npm update, remove html from readme

* add tests and licence badges

* update changelog.md

* update .npmignore and .release-it.json

* update .release-it.json

* Release client@1.0.0-rc.0

* revert d32f1ed

* fix .npmignore

* replace @redis with @node-redis

* Release client@1.0.0-rc.0

* update json & search version

* Release json@1.0.0-rc.0

* Release search@1.0.0-rc.0

* update dependencies

* Release redis@4.0.0-rc.4

* fix #1724 - fix LINDEX signature

* add positive test for LINDEX

* fix #1718 - add support for buffers in pubsub

* Fixed a few typos.

* fix ARRPOP

* fix #1726

* enhance cluster reshard handling

* Adds RediSearch demo.

* Adds intro sentence.

* Made top level comment more descriptive.

* Adds RedisJSON example.

* Renamed JSON search example.

* Some refactoring.

* Fixed search example for JSON.

* Minor wording updates.

* Added missing pet name.

* Adds JSON package overview.

* Fixed typo.

* Search package README initial version.

* remove echo from docker entrypoint.sh

* npm update

* update docs

* fix merge

* fix merge

* Release client@1.0.0

* npm update

* Release search@1.0.0

* update sub modules

* Release redis@4.0.0

* update README

* Fixed `zScanIterator` example (#1733)

The example for `zScanIterator` had the wrong values in the `for` loop.

* fix #1738 - add support for buffers in HSET

* fix for c96a0dd - remove .only from HSET tests

* increase HSET test coverage

* fix for d0de622

* ref #1741 - fix socket options type

* fix #1741 - change default to `localhost` and update docs

* fix #1739 - add support for number as value in HSET

* fix tests workflow

* increase pushGeoCountArgument test coverage

* init time series

* Update v3-to-v4.md (#1737)

* Update v3-to-v4.md

* Update v3-to-v4.md

* Update v3-to-v4.md

* Update docs/v3-to-v4.md

Co-authored-by: Simon Prickett <simon@crudworks.org>

* Update docs/v3-to-v4.md

Co-authored-by: Simon Prickett <simon@crudworks.org>

* Update docs/v3-to-v4.md

Co-authored-by: Simon Prickett <simon@crudworks.org>

* Update docs/v3-to-v4.md

Co-authored-by: Simon Prickett <simon@crudworks.org>

* Update docs/v3-to-v4.md

Co-authored-by: Simon Prickett <simon@crudworks.org>

* Update docs/v3-to-v4.md

Co-authored-by: Simon Prickett <simon@crudworks.org>

Co-authored-by: Leibale Eidelman <leibale1998@gmail.com>
Co-authored-by: Simon Prickett <simon@crudworks.org>

* fix #1744 - add npm minimum version

* fix #1743 - restrict the list of published files (#1742)

* fix TS.CREATERULE

* fix #1673 - Export RedisClientType and RedisClusterType types (#1704)

* add description to package.json

* fix #1749 - FT.SEARCH SORTBY

* Client: Export errors (#1750)

* Client: Export errors

* export RedisModules & RedisScripts as well

Co-authored-by: Leibale Eidelman <leibale1998@gmail.com>

* fix for 592714f - "fix tests"

* fix for a453114 - fix FT.SEARCH

* fix FT.SEARCH

* improve encodeCommand performance, add set-get-delete-string benchmark

* fix encodeCommand

* Add the `event` doc (#1753)

* hdr-histogram-js

* fix #1764 - fix PubSub resubscribe

* fix #1758 - implement some CLIENT commands, add `name` to `RedisClientOptions`

* update tests badge link

* update license badge link

* ref #1765 - support lowercase command names in legacy mode

* update benchmarks

* fix LINSERT test title

* fix v4 benchmarks

* fix discord link

* fix #1771 - fix PubSub resubscribe

* fix #1766 - allow .quit() in PubSub mode

* fix for e112568 - should not retry connecting if failed due to wrong auth

* Add LGTM (#1773)

* Update README.md

* Update README.md

Co-authored-by: Leibale Eidelman <leibale1998@gmail.com>

* fix #1734 - fix PubSub unsubscribe race condition

* Support RedisTimeSeries (#1757)

* Implement missing commands and add test

* Update DECRBY.spec.ts

* Small changes

* clean code

* Update MGET_WITHLABELS.ts

Use map in transformReply

Co-authored-by: leibale <leibale1998@gmail.com>

* fix #1774 #1767 - update docs

Co-authored-by: Chayim <chayim@users.noreply.github.com>

* Search commands (#1778)

* ft.alter

* ft.profile

* clean code

* fix generated documentation

* fix documentation workflow

* fix documentation workflow

* fix documentation workflow

* update CHANGELOG

* Release client@1.0.1

* update tsconfig.json

* Release json@1.0.1

* Release search@1.0.1

* fix time-series .release-it.json

* add @node-redis/time-series to README

* add time-series to all in one

* Release redis@4.0.1

Co-authored-by: Richard Samuelsson <noobtoothfairy@gmail.com>
Co-authored-by: mustard <mhqnwt@gmail.com>
Co-authored-by: Simon Prickett <simon@redislabs.com>
Co-authored-by: Simon Prickett <simon@crudworks.org>
Co-authored-by: Suze Shardlow <SuzeShardlow@users.noreply.github.com>
Co-authored-by: Joshua T <buildingsomethingfun@gmail.com>
Co-authored-by: Aditya Rastogi <adit.rastogi2014@gmail.com>
Co-authored-by: Rohan Kumar <rohan.kr20@gmail.com>
Co-authored-by: Kalki <shashank.kviit@gmail.com>
Co-authored-by: Chayim <chayim@users.noreply.github.com>
Co-authored-by: Da-Jin Chu <dajinchu@gmail.com>
Co-authored-by: Henrique Corrêa <75134774+HeCorr@users.noreply.github.com>
Co-authored-by: Evert Pot <me@evertpot.com>
Co-authored-by: AnnAngela <naganjue@vip.qq.com>
Co-authored-by: Damien Arrachequesne <damien.arrachequesne@gmail.com>
Co-authored-by: kjlis <kjlis@users.noreply.github.com>
Co-authored-by: Vojta Staněk <stanekv01@gmail.com>
Co-authored-by: Guy Korland <gkorland@gmail.com>
Co-authored-by: Avital Fine <79420960+AvitalFineRedis@users.noreply.github.com>
@jayeclark
Copy link

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
Copy link
Contributor

leibale commented Dec 13, 2021

@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
Copy link

@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
Copy link

OxleyS commented Jan 11, 2022

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
Copy link

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.

poppinlp pushed a commit to poppinlp/node_redis-vs-ioredis that referenced this issue Feb 2, 2022
jellis24 added a commit to jellis24/connect-redis that referenced this issue Apr 6, 2022
V4 is too breaking at the moment to work well with the other supported
clients and the `legacyMode` is incomplete and unreliable as a fallback.

Ref: redis/node-redis#1765
@bschelling
Copy link

Switched to v4, now all the values must be strings. Type boolean throws an error...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

10 participants