Quest: Drop mixins

[marcoow]

ember-simple-auth has been heavily leveraging mixins form the beginning. Using mixins is what allowed ESA to be integrated easily be developers into their applications since the only thing that they'd need to do was to mix-in these mixins into some of the routes of the application and ESA would just work. However, with native JavaScript classes, mixins aren't really well supported and you have to fall back to constructs like

export default class ProtectedRoute extends Route.extend(AuthenticatedRouteMixin) {
}

which mixes native classes and Ember's old object model and thus results in unintuitive and harder-to-understand-than-necessary code. Also, eventually mixins will likely be removed from Ember altogether and they already trigger linter warnings which means installing ESA into an app will trigger new linter warnings which isn't a great and particularly reassuring experience obviously.

For those reasons, ESA should drop its mixins and adopt a new approach. There are a few key attributes we're looking for in a new solution:

• Ease of use: as explained above, mixins made ESA super easy to use for everyone and we don't want to give that up when switching to a new mechanism. Ideally, integrating ESA into an app would still only require a few steps with little complexity to understand.
• Customizability: another advantage of mixins is that everything they add to the respective classes can be overridden again and thus customized. That gives developers an easy way to modify particular aspects of ESA's behavior so it perfectly fits their needs without having to go through lots of complexity. In fact, we even extracted and designed some of the methods on the ESA mixins specifically with extensibility and customizability in mind, e.g. https://github.com/simplabs/ember-simple-auth/blob/master/addon/mixins/authenticated-route-mixin.js#L118
• Sustainability: obviously we don't want to change the main mechanism for integrating ESA into an app frequently so we're looking for something that will be stable for the foreseeable future. That means obviously we cannot use anything that is already deprecated in Ember 3.x but should also be watching for what might be coming in Ember and/or JS in the future and be sure the mixin replacement we choose now will continue to work.

Status Quo

ESA currently provides 5 mixins:

• ApplicationRouteMixin: This is an optional mixin that when used, maps certain events of the session to methods on the application route (by overriding init) that it provides default implementations of.
• AuthenticatedRouteMixin: This is mixed in to route classes that are to expect the session to be authenticated in order to be accessed. The mixin adds a custom beforeModel method that will call the original one (if present) if the session is authenticated or call triggerAuthentication otherwise that it provides a default implementation of which redirects to a configurable login route
• UnauthenticatedRouteMixin: This is the opposite of the AuthenticatedRouteMixin and works in the same way by adding a default implementation of beforeModel that checks whether the session is not authenticated and redirects otherwise (without a dedicated method for that)
• OAuth2ImplicitGrantCallbackRouteMixin is used in combination with the OAuth2ImplicitGrantAuthenticator and triggers authentication of the session via that authenticator when the route is visited (by overriding activate)
• DataAdapterMixin is used with Ember Data adapters and overrides the handleResponse method so that it automatically invalidates the session on 401 error responses

Options

There are several options that might be worth looking into more deeply:

• Decorators: This seems to be the obvious choice for replacing mixins. Instead of the above example, one would simply write

import { authenticated } from 'ember-simple-auth/decorators/routes';

@authenticated
export default class ProtectedRoute extends Route {
}

The advantage of decorators is they would still allow integrating ESA super easily but the disadvantage is the uncertainty around the decorators proposal and the fact that we might not be able to do everything we need to do with the final version of the proposal

• Custom mixin mechanism: Using a mechanism for mixins that does not rely on the Ember Object model but works with native JavaScript classes would be another option that minimises the changes in terms of how ESA is integrated into an application. The above example could look something like this with an approach like that:

import { Protected } from 'ember-simple-auth/mixins/routes';

export default class ProtectedRoute extends Protected(Route) {
}

More information on the approach can be found here. The disadvantage of this is that it is fairly exotic and while technically ESA could still be adopted in a way that is very similar to the current mixins-based mechanism, it would result in substantially increased complexity as it'd be unclear to many devs what they are doing exactly and how the mechanism works.

