-
-
Notifications
You must be signed in to change notification settings - Fork 5
Optimize JWT token size by extracting user avatars to separate endpoint #36
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
Conversation
- Remove avatar from JWT token payload and create dedicated /api/v1/users/:id/avatar endpoint. Reduces token size. Backend validates and processes images.
jclausen
left a comment
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.
I'm a bit confused on how this PR is optimizing the token size since we already removed that from the custom claims in a previous pull request? We're also resizing the image twice and duplicating a bunch of code.
Some changes requested, as well as some clarification on the need for this PR.
| allowLogin : true | ||
| } | ||
| }, | ||
| defaultAvatar : "data:image/jpeg;base64,/9j/4AAQSkZJRgABAQAAAQABAAD/2wBDAAMCAgICAgMCAgIDAwMDBAYEBAQEBAgGBgUGCQgKCgkICQkKDA8MCgsOCwkJDRENDg8QEBEQCgwSExIQEw8QEBD/2wBDAQMDAwQDBAgEBAgQCwkLEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBD/wAARCADIAMgDASIAAhEBAxEB/8QAHQABAAIDAQEBAQAAAAAAAAAAAAcIBAUGAgEDCf/EAEYQAAEDAwICBAsHAgMGBwAAAAEAAgMEBQYHERIhMVFhcQgTFRYiQVWRobHRFDI1c4GS8P8" |
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.
Let's use the store to provide the default avatar, rather than hard-coding it in to two different places. ( here and above )
| fetchUsers(){ | ||
| usersAPI.list( { "sortOrder" : "lastName DESC, firstName DESC" }, this.authToken ) | ||
| .then( result => this.usersData = result.data ) | ||
| .then( result => { |
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.
I would just modify line 100 of UserService to pass includes="avatar" to the getMemento() call. Then there's no need for a separate AJAX call for each user. Same with the user API single user method.
| errors : [] | ||
| } | ||
| }, | ||
| defaultAvatar : "data:image/jpeg;base64,/9j/4AAQSkZJRgABAQAAAQABAAD/2wBDAAMCAgICAgMCAgIDAwMDBAYEBAQEBAgGBgUGCQgKCgkICQkKDA8MCgsOCwkJDRENDg8QEBEQCgwSExIQEw8QEBD/2wBDAQMDAwQDBAgEBAgQCwkLEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBAQEBD/wAARCADIAMgDASIAAhEBAxEB/8QAHQABAAIDAQEBAQAAAAAAAAAAAAcIBAUGAgEDCf/EAEYQAAEDAwICBAsHAgMGBwAAAAEAAgMEBQYHERIhMVFhcQgTFRYiQVSRobHRFDI1c4GS8P8" |
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.
Same note as above. Just make this a store property and bring it in that way.
| return this; | ||
| } | ||
|
|
||
| function processAvatar(){ |
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.
I'm a little confused on the need for this, since it's already being resized in the javascript code prior to the upload. Can you clarify?
|
Thanks for reviewing and the remarks, I agree, will create a new PR |
create dedicated /api/v1/users/:id/avatar endpoint. Reduces token size. Backend validates and processes images.