RateLimiterMongo Docs/Logic Change Needed

[cachemap]

I think a misunderstanding of the changes to mongoose in 6.* has caused a mismatch in the docs and code related to the RateLimiterMongo.

In this comment, an update was made to the docs that seems to indicate v6 mongoose's connect returns a Promise that resolves to a Connection. In actuality, it resolves to a Mongoose instance.

So, the storeClient that RateLimiterMongo receives is supposed to be a Connection, but due to the API changes mongoose made, if you follow the example in this repo's docs, you'll end up passing a Mongoose instance instead of a Connection. And, because of this, downstream code in getDriverVersion was changed erroneously.

Here's the signature from the TypeScript types:
function connect(uri: string, options?: ConnectOptions): Promise<Mongoose>;

Thus, I think the docs should read like so:

// For mongoose version <= 5
// (RETURNS A Connection)
mongoConn = mongoose.createConnection(`mongodb://127.0.0.1:27017/${dbName}`, mongoOpts);

// For mongoose version > 5
let mongoConn;
try {
  // (Resolves to A Mongoose object, NOT A Connection)
  mongooseInstance = await mongoose.connect('mongodb://127.0.0.1:27017/${dbName}');
  mongoConn = mongooseInstance.connection; // connection is a getter for the default connection and so I don't think can be destructured?
} catch (error) {
  handleError(error);
}

const opts = {
  storeClient: mongoConn,
  points: 10, // Number of points
  duration: 1, // Per second(s)
};
  
const rateLimiterMongo = new RateLimiterMongo(opts);

The Root Cause / Historical Background

In this PR: #137, it seems this detail was overlooked/misunderstood and the code was modified to implicitly account for whether RateLimiterMongo is passed a Mongoose instance or a Connection instance due to the misleading docs.

Here's what the code looks like today:

function getDriverVersion(client) {
  try {
    // NOTE: If the docs actually showed you passing the Connection instance instead of the top-level Mongoose instance, this ternary would not be necessary
    const _client = client.client ? client.client : client;

    const { version } = _client.topology.s.options.metadata.driver;
    const _v = version.split('.').map(v => parseInt(v));

    return {
      major: _v[0],
      feature: _v[1],
      patch: _v[2],
    };
  } catch (err) {
    return { major: 0, feature: 0, patch: 0 };
  }
}

This is to say, that in test/RateLimiterMongo.test.js, mongoClient and mongoClientV4 should be the same shape (or more accurately, there should only be mongoClient as there should be no distinction between them), as the only reason they appeared to have a different shape is because the understanding at the time suggested erroneously passing the resolved value of mongoose.connect instead of the Connection referred to by (await mongoose.connect(...)).connection (evidence of this being the docs and also this comment).

I think this issue arose because of a misunderstanding of types/Mongoose's API change rather than a bona-fide change to the Mongo Driver that mongoose wraps.

[cachemap]

Now, I could be wrong and instead maybe MongoDB Driver 4.0 and 4.1.3 really do have different shapes, but I find that somewhat less believable. Please advise if I truly am wrong about all this.

[animir]

@unitytotheunity Hi, thank you for the brilliant investigation.
I think, this line const _client = client.client ? client.client : client saved a lot of time for many developers, even if it is written because of a misunderstanding of how the connection could be got from mongoose.

I changed the docs to highlight, that mongooseInstance can be set as storeClient:

  mongooseInstance = await mongoose.connect(`mongodb://127.0.0.1:27017/${dbName}`);
  mongoConn = mongooseInstance.connection

  const opts = {
    storeClient: mongooseInstance || mongoConn,
    // ...
  };

Let me know if that is clear enough.

[cachemap]

@animir I appreciate your rapid response!

I like the changes you've made! I'll add that it may be worth considering adding a comment above the storeClient: ... line that mentions that both methods of initialization are supported explicitly for developer convenience.

While not necessary, I think it helps:

• clear up any confusion as to why either mongooseInstance or mongoConn is allowed
• make any developers reading the docs feel better knowing that rate-limiter-flexible tries to provide a superb developer experience (it's little details like this that bring me confidence in new dependencies I'm considering adding/relying on in my projects)

[animir]

@unitytotheunity For sure, I put a note to the list of future major release, since it would introduce breaking changes.