• Providing individual methods: Instead of providing ESA's functionality via mixins it would be possible of course to export the individual methods instead so that those would be called manually be the dev in the appropriate places instead of automatically via the mixins. The above example would look something like this with the approach:

import { requireAuthentication } from 'ember-simple-auth/helpers/routes';

export default class ProtectedRoute extends Route {
  async beforeModel() {
    await requireAuthentication();
    …
  }
}

While this has the advantage of being fairly simple and straight-forward, the disadvantage is that it requires the dev to follow more steps when adapting ESA in an application which leads to more complexity simply as the amount of glue code that integrates ESA and the application is higer.

Prior Art

@mike-north did some work some time ago to split the ESA codebase apart into exports of individual methods that could be imported and called directly as a replacement for mixins. Unfortunately nobody ever found the time to implement it and the effort didn't lead anywhere…

[Turbo87]

it would result in substantially increased complexity as it'd be unclear to many devs what they are doing exactly and how the mechanism works.

I think understanding "how the mechanism works" is significantly more complicated for decorators than it is for the "mixin pattern", which is essentially just a factory function that produces a subclass of the passed in class 😅

[marcoow]

Hm, maybe – I think the difference is decorators are (or will be) something that people know and is familiar to them while methods like the above might not be.

[Turbo87]

function mixinFoo(ParentClass) {
  return class extends ParentClass {
    foo() { /* ... */ }
  };
}

I honestly don't understand what's so hard to understand about such a factory function 🤔

[LevelbossMike]

I personally feel like the class decorator approach would be easier to integrate than importing functions and then creating your own base-classes. At least in the first steps of a project.

It feels significantly more "magical" than the individual methods- and "custom-mixin"-approach though and I don't see the benefit over a mixin conceptionally as it's basically just a mixin applied by a decorator, right?

Additional questions regarding this approach: Can people override methods mixed in that way? Can class-decorators be combined?

The "custom mixin"-approach feels like a good middle way as it's pretty easy to understand what's happening, very explicit and doesn't come with any integration headaches for people that just want to get started and don't want to dive deeper into the things that ESA is doing internally.

[marcoow]

Additional questions regarding this approach: Can people override methods mixed in that way? Can class-decorators be combined?

Yes, both are possible.

[jgwhite]

I think option 3 could be valuable both in the short term as a bridging solution (giving the team more time to work on the design of a higher-level API) and in the long term as a set of primitives for those who need a more granular API.

The uncertainty around decorators is a risk, for sure, but I have to say I love how it reads! There’s something pleasingly intention-revealing about the decorator form.

[zeppelin]

I agree with @jgwhite: option 3 is great to have, because it can co-exist with other options focused on the class itself. To me, it's basically deciding between 1 and 2.

[chriskrycho]

A couple thoughts:

1. Decorators are pretty, but they also basically reintroduce not only the convenience but also the problems of mixins/multiple inheritance, in that behavior is far away from the implementation and can be stomped accidentally by other decorators in ways that are very difficult to identify. This doesn’t mean we should never reach for them, but it does mean that the tradeoffs for them are heavily biased toward writing code over reading and especially over maintaining code.
2. In part because of the state of flux around the decorator spec, insofar as types are useful to consumers of the app, decorators are a non-starter, whereas both options 2 and 3 are perfectly amenable to typing not only usefully but indeed correctly.
3. Related, understanding how decorators modify a class is rather more involved than either of the other methods. A “manual mixin” is a little strange, but lots of folks have experience with factory methods and many fewer have any idea what’s going on with descriptors. When someone inevitably hits a bug or wants to extend behavior or just wants to learn how things work under the hood, option 3 is easily the most discoverable and option 2 still worlds ahead of decorators.

