Skip to content
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

initial pass at user mixins approach #1578

Closed
wants to merge 1 commit into from
Closed

initial pass at user mixins approach #1578

wants to merge 1 commit into from

Conversation

clarkbw
Copy link

@clarkbw clarkbw commented Aug 8, 2015

I wanted to make this pull request not as something I think is ready for production (it doesn't pass all the tests yet) but as a way to present an alternate approach for the User model in loopback. Also recognizing that a change in approach like this would likely require a major version bump to something like loopback 3.0 so there's time to work it out.

monolithic user model

Here's my argument against the current User model approach in loopback. It's monolithic and therefore full of assumptions that are hard to work with. There are awkward email templates for verify and password reset as well as realms and usernames which are useful but may be superfluous in many applications.

Most hackers can get what they want done given the way JavaScript works and with the new mixins model there are plenty of tools available but I think we can create a simpler approach from the ground up.

user mixins

This is an alternate approach using mixins to define the User model. It starts with a simpler User model defined in JSON that is then built up via JavaScript mixins. The benefits of this approach is that there are less moving parts for the base User model, the loopback story becomes one of a User model that relies on AccessTokens for authentication but the rest is left up to developers (username, password, realm).

Here's where my mixin approach went right and wrong:

The mixins are built into the loopback repo and I think they should actually be separate modules loaded via npm. The User in my code defaults to a mixin that replicates the current system however I think a simpler default module might be a username / password module or maybe an email / password module. I also imagine realm support is desired for enterprise customers so I'll leave that up to you guys as a perhaps as separate module or mixin that offers realm support.

Because the mixins are all still in the loopback repo the user tests are still massive. Separating out the concerns of the user methods could mean that other modules are responsible for the user testing. Obviously that shifts the responsibility around but also simplifies the base system. Loopback tests could focus solely on Users and AccessTokens and avoid sending emails etc.

With this new mixin system I'm more easily able to create an alternate login scheme that doesn't use passwords but only email addresses and accessTokens. For my application I want people to be able to log in with an email address only so I need to disable all the password requirements from the User model. It is not impossible, only somewhat difficult to rip out the password requirements that loopback imposes on Users.

In a future where I can have a separate mixin that handles my "email only" authentication I can also provide rich email templates within the module. I don't think is a good idea for loopback to pull email templates into the repo (#1245) given that some devs might not be using email at all.

tests

Two tests are still failing, but I'm ignoring that for now to see if this approach is worth a discussion and further exploration.

  • I have a double presence check for Email is required [ 'presence', 'presence', 'format.null' ]
  • Also User ctor exports default Email model is returning Email instead of email.

@slnode
Copy link

slnode commented Aug 8, 2015

Can one of the admins verify this patch? To accept patch and trigger a build add comment ".ok\W+to\W+test."

@raymondfeng
Copy link
Member

@clarkbw This is fantastic. Thank you for putting the ideas into concrete code! In general, it aligns well with my vision on how models should be built in LoopBack. Here is what I have in mind.

  1. Mixins are fundamental building blocks. Each mixin consists of a list of properties, methods and other constructs such as relations. Some of them are defined at the class level while others at instance (prototype) level. Mixin is a combined form of interfaces and abstract classes.
  2. Mixins can have two different flavors:
    • Definition of properties/methods/... in JSON/JS format (static)
    • A factory class that applies properties/methods/... to existing models (dynamic).
  3. Mixins are composable via inheritance or mixin. In some cases, we can build facade mixins that delegate to other mixins. For example, DAO can delegate to connector implementations.
  4. Models are assemblies of mixins. We should also allow relations to reference mixins as targets.
  5. Connectors are providers/implementations of mixins. Each connector can implement one or more mixins. Most of the DB connectors will implement the CRUD mixin. Please note even CRUD can be decoupled into multiple mixins, such as Key/Value and Queryable. Other capabilities such Discovery and Migration are mixins too. Each connector is contracted as a list of mixins.
  6. Attaching a model to a datasource/connector will instruct LoopBack to inject the mixins from the underlying connector to the model.
  7. Built-in models are composed from a set of mixins. Some of the core functions will only depend on the mixins instead of the whole model. It will give us more flexibility to choose and pick.

@clarkbw
Copy link
Author

clarkbw commented Aug 8, 2015

@raymondfeng you spelled it out so clearly! Your vision is exactly what I was thinking of when looking at how to build Models.

@ritch
Copy link
Member

ritch commented Aug 10, 2015

monolithic user model

Tottally agree. And as the author of the User model you would probably expect me to argue for what we have now. On the contrary I've run into this pain quite a few times myself and have realized the mistakes made in the design of the user model.

If we move User properties into Mixins can we cannot introspect them in our tooling... perhaps we can make Mixins properties declarative (JSON files) so this is not an issue? Or perhaps this is an advanced enough feature that it does not need to be supported by tooling.

I think I'd like to try to build this new User refactor outside of the core. This will allow us to release it incrementally and opt into moving into core. This might make it a bit more difficult to maintain backwards compatibility, but this also gives us the choice to even offer backwards compatibility.

Two tests are still failing.

I have a double presence check for Email is required `[ 'presence',
'presence', 'format.null' ]`

Also User ctor exports default Email model is returning Email instead
of email.
@pulkitsinghal
Copy link

I think this is beautiful, y'all rock out loud!

@gausie
Copy link
Contributor

gausie commented Dec 22, 2015

Is this planned for v3?

@clarkbw
Copy link
Author

clarkbw commented Mar 16, 2016

Closing this, I wanted to show what was possible but I don't think either myself or the loopback team is working on this anymore. Feel free to reopen this if I'm wrong here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants