Completed test - #15
react-dev-senior-level wants to merge 5 commits into
Conversation
Created Login, Register Page
Added Auth, Guest Guard
There was a problem hiding this comment.
Automated Testing:
- Lint: fails
- test: fails
- coverage: fails
- build: fails
Manual Review
- Has no custom Angular animations
- Good use of services
- Has auth guards AND guest guards
- No readme describing changes
- Good use of forms (except password field with type=text)
- Lazy loading modules
- Does not validate JWT
- Unsure about async
- Use of local storage for a JWT is a security vulnerability!!!
- Pretty flat directory structure.
- Would like to see more typescript features (privacy modifiers / return types / type setting)
- Lots of importing modules that are not used - very bad for performance
- Lots of dependency installations for things that are clearly not used in the project
- Could not get it to run!!!
| "@angular/platform-browser-dynamic": "^5.2.0", | ||
| "@angular/router": "^5.2.0", | ||
| "bcryptjs": "^2.4.3", | ||
| "bluebird": "^3.5.1", |
There was a problem hiding this comment.
Not required - you're using TypeScript
| "@angular/platform-browser": "^5.2.0", | ||
| "@angular/platform-browser-dynamic": "^5.2.0", | ||
| "@angular/router": "^5.2.0", | ||
| "bcryptjs": "^2.4.3", |
There was a problem hiding this comment.
Why do you need a key stretching algorithm?
| "concurrently": "^3.5.1", | ||
| "cookie-parser": "^1.4.3", | ||
| "core-js": "^2.4.1", | ||
| "cors": "^2.8.4", |
| "@types/jasminewd2": "~2.0.2", | ||
| "@types/node": "~6.0.60", | ||
| "codelyzer": "^4.0.1", | ||
| "eslint": "^4.19.1", |
There was a problem hiding this comment.
Why not tslint? Angular has it's own linting script.
| export const environment = { | ||
| production: false | ||
| production: false, | ||
| apiUrl: 'https://dev-test-service.madebywiser.com' |
| logout() { | ||
| this.apiService.reset(); | ||
| const route = this._router; | ||
| setTimeout(function() { |
There was a problem hiding this comment.
Fat arrow function has this scoped - so you wouldn't need to declare a constant in order to access a class member.
setTimout(
() => {
this._router.navigate(['/','auth','login']);
window.location.reload();
}, 1500
);| const route = this._router; | ||
| setTimeout(function() { | ||
| route.navigate(['/auth/login']); | ||
| window.location.reload(); |
There was a problem hiding this comment.
Shouldn't require a reload if you've managed state properly
| import * as moment from 'moment'; | ||
| import { pick } from 'lodash'; | ||
| import { AuthData } from '../models/auth-data.model'; | ||
| import { environment } from '../../../environments/environment'; |
There was a problem hiding this comment.
This should have been passed into the module, and not imported directly. Read about the static forRoot and forChild methods on module definitions and the @Inject argument decorator.
| import { HttpClient, HttpHeaders } from '@angular/common/http'; | ||
| import * as moment from 'moment'; | ||
| import { pick } from 'lodash'; | ||
| import { AuthData } from '../models/auth-data.model'; |
There was a problem hiding this comment.
Relative paths are fragile within discrete modules and could've been avoided with the environment you were provided.
|
|
||
| reset() { | ||
| sessionStorage.removeItem('auth_data'); | ||
| localStorage.removeItem('auth_data'); |
There was a problem hiding this comment.
Security vulnerability. Should've just stuck to sessionStorage!!!
2 participants
No description provided.