My very strong recommendation is to build option 3 and see what people's real world use of that primitive plays out as. It’s perfectly safe to then add option 2 as a convenience on top of it, itself implemented exactly in terms of those primitives. This is the ideal toward which Ember itself is working in most major API designs these days, and it’s a good one: if a user needs the control afforded by the lower-level primitive, for whatever reason including because they’re experimenting with new router ideas or similar, they can have it. And you may find that the little bit of “boilerplate” is in practice not that bad after all… or that it is and you ship option 2! But starting with the primitives gives you flexibility later and forward motion now.

---

My feelings on decorators in general

[Samsinite]

Why not support both a factory function, and class decorators? Under the hood they are basically the same.

[chriskrycho]

I thought about suggesting that! At this point I think that decision can be deferred, but also there are at least two good reasons not to:

• impact on bundle size: if both were bundled it would fairly substantially increase the cost of including ESA. Hopefully Embroider will begin to make that a non-issue for apps later this year but for now it has to be a consideration.
• the goal of shared solutions: as a community, we tend to value converting on a Standards way of solving any given problem. Shipping both of those would substantially muddy the waters here. Which one is *recommended *? Is there a time when one is better than the other? How do I pick between them for my app? For this spot in my app?

And the other upside if shipping primitives first is that it lets users experiment with life without either as well as with solutions built on top of them without committing to that sugar as a public API.

[Turbo87]

FWIW we already ship the primitives and the mixins are mostly just the glue code that connects those primitives to the built-in constructs in Ember.

to be honest, I have doubts that extracting mid-level primitives between the already-shipped low-level primitives and the high-level mixins will actually result in any significant advantages.

[marcoow]

I understand everyone's reasons for preferring option 3. My only worry is that we would/might not be able to keep integrating ESA into an app as simple as it is now with that approach. It's just a difference in complexity and the amount/depth of concepts you have to understand when you have to invoke a particular method in a particular place etc. The simple integration mechanism has been the main focus of ESA from the beginning (and probably is the only thing about it that is still simple now). Another option of course could be to export the primitives and also export base classes that invoke the primitives in all the right places. I think the overhead in bundle size would be neglect-able.

However, some of the above mixins could likely be replaced with other approaches altogether. For example, what the AuthenticationRouteMixin does could be moved into an instance initializer or so, removing the need for that mixin altogether (you could then opt out of that via a config setting maybe). For the AuthenticatedRouteMixin/AuthenticatedRouteMixin an alternative approach could be adding something to the router DSL (torii has had that for some time).

[marcoow]

I took a look at what I think could be a good approach here. The mixins do not necessarily translate to function exports 1:1 and we can get rid of some concepts completely.

ApplicationRouteMixin

Since the only thing this mixin does is map events on the session service to methods on the application route it could be dropped completely and be moved to an instance initializer, e.g.:

import sessionAuthenticated from 'ember-simple-auth/routing';

export default {
  name: 'ember-simple-auth',

  initialize(app) {
    let session = app.lookup('service:session');
    let applicationRoute = app.lookup('route:application');
    session.on('authenticationSucceeded', (...args) => applicationRoute.sessionAuthenticated ? applicationRoute.sessionAuthenticated(...args) : sessionAuthenticated(...args));
    // …other events
  }
};

If the mixin is deprecated of course that means
you can no longer call e.g. super.sessionAuthenticated() in the application route when overriding one of the methods. For that reason it might make sense to also
export the individual methods so extending the default implementation is still possible without having to copy the code, e.g.:

import { sessionAuthenticated } from 'ember-simple-auth/routing';

export default class ApplicationRoute extends Route {
  sessionAuthenticated() {
    console.log('session was authenticated!');
    sessionAuthenticated();
  }
}

AuthenticatedRouteMixin

The AuthenticatedRouteMixin adds its own implementation of beforeModel that check whether the session is authenticated and redirects to a configurable login route otherwise. If it intercepts a transition and in fact redirects, it also stores the target route of the intercepted transition in the session which is then read in the ApplicationRouteMixin's sessionAuthenticated method and retried so that when a user requests /protected, they are redirected to e.g. /login to authenticate and taken to /protected once that was successful. That creates a kind of implicit connection between the two mixins of course – this doesn't lead to any immediate consequences but should be kept in mind when designing the new API (and also is something that is not solved in a particularly great way by the current API).

