maritz / nohm

node.js object relations mapper (orm) for redis
http://maritz.github.io/nohm/
MIT License
500 stars 64 forks source link

redis and ioredis modules appear to be devDependencies (not production dependencies) #152

Closed mallove closed 4 years ago

mallove commented 4 years ago

It looks like redis and ioredis are only used for testing:

$ egrep -r 'require.*(io)?redis' nohm/
nohm//test/redisHelperTests.js:const redisHelper = require(__dirname + '/../tsOut/typed-redis-helper');
nohm//test/featureTests.js:  redis = require('redis');
nohm//test/testArgs.js:  const Redis = require('ioredis');
nohm//test/testArgs.js:  exports.redis = require('redis').createClient(
nohm//test/testArgs.js:  exports.secondaryClient = require('redis').createClient(
nohm//test/tests.js:    require('./helper.js').cleanUp(redis, args.prefix, cb);
nohm//test/pubsub/child.js:var redis = require('redis');
nohm//test/regressions.js:var redisPromise = require(__dirname + '/../tsOut/typed-redis-helper');
nohm//docs/index.md:const redisClient = require('redis').createClient();
nohm//docs/index.md:const redis = require('redis');
nohm//docs/index.md:const Redis = require('ioredis');
nohm//docs/index.md:If you require ioredis for other parts of your application and performance is important to you, the best option is probably to use ioredis for that and node\*redis for nohm. There _should_ be no conflicts when using both in the same application. Tread with caution though and test and benchmark it yourself for your use-cases.
nohm//docs/index.md:const secondClient = require('redis').createClient();
nohm//docs/index_v1.md:var redisClient = require('redis').createClient();
nohm//docs/index_v1.md:var redisClient = require('redis').createClient();
nohm//docs/index_v1.md:var secondClient = require('redis').createClient();
nohm//docs/api/index.html:const redis = require('redis').createClient();
nohm//docs/api/index.html:<p>This requires a running redis server. (you can configure host/port with the command line arguments --redis-host 1.1.1.1 --redis-port 1234)</p>
nohm//docs/api/tsOut_model.js.html:const typed_redis_helper_1 = require("./typed-redis-helper");
nohm//docs/api/tsOut_index.js.html:const redis = require("redis");
nohm//docs/api/tsOut_index.js.html:const typed_redis_helper_1 = require("./typed-redis-helper");
nohm//README.md:const redis = require('redis').createClient();
nohm//README.md:This requires a running redis server. (you can configure host/port with the command line arguments --redis-host 1.1.1.1 --redis-port 1234)
nohm//extra/stress.js:  const Redis = require('ioredis');
nohm//extra/stress.js:  redisClient = require('redis').createClient(
nohm//examples/rest-user-server/rest-server.js:const redis = require('redis');

This can lead to confusion in npm list output, e.g., it looks like redis and ioredis are needed in production, not just development:

$ npm list --only production
...
├─┬ nohm@2.2.3
│ ├─┬ debug@4.1.1
│ │ └── ms@2.1.2
│ ├── ioredis@4.15.1 deduped
│ ├── lodash@4.17.15 deduped
│ ├─┬ redis@2.8.0
│ │ ├── double-ended-queue@2.1.0-0 deduped
│ │ ├── redis-commands@1.3.5 deduped
│ │ └── redis-parser@2.6.0
│ ├── traverse@0.6.6
│ └── uuid@3.3.3 deduped

I think the below patch should resolve the issue:

$ git diff
diff --git a/package.json b/package.json
index aaccc76..3671f3f 100644
--- a/package.json
+++ b/package.json
@@ -46,9 +46,7 @@
   },
   "dependencies": {
     "debug": "^4.1.1",
-    "ioredis": "^4.9.5",
     "lodash": "^4.17.11",
-    "redis": "^2.8.0",
     "traverse": "^0.6.6",
     "uuid": "^3.3.2"
   },
maritz commented 4 years ago

You are definitely right about ioredis.

However redis is used at least once in the case of not supplying a redis client to setClient. Whether that's really useful is a different question. Maybe that functionality should be removed.

Just as an aside: Your grep is flawed since most source code now uses imports instead of require.

maritz commented 4 years ago

Oh, while trying to move it to optionalDependencies typescript correctly told me that that would be a mistake since ioredis is also used in the typed-redis helper where we use it to detect whether an exec call failed.

As far as I can tell that is the only thing where ioredis differs from node_redis from nohms perspective, but it is a vital one and thus we really need to know what is happening in there.

As such I don't see a way to remove either of those from the dependencies without changing the code and for ioredis specifically I don't even know how that changed code would look like. Any PRs that properly address this are welcome.