Phase 1 - extract model layer api - for review - DO NOT MERGE

[Sing-Li]

For review purpose - DO NOT MERGE

[martijnwalraven]

Hi Sing Li,

We're meeting later today, but I wanted to give some feedback before so you may have time to think about it.

My main comment is that we need to better differentiate between code running on the client and on the server, and this will have repercussions for the current design.

Meteor blurs the distinction between client and server because we have minimongo on the client, meaning we get to use a similar data access API on both. But we won't have this convenience with alternative accounts providers, as these will only run on the server.

What I think is missing from the current design is a consideration of how server-side user data actually gets to the client. The only realistic option right now it seems is to continue to use DDP/minimongo for this. Apollo/GraphQL will likely become a a viable alternative in the future, but we're not there yet and we also have backwards compatibility to worry about. So that suggests some changes to the design:

• I would suggest renaming ModelUser to the more descriptive AccountsProvider, and make sure to only use (subclasses of) this on the server.
• AccountsCommon should not depend on either a DDP connection or AccountsProvider and should not set this.users. Methods like user() should be made abstract and implemented separately on the client and server.
• The AccountsClient should accept a DDP connection, and _initConnection should be moved from AccountsCommon.
• this.users should be initialized to the users collection as usual on the client. Client-side code should be able to continue to use the minimongo API to access the users collection. Collection mutations should still be supported when the MongoDB accounts provider is in use.
• The AccountsServer should accept an AccountsProvider instance.
• Server-side code should publish at least the current user to the users collection, using the acounts provider to actually load the data. We probably also want a way to publish other users to the client, so we may need a new API for that.
• We shouldn't set this.users to the accounts provider, because current code depends on the fact that it is a Mongo collection. I think we should use a separate this.accountsProvider for this. When using a MongoDB accounts provider, I think we should still allow people access to the underlying collection (this.users) for backwards compatibility. For other accounts providers, we preferably would throw a descriptive error.

Does this make sense to you?

[Sing-Li]

Absolutely. Looking forward to the meeting today.

[martijnwalraven]

I think we have a meeting right now :)

[martijnwalraven]

A few small comments about the design of the provider API:

• I think findSingle is a bit ambiguous, so I would change it to findById (which is also consistent with findByUsername).
• findCurrentUser, findSingleLoggedIn seem a little misleading, because they still expect a userId. Maybe make findById a little more configurable and allow passing in field names?
• In general, I think it is best to write things out, so insertHashedLoginToken instead of insertHLoginToken for instance.
• Some functions probably can be removed or cleaned up (like findUsersInServices, getMongoUsersForReactiveWork)

[CLAassistant]

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}