A good approach for getting rid of the AuthenticatedRouteMixin would likely be exporting the functionality that ensures the session is authenticated as a separate method for the developer to invoke in the route they want it in – as several people recommend as their favorite option above:

import { requireAuthentication } from 'ember-simple-auth/routing';

export default class ProtectedRoute extends Route {
  async beforeModel() {
    let isAuthenticated = await requireAuthentication();
    if (isAuthenticated) {
      // do/load other things here
    }
  }
}

Showing how this could be made reusable so the above doesn't have to be repeated for every protected route in an application by e.g. extracting the logic into a custom base class route could probably be covered in the README.

The other thing that the AuthenticatedRouteMixin does is it defines a triggerAuthentication method (with a default implementation) that is invoked
whenever a protected route is accessed without the session being authenticated and the session needs to be authenticated. By default that method will
simply redirect to the authenticatedRoute. This method is useful to have so it can be overridden if the application is using a different authentication
mechanism (e.g. one that relies on a modal which doesn't have its own route or in an Electron or Cordova app where you'd maybe want to authenticate the user via something that is implemented natively outside of the Ember.js app). However, if we export the method for ensuring authentication for invocation by the developer directly, there is no clear alternative for the triggerAuthentication method unless we'd build some sort of callback mechanism, e.g.:

import { requireAuthentication, triggerAuthentication } from 'ember-simple-auth/routing';

export default class ProtectedRoute extends Route {
  async beforeModel() {
    let isAuthenticated = await requireAuthentication(() => {
      console.log('triggering authentication');
      triggerAuthentication(); // alternatively one could implement their own logic of course
    });
    if (isAuthenticated) {
      // do/load other things here
    }
  }
}

I don't think an API like that would be particularly great though – maybe it makes sense to introduce a new authenticationRequired event or so on the session service (similar to the existing events) that could then be mapped to a method on the application route via the initializer that replaces the ApplicationRouteMixin. That would streamline the API a bit since all of the methods that handle similar things (handling successful login, handling successful logout, triggering login) are handled in a similar way then via events and all of those events end up being handled on the application route. Like for the other methods on the application route, I'd still export the default implementations so those could be reused when implementing the methods on the application with the intention to only add to the default behavior:

import { triggerAuthentication } from 'ember-simple-auth/routing';

export default class ApplicationRoute extends Route {
  triggerAuthentication() {
    console.log('triggering authentication!');
    triggerAuthentication();
  }
}

Of course we'd lose the ability to easily trigger authentication in different ways on different routes which you can do with the current implementation of the AuthenticatedRouteMixin by overriding the method differently in different routes (although I doubt anyone relies on that and it'll still be possible to reintroduce something similar with this new approach).

UnauthenticatedRouteMixin

The UnauthenticatedRouteMixin could be replaced by essentially the same mechanism explained above for the AuthenticatedRouteMixin since both are doing essentially the same things just in opposite ways:

import { preventAuthentication } from 'ember-simple-auth/routing';

export default class OpenRoute extends Route {
  async beforeModel() {
    let isUnauthenticated = await preventAuthentication();
    if (isUnauthenticated) {
      // do/load other things here
    }
  }
}

