Feature suggestion: Track registered Mongo collection instances.

[JorgenVatle]

The ability to retrieve a collection by name should ease with some edge cases where package authors rely on shims for the Mongo.Collection module in order to add functionality. As far as I'm aware, there aren't really any good ways of retrieving collections by name, you need to import them explicitly, or patch directly Mongo.Collection to add this functionality.

cultofcoders:redis-oplog is a good example of where that doesn't always work out well. Collections registered by compiler/build plugin packages appear to be registered before standard feature packages. This leads to the oplog package having no real way to reliably access those collection instances.

Just opening this as a draft PR so we can have a quick discussion whether this would be a worthwhile addition. 👍

[CLAassistant]

All committers have signed the CLA.

[harryadel]

How does this affect https://github.com/Meteor-Community-Packages/mongo-collection-instances?

[JorgenVatle]

The proposed changes shouldn't have any impact on mongo-collection-instances. The collection map is added under Mongo.Collections not Mongo.Collection.

Though, seeing this package's implementation, it would be nice to have a simple .get() method on Mongo.Collection, then hiding away the Collections map with an underscore or something to indicate it's something that shouldn't be accessed directly. In this case, mongo-collection-instances should still function correctly, it would just override the .get() method provided by Meteor. The collection map would still work and be accessible to other packages as it is tracked in the Mongo.Collection constructor.

[JorgenVatle]

A .get() method directly on Mongo.Collection would probably a nicer way to access registered collections instead of exposing a plain Map instance.

I'd be happy to update the PR accordingly next week and add some documentation for it if it sounds good to you. 👍

[harryadel]

Thanks for the explanation Jorgen, I'm no part of the core team but if @nachocodoner & @leonardoventurini, maybe even @radekmie since he has done many mongo performance improvements then I think it'd be a good idea to act on. Thank you for your efforts.

[radekmie]

I think it's fine to add such a global registry, but I definitely would not make it a public API. It can be one of those "documented internals" instead, where Meteor would try to but wouldn't have to keep backward compatibility.

Also, it's now technically possible to get all collection names through drivers but not their instances, so I'd call it something like Mongo._collectionInstances.

[leonardoventurini]

This might come in handy to prevent circular imports. I've used mongoose in the past quite a bit for raw Node.js projects, and it allows us to get the model by simply calling model('name'), this helps when we reference other collections inside hooks, instead of importing them there directly, which as the collections are sometimes exported all at once, could cause a chicken and egg dilemma. With that, I believe having this ability in Meteor is a great add, but I would vouch for making this a public API, perhaps making it more ergonomic even like collection('name').

[JorgenVatle]

Haven't had the chance to follow up the proposal just yet. Got some time on Monday and/or Tuesday and will update the PR from there. 👍

[JorgenVatle]

@leonardoventurini I've opted not to make any changes to the collection constructor so far in this PR as I worry that it might involve some changes to current functionality for the Collection constructor.

That being said, in current versions of Meteor, calling Mongo.Collection() without the new keyword does cause a somewhat cryptic exception during replication setup. So it might not be much of a risk to begin with. There's probably even some room to add a failsafe to warn users when you forget to new keyword.

Should be a very quick and easy change for either implementation though.

[JorgenVatle]

Oops. just came to realize I completely misunderstood your suggestion @leonardoventurini. I got fixated on using the constructor for retrieving collections.

[jamauro]

Thanks for putting this together. It'll be a really nice addition. One thought on the most recent changes: do you think people might get confused by Mongo.Collection vs Mongo.collection? It's also not implicitly creating a collection like the Node.js Mongo driver db.collection which could lead to some confusion. I wonder if it's worth changing it to avoid that, e.g. Mongo.getCollection which would align with Mongo docs (but apparently not the Node driver). Alternatively, maybe the name should remain Mongo.collection and it should implicitly create the collection if one doesn't exist to align with the Node Mongo driver. That might be kind of nice as an alternative to new Mongo.Collection and it could remain backwards compatible.

Just some food for thought. Naming is hard. :)

[harryadel]

@jamauro That's a very interesting point, worth considering :+1:

[JorgenVatle]

Very fair point! I agree, there is certainly a concern that there might be some confusion between the two. I love the idea of implicitly creating the collection. Though, I believe it could lead to some edge cases.

Specifically, if you are defining your collections using new Mongo.Collection() but you have a call to Mongo.collection() that gets registered before the constructor, the constructor will end up throwing an exception since two collections with the same name is not allowed. Collection options could possibly also be a concern if we were to refactor to handle that.

I think getCollection would be the best middle ground here. It clearly indicates behavior and it's close to the mongosh API.