-
-
Notifications
You must be signed in to change notification settings - Fork 141
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
Token manager Implemented #222
base: master
Are you sure you want to change the base?
Conversation
Codecov Report
@@ Coverage Diff @@
## master #222 +/- ##
========================================
- Coverage 95.29% 94% -1.29%
========================================
Files 55 56 +1
Lines 1274 1334 +60
Branches 188 180 -8
========================================
+ Hits 1214 1254 +40
- Misses 55 74 +19
- Partials 5 6 +1
Continue to review full report at Codecov.
|
@@ -44,6 +32,7 @@ const defaultOptions = { | |||
|
|||
export class AccountsServer { | |||
public options: AccountsServerOptions; | |||
public tokenManager: any; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you can add real typings there would be great
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure ! Should be done now
|
||
private validateConfiguration(config: Configuration): void { | ||
if (!config) { | ||
throw new Error('[ Accounts - TokenManager ] configuration : A configuration object is needed'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you use the error module?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure ! Should be done now
Can you add an example about how do you instantiate the server class with the token manager? |
import TokenManager from '@accounts/token-manager'
const tokenManager = new TokenManager({
secret: 'secret',
emailTokenExpiration: 1000 * 60 * 60 * 2,
access: {
expiresIn: '1h'
},
refresh: {
algorithm: 'HS256'
}
})
new AccountsServer({
db,
tokenManager
}, {
password: new Password(passwordConfig)
}) |
So the user will have to install a new package manually, can't we just make the initialisation internal? |
I think we should keep the packages simple and make the user-friendly in the preset packages ? The relations between packages would become too complicated in my opinion |
bdf442b
to
3e57ec8
Compare
Here is the PR to implement the token manager with the rest of the suite
I had to open a new one because jest broke when I tried to merge with master