In this case though an event on the session might not make sense to trigger the redirect as we don't really want to invalidate the session and thus change its state, we just want the route not to be accessible if the session is authenticated (which is useful for e.g. the login route – you'll want to redirect them elsewhere when they access that and the session is not authenticated already but you do not necessarily want to log them out). Probably passing in the route to redirect the user to in case the session is authenticated is better in this case (I could also not think of anything different anyone might like to do in this case and it has also never been possible to do anything but redirecting with ESA's current mixin mechanism):

import { preventAuthentication } from 'ember-simple-auth/routing';

export default class OpenRoute extends Route {
  async beforeModel() {
    let isUnauthenticated = await preventAuthentication('index');
    if (isUnauthenticated) {
      // do/load other things here
    }
  }
}

OAuth2ImplicitGrantCallbackRouteMixin

The OAuth2ImplicitGrantCallbackRouteMixin should be easily replaced by a single function and some documentation on where to invoke it as well:

import { handleOauth2ImplicitGrantCallback } from 'ember-simple-auth/routing';

export default class CallbackRoute extends Route {
  activate() {
    handleOauth2ImplicitGrantCallback(location);
  }
}

Since this something that's only used in one route in the entire application and does not apply for all kinds of routes, I'd be less worried about the cognitive overhead having to deal with the function directly introduces.

DataAdapterMixin

Regarding the DataAdapterMixin I'm not sure yet what a good approach would be. We currently both support adding headers via ajaxOptions as well as headersForRequest and I think ajaxOptions could probably go away but of course that depends on what versions of ember data we want to support etc. and that would require some additional research.

Overall solution

With the solution explained here we get rid of all of the mixins (DataAdapterMixin is tbd as explained above) and are left with 2 main methods only which are requireAuthentication which replaces the AuthenticatedRouteMixin and preventAuthentication which replaces the UnauthenticatedRouteMixin. The ApplicationRouteMixin is removed without a direct replacement and we're moving its logic into an instance initializer instead (which could be opt-in or opt-out even – tbd). We still keep the flexibility of customizing ESA's default handling of e.g. session invalidation etc. requireAuthentication and preventAuthentication could still be accompanied by authenticated and unauthenticated decorators respectively later on if we choose to or factory methods or whatever – however I'm not sure that's even necessary. I think my key point here is that if we reduce ESA's API (at least the part of if that most people would have to deal with most of the time) to only 2 methods, that does already simplify it quite a bit in which case the additional complexity of having to deal with the bare functions directly that I mentioned above as one of my worries, matters less.

[Turbo87]

it could be dropped completely and be moved to an instance initializer

using initializers has developed into an anti-pattern. they make it significantly harder to mock things in tests for example and they can make rendering tests slower than necessary.

[marcoow]

Hm, what’s a good alternative for an adding to add some code to an app that auto-connects parts of the app in the way described above? Or is there no alternative?

[chriskrycho]

My take at this point is that auto-connecting things like that is not worth the trivial convenience it adds. It is convenient, but taking a very short time to wire up something in your application route just isn't that big of a deal and does make everything both easier to test and easier to understand for someone new to the code.

[BryanCrotaz]

Hm, what’s a good alternative for an adding to add some code to an app that auto-connects parts of the app in the way described above?

In this case, setting it up in the Route constructor?

[marcoow]

Hm, I understand these points. The thing is we should not force people to copy/paste this into their application route:

import { triggerAuthentication, sessionAuthenticated, sessionInvalidation,  } from 'ember-simple-auth/routing';

export default class ApplicationRoute extends Route {
  @service session;

  constructor() {
    this.session.on('authenticationSucceeded', () => this.sessionAuthenticated());
    this.session.on('invalidationSucceeded', () => this.sessionInvalidated());
    this.session.on('requireAuthentication', () => this.triggerAuthentication());
  }

  sessionAuthenticated() {
    sessionAuthenticated();
  }

  sessionInvalidated() {
    sessionInvalidated();
  }

  triggerAuthentication() {
    triggerAuthentication();
  }
}

While this is arguably more explicit and less magical, it'd substantially increase the cognitive overhead when integrating ESA into a codebase.

An alternative to this could be just providing one function to be called in the constructor that would effectively set up the above (without actually adding the methods to the class of course):

import { setupAutoSessionRouting  } from 'ember-simple-auth/routing';

export default class ApplicationRoute extends Route {
  constructor() {
    setupAutoSessionRouting();
  }
}

[Turbo87]

how do you handle cleanup of these events then?

[marcoow]

I assume willDestroy?

[chriskrycho]

we should not force people to copy/paste this into their application route… While this is arguably more explicit and less magical, it'd substantially increase the cognitive overhead when integrating ESA into a codebase.

This is the fundamental point of disagreement about the design here. I think one-time extra cognitive load during setup more than pays for itself during the long lives of applications; more than that, I actually think it's helpful in teaching people what they addon is actually doing when they set it up.

I won't belabor the point further though, as I think I've made my perspective clear and I don't want to be that guy who's just arguing. 😉

[marcoow]

I actually think it's helpful in teaching people what they addon is actually doing when they set it up.

Indeed we disagree here :) I think my short response is that if you're supposed to understand everything you're using you cannot actually do anything because there's no way you can ever live up to that expectation – I strongly believe that hiding details is one of ESA's strong points that many people appreciate and that only enabled its relatively wide adoption. Also: is @tracked bad because it doesn't teach you how tracking works in Ember/Glimmer? I think sometimes hiding details is the whole point or a relevant point at least when designing an API.

