Sitelet https://web.archive.org/web/20201028100445/https://github.com/feathersjs/feathers/pull/1611
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

feat: Add setup method for auth strategies #1611

Open
wants to merge 1 commit into
base: master
from

Conversation

@vonagam
Copy link
Contributor

@vonagam vonagam commented Oct 9, 2019

Add setup method to AuthenticationStrategy by analogy with service setup. More convenient alternative to setAuthentication/setApplication/setName and removes the need for getters, since it ensures that all services are already registered by the time of a call.

For example, right now every time you get entityService in a strategy you call:

  1. AuthenticationBaseStrategy.configuration
  2. AuthenticationBase.configuration
  3. Object.assign and app.get
  4. app.service

All of those calls can be done once in setup method instead doing them every time.

Also fixed small inconsistencies in app.setup:

  1. Moved this line up since otherwise a service registered in other service setup call will not get setup call.

  2. Moved those lines down so that setup call for a dynamically registered service happens after the service is registered just like for a usual service.

@vonagam vonagam force-pushed the vonagam:feat-auth-strategy-setup branch from d50263b to f818a04 Dec 15, 2019
@jnardone
Copy link
Contributor

@jnardone jnardone commented Feb 4, 2020

couldn't you do all of this in setApplication?

@vonagam
Copy link
Contributor Author

@vonagam vonagam commented Feb 4, 2020

it ensures that all services are already registered by the time of a call

setApplication (with setName, setAuthentication and verifyConfiguration) is called in register method, before setup, and not everything is set up at that point (not all services registered).

Why have those separate methods when one will do? In Service you do not have setApplication, setName or verifyConfiguration, only setup.

@jnardone
Copy link
Contributor

@jnardone jnardone commented Feb 4, 2020

Good points. This would be very useful for a project I'm currently working on.

@daffl daffl closed this Jul 11, 2020
@daffl
Copy link
Member

@daffl daffl commented Jul 11, 2020

Hm, sorry I'm not sure why this got closed. Going to reopen but will review again to add in v5 (which will also have asynchronous application setup).

@daffl daffl reopened this Jul 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants
You can’t perform that action at this time.