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

Deprecate mixins #2198

Merged
merged 20 commits into from
Jun 4, 2020
Merged

Deprecate mixins #2198

merged 20 commits into from
Jun 4, 2020

Conversation

marcoow
Copy link
Member

@marcoow marcoow commented May 8, 2020

This PR deprecates the mixins and introduces a new API that is exposed completely via the session service.

We discussed a few options for replacing the current mixins with a new mechanism in #2185 (thanks to everyone who chimed in!) but focussed mostly on concepts that would replace the mixins directly. Since we have dropped support for older versions of Ember though and now have new capabilities that we can leverage that were not available when ESA's current mixin mechanism was first introduced, I realized we could build a completely new API.

Historically, transitioning between routes was only really possible from route classes (or controllers) since there was no routing service. For that reason, we had to establish a connection between the session and its change state on one side and the routes on the other side. With the routing service that is no longer necessary though as we can transition between routes from the session directly using the service. That means we can just drop the ApplicationRouteMixin that implemented the above described connection between the routing layer and the session and handle all of that in the session itself. We still maintain customizability of course since any app can simply extend the session service and override the respective methods, similar to how you would do the same thing in the application route currently.

The AuthenticatedRouteMixin's and UnauthenticatedRouteMixin's main responsibilities were adding custom logic into the route classes' beforeModel methods that would check for the session to be authenticated or not and transition elsewhere etc. A mixin is a nice way to share that code and automatically put it in the right place. While exposing that API via the session service now will require a bit more code, it still doesn't put significant additional cognitive load on the developer and it's easy to extract the code into a base class if one wanted to do that:

export default ProtectedRoute extends Route {
  @service session;

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

The opposite of this would be this.session.prohibitAuthentication('index') for disallowing a route to be visited when the session is authenticated already.

The OAuth2ImplicitGrantCallbackRouteMixin and DataAdapterMixin mixins are deprecated without 1:1 replacements. They provide very little value anyway and would usually only be used once per application. Thus, adding guidance in the docs for achieving the respective behavior should be sufficient.

TODO

  • update README and guides
  • deprecate DataAdapterMixin
  • deprecate OAuth2ImplicitGrantCallbackRouteMixin
  • update the API docs template to show the deprecation status of mixins

closes #2185

@marcoow marcoow mentioned this pull request May 8, 2020
@marcoow marcoow force-pushed the drop-mixins branch 4 times, most recently from 4de50dd to b6704d8 Compare May 9, 2020 18:12
@marcoow marcoow marked this pull request as ready for review May 9, 2020 18:29
Copy link

@jelhan jelhan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot for putting all that work into Ember Simple Auth! To small comments on the API design. Hope it's helpful.

@marcoow marcoow requested a review from a team May 11, 2020 10:08
@marcoow marcoow force-pushed the drop-mixins branch 3 times, most recently from 0e836d3 to 7b7a0b4 Compare May 11, 2020 10:42
Copy link

@patsy-issa patsy-issa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor clarification, loving the new api <3

Procfile.server Outdated Show resolved Hide resolved
Copy link

@patsy-issa patsy-issa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One minor fix needed for the readme and this looks good to go.

README.md Show resolved Hide resolved
@patsy-issa
Copy link

hmm any ideas on why the specs are failing?

@marcoow
Copy link
Member Author

marcoow commented May 27, 2020

@patsy-issa I think we're running into this: ember-cli/babel-plugin-ember-modules-api-polyfill#64 (it's also broken on master)

@rwjblue
Copy link
Contributor

rwjblue commented May 29, 2020

@marcoow - ember-cli-babel@7.20.1 was just released, and should fix the underlying issue referenced in ember-cli/babel-plugin-ember-modules-api-polyfill#64.

https://github.com/babel/ember-cli-babel/releases/tag/v7.20.1

@marcoow
Copy link
Member Author

marcoow commented May 29, 2020

Thanks @rwjblue ❤️

@marcoow
Copy link
Member Author

marcoow commented May 29, 2020

Unfortunately we're running into ember-cli/babel-plugin-ember-modules-api-polyfill#110 now…

@marcoow marcoow mentioned this pull request May 29, 2020
@marcoow
Copy link
Member Author

marcoow commented Jun 3, 2020

@patsy-issa CI is green now

Copy link

@patsy-issa patsy-issa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@marcoow marcoow merged commit 8ab24c3 into master Jun 4, 2020
@delete-merged-branch delete-merged-branch bot deleted the drop-mixins branch June 4, 2020 07:50
@pjcarly
Copy link

pjcarly commented Jun 5, 2020

Great to see this merged. Thanks for the amazing work!

Any idea when we can expect a release?

@sdebarros
Copy link
Contributor

@pjcarly we released a beta version including this today. 🎉

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

Successfully merging this pull request may close these issues.

Quest: Drop mixins
8 participants