That said though, there's a difference between hiding details and magically adding or changing stuff in an app. With the mixin approach things were actually not so magical since we relied on a (more or less) well-understood mechanism in Ember. The fact that

export default Route.extend(ApplicationRouteMixin);

adds methods to the route isn't surprising as that's what mixins do. I agree that

export default class ApplicationRoute extends Route {
  constructor() {
    setupAutoSessionRouting();
  }
}

adding kind of optional methods to the application route is not necessarily straight forward. So maybe there's a better solution. Maybe we could have something that would essentially do this:

import { triggerAuthentication, sessionAuthenticated, sessionInvalidation,  } from 'ember-simple-auth/routing';

session.on('authenticationSucceeded', () => sessionAuthenticated());
session.on('invalidationSucceeded', () => sessionInvalidated());
session.on('requireAuthentication', () => triggerAuthentication());

This would not add any of the event handlers to the application route or invoke methods on the application route if present. It would simply apply ESA's default handling of these events. If we don't want to use an initializer we could have something to add to the router or something:

import EmberRouter from '@ember/routing/router';
import ESARouter from 'ember-simple-auth/routing';

const Router = EmberRouter.extend);

Router.map(function() {
  …
});

export default ESARouter(Router);

If anyone wants to customize any of the default behavior, the way to do that would be the above and you'd have to copy/paste code into the application route and set up the event handlers yourself.

In general, my main goal would be to find an API that hides all of the details from anyone who isn't interested while at the same time not leaning heavily on things being added/changed magically but providing some level of customizability.

I won't belabor the point further though, as I think I've made my perspective clear and I don't want to be that guy who's just arguing. 😉

Don't worry – the input is highly appreciated!

[jelhan]

@marcoow Could you elaborate why we would need the route hooks registered at all of they just contain default implementation? Consumer could still implement a custom function if needed. But until then directly binding event to default event handler should be fine. It reduces complexity and lines of code needed for integration. It would be still obvious how to replace or extend a default implementation.

[marcoow]

Yes, that's basically what I'm suggesting here. We'd want an easy way to switch on the default implementation which could be this (for example):

import EmberRouter from '@ember/routing/router';
import ESARouter from 'ember-simple-auth/routing';

const Router = EmberRouter.extend);

Router.map(function() {
  …
});

export default ESARouter(Router);

An alternative would be an instance initializer but if that has downsides it's likely not a great alternative in particular as we're looking for a sustainable solution.

We'd also want a straight forward way to extend or customize the default behaviour which you'd usually want at the routing layer as it involves redirecting. That could be this:

import { triggerAuthentication, sessionAuthenticated, sessionInvalidation,  } from 'ember-simple-auth/routing';

export default class ApplicationRoute extends Route {
  @service session;

  constructor() {
    this.session.on('authenticationSucceeded', () => this.sessionAuthenticated());
    this.session.on('invalidationSucceeded', () => this.sessionInvalidated());
    this.session.on('requireAuthentication', () => this.triggerAuthentication());
  }

  sessionAuthenticated() {
    // custom behavior goes here potentially plus invoking the default behavior:
    sessionAuthenticated();
  }

