-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
Feature suggestion: Track registered Mongo collection instances. #13174
Feature suggestion: Track registered Mongo collection instances. #13174
Conversation
The idea behind this is to ease with package development where collections cannot always be imported explicitly. The ability to retrieve a collection by name should ease with some edge cases where package authors rely on shims for the Mongo.Collection global in order to add functionality. cultofcoders:redis-oplog is a good example of where this 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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've made similar changes in past projects and believe it's good to integrate them into the core.
Anyone has any inconvenience here?
How does this affect https://github.com/Meteor-Community-Packages/mongo-collection-instances? |
The proposed changes shouldn't have any impact on Though, seeing this package's implementation, it would be nice to have a simple |
A I'd be happy to update the PR accordingly next week and add some documentation for it if it sounds good to you. 👍 |
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. |
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 |
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 |
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. 👍 |
92f3794
@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 That being said, in current versions of Meteor, calling Should be a very quick and easy change for either implementation though. |
Oops. just came to realize I completely misunderstood your suggestion @leonardoventurini. I got fixated on using the constructor for retrieving collections. |
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 Just some food for thought. Naming is hard. :) |
@jamauro That's a very interesting point, worth considering 👍 |
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 I think |
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 directlyMongo.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. 👍