-
-
Notifications
You must be signed in to change notification settings - Fork 795
fix: minor stuff #1562
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
fix: minor stuff #1562
Conversation
13753b9 to
79d2e7b
Compare
|
Sweet, all makes sense, thank you! I was actually wondering why all other dependencies always get updated but it's basically when a dependent package has a new version and |
|
Ah... i get it now. It is because of this file. Because almost everything depends on |
|
In my opinion it should rather be peerDependencies to favor the version installed by the developer and avoid duplicates. |
|
Hi, your 2nd change breaks my implementation. I login to my application using Username + Password (LocalStrategy). And there's the point. In getPayload, I check whether I have to recreate the token and delete the old one from authResult if so. Which causes in turn that a new token with new Payload gets created. With the current (your) Implementation, I have no clue how I could achieve the same, since getPayload will never again get called for JWTStrategy or any derived. |
|
I think it is valid to change this back if you can add a test that illustrates your use case. |
|
You can also extend default class MyAuthenticationService extends AuthenticationService {
async create (data, params) {
const authResult = await super.create(data, params);
// do your checks of authResult here
return authResult;
}
}Or even hooks may work (and be more feathers-way) since |
|
Thanks guys for your quick response and thank you @vonagam for your hint about overriding the So I want step back a little and thank @daffl for your support about moving back. @vonagam, look at it in a way of consistency. Until now, for every And from this perspective I also would appreciate to change this back to the consistent behavior. |
|
Why are you against overriding |
|
because that's exactly what I do - I want to modify/change the Payload. |
|
One decides whenever to use old token or create new one in For me |
|
Yes, you are right! - It Works! Thanks! |
Minor stuff:
Moved
@feathers/*dependencies fromdevDependenciestodependenciesif their types are present in a package build.Moved this line up, since no need to call
getPayloadandgetTokenOptionsif their result is irrelevant.Here used
locationas is, without prepending slash, as it will be stripped anyway.In
@feathersjs/configurationallow calling without an argument and useapp!.instead of(app as Application)..No need to
service &&here since line before will throw ifserviceis undefined, also in typesserviceis non-nullable.Note:
@feathersjs/commonsseems to be stuck at4.3.0while everyone else is already at4.3.2(it skipped4.3.1too).