  sessionInvalidated() {
    // custom behavior goes here potentially plus invoking the default behavior:
    sessionInvalidated();
  }

  triggerAuthentication() {
    // custom behavior goes here potentially plus invoking the default behavior:
    triggerAuthentication();
  }
}

The reason why I suggest exporting the default behaviour here as well is so you do not need to copy that code (which might change over time of course so that you'd have to adapt the copied code) but can just implement custom logic in addition to also invoking the default behaviour.

[marcoow]

Also to be clear: opting-in to the default behaviour via the router is arbitrary really – it could be using any mechanism that allows us to get a hold of an owner.

[jelhan]

A special router, custom mixin, factory function and decorator is too much magic in my opinion. I think it's fine to ask the user to register three event handlers in application route (and remove them on destroy). It's not much more lines of code but directly teaches the building blocks and provides an obvious way how to customize. It would be still simple without being magic.

[marcoow]

A special router, custom mixin, factory function and decorator is too much magic in my opinion

To be clear – nobody is suggesting a special router nor all of the above together. Decorators are mostly off the table for now as well. The easiest way would be to use an instance initializer to hook up the default functionality but as that doesn't seem to be a sustainable solution we'll have to look for alternatives.

[jelhan]

we'll have to look for alternatives.

The alternative I'm arguing for would be like this:

import { triggerAuthentication, sessionAuthenticated, sessionInvalidation,  } from 'ember-simple-auth/routing';

export default class ApplicationRoute extends Route {
  @service session;

  constructor() {
    this.session.on('authenticationSucceeded', () => sessionAuthenticated);
    this.session.on('invalidationSucceeded', () => sessionInvalidated);
  }

  willDestory() {
    this.session.off('authenticationSucceeded', () => sessionAuthenticated);
    this.session.off('invalidationSucceeded', () => sessionInvalidated);
  }
}

I don't see any need for an additional layer of high-level API as this is already very simple integration.

I see this similar for other functionalities for ember-simple-auth. E.g. relying on the primitives exposed by session service might also be enough for authentication and unauthenticated route functionalities:

export default class ApplicationRoute extends Route {
  @service session;

  async beforeModel() {
    try {
      await this.session.requireAuthentication();
    catch (error) {
      // user is not authenticated
      return this.transitionTo('login');
    }
  }
}

[marcoow]

I don't see any need for an additional layer of high-level API as this is already very simple integration.

It is relatively simple but I'd rather look for a way that is even simpler since the flexibility this provides is not needed by a large part of apps that just want to stick with the default implementation. Thus there is no real benefit forcing everyone to write this glue code themselves. Manually subscribing/unsubscribing to events should certainly be possible for everyone who wants it but I strongly believe there is value in not forcing a bunch glue code onto every user of the addon.

[pichfl]

I still think that a bunch of factory functions that do the modifications for you would be the easiest path forward. The factories must only use public API and basically represent the guide on how to apply the smaller building blocks on their own by applying them automatically.

If you are happy with the defaults, use protected(Route), if not use the primitives.

It would be nice to have primitives for binding/removing the event listeners and so on.

[chriskrycho]

Let me clarify a point here:

if you're supposed to understand everything you're using you cannot actually do anything because there's no way you can ever live up to that expectation

This isn't what I meant, and I don't think it's a reasonable characterization of my position, but I understand how what I wrote read this way. What I'm actually saying above is that the "boilerplate" here just teaches you what you're changing. It doesn't teach you all the mechanics or require you to understand them; it does teach you that you're modifying the behavior of various routes. If you later want (or need!) to understand all the mechanics, you have an obvious thread to tug on.

Folks may or may not find that persuasive—our disagreement appears to be deep-seated and intractable, and that just is what it is; taste does vary! I just wanted to clarify what I was and was not saying above. 😅

[marcoow]

This isn't what I meant, and I don't think it's a reasonable characterization of my position

Sorry, didn't mean to mis-characterize your position. The input is greatly appreciated and I do understand where you're coming from and what you're saying. I just think ease of adoption is one of ESA's core strengths and we should all aim for a solution that

• requires few changes/additions in the app
• forces as little of the internals as possible on devs
• …while not relying on any automagic mechanisms that modify the app in unexpected ways etc.
• …and while still providing escape hatches if you want to have more control/customize things

Right now integrating ESA requires mixing in 2 or at most 3 mixins in some routes which I think checks all the above boxes – although comes with the downside of mixins of course.

[marcoow]

I think the best option would be to expose all functionality via the session service. I put together a quick example here. The idea is to add a few new methods into the service that replace the existing mixins more or less 1:1:

• AuthenticatedRouteMixin becomes:

import Route from '@ember/routing/route';
import { inject as service } from '@ember/service';

export default Route.extend({
  session: service(),

  beforeModel(transition) {
    this.session.requireAuthentication(transition, 'login');
  },

  model() {
    return this.get('store').findAll('post');
  }
});

• UnauthenticatedRouteMixin becomes:

import Route from '@ember/routing/route';
import { inject as service } from '@ember/service';

export default Route.extend({
  session: service(),

  beforeModel(transition) {
    this.session.prohibitAuthentication(transition, 'index');
  },
});

• ApplicationRouteMixin becomes:

import { inject as service } from '@ember/service';
import Route from '@ember/routing/route';

export default Route.extend({
  session: service(),
  sessionAccount: service('session-account'),

  init() {
    this._super(...arguments);
    this.session.on('authenticationSucceeded', () => this.sessionAuthenticated());
    this.session.on('invalidationSucceeded', () => this.session.handleInvalidation());
    this.session.on('authenticationRequested', () => this.session.triggerAuthentication('login'));
  },

  beforeModel() {
    return this._loadCurrentUser();
  },

  sessionAuthenticated() {
    this.session.handleAuthentication('index');
    this._loadCurrentUser();
  },

  _loadCurrentUser() {
    return this.get('sessionAccount').loadCurrentUser().catch(() => this.get('session').invalidate());
  }
});

In this example, the app adds custom logic to the handler for the authenticationSucceeded event. To make things easier for the standard integration path of ESA, we could provide a convenience method handleSessionEvents or so which would be a shortcut for setting up the session service's standard event handlers for its own events like this:

this.session.on('authenticationSucceeded', () => this.session.handleAuthentication('index'));
this.session.on('invalidationSucceeded', () => this.session.handleInvalidation('index'));
this.session.on('authenticationRequested', () => this.session.triggerAuthentication('login'));

[jelhan]

I like that proposal. Maybe even the ApplicationRouteMixin could be replaced by methods on the session service? ESA would not be configured anymore on application route but by providing a custom session service, which extends from the base service?

// app/services/session.js
import BaseService from 'ember-simple-auth/services/session';
import { inject as service } from '@ember/service';

export default class SessionService extends BaseService {
  @service sessionAccount;

  sessionAuthenticated() {
    // call super to have default implementation
    super.sessionAuthenticated();

    // do some additional custom stuff
    this.get('sessionAccount').loadCurrentUser().catch(() => this.get('session').invalidate());
  }
}

Extending a service is not a very common way in ember to customize an addon but it would be similar to how adapters and serializers are customized. Similar to mixins it has the risk of name clashes but this could be reduced if the base service only exposes it's public API on that service and hides it's internal state and methods.

[marcoow]

Yes, extending the service would work as well. In the above example, attaching the event handlers in the application route is kind of arbitrary and could be done elsewhere as well.

Note: All of this being wired up in the routes is mostly for historical reasons since there was no router service in earlier versions of Ember and you needed to have something on the route be called when the session's state changed so you could transition accordingly and the routes were really the only place where that was possible.

In fact, the events on the session service could be dropped altogether since they are no longer needed when extending the session service (and could easily be added in again in an extension of the session service) – see f03483a (I don't know why the tests are failing 😞 )

[marcoow]

There's a WIP PR now: #2198