-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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: login without reload (#678) (#679) #914
Conversation
I'll check e2e tests now. |
@priscilawebdev will do the first CR, I'll do it when you finish. I'm still a bit stuck with #168 :) |
730d492
to
09d7896
Compare
@@ -46,7 +46,7 @@ export function generateRandomHexString(length: number = 8) { | |||
|
|||
export function signPayload(payload: JWTPayload, secret: string, options: JWTSignOptions) { | |||
return jwt.sign(payload, secret, { | |||
notBefore: '1000', // Make sure the time will not rollback :) | |||
notBefore: '1', // Make sure the time will not rollback :) |
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.
Mmm .... this affect #168 I'll need to research the collateral damage
src/webui/app.js
Outdated
return ( | ||
<Route /> | ||
<div className="page-full-height"> | ||
<Header |
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'd create a different method for each render section.
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 we move to new UI skip this part.
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 have implemented this :P
src/webui/components/Login/index.js
Outdated
event.preventDefault(); | ||
const {username, password} = this.state; | ||
this.props.onSubmit(username, password); | ||
this.setState({username: '', password: ''}); |
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.
Are you sure you want to submit before committing the state?
const {visibility, onCancel, error} = this.props; | ||
const {username, password} = this.state; | ||
return ( | ||
<div className="login-dialog"> |
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.
Too long body, the same here, methods by sections might help readability.
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.
Skip this one
src/webui/modules/detail/index.jsx
Outdated
await this.loadPackageInfo(packageName); | ||
UNSAFE_componentWillReceiveProps(newProps) { | ||
if (newProps.isUserLoggedIn !== this.props.isUserLoggedIn) { | ||
let packageName = this.getPackageName(newProps); |
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.
const
or inject this.getPackageName(newProps);
directly
@@ -49,7 +50,8 @@ export default class Detail extends React.Component { | |||
try { | |||
const resp = await API.request(`package/readme/${packageName}`, 'GET'); | |||
this.setState({ | |||
readMe: resp | |||
readMe: resp, |
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 rename resp
to readMe
you can use here destructuring
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.
@juanpicado I had the same idea.
src/webui/modules/home/index.js
Outdated
{this.state.loading ? this.renderLoading() : this.renderPackageList()} | ||
</div> | ||
); | ||
{this.state.loading ? this.renderLoading() : <PackageList help={this.state.packages.length === 0} packages={this.state.packages} />} |
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.
this.state.packages.length
lodash provides here isEmtpy
which is kinda safe
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.
you can destructure state here as well.
src/webui/modules/home/index.js
Outdated
@@ -116,8 +121,4 @@ export default class Home extends React.Component { | |||
renderLoading() { | |||
return <Loading text="Loading..." />; |
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 strings are static
each render does not create a new string Object in memory. Either constant also does the same effect
src/webui/utils/login.js
Outdated
return true; | ||
} | ||
|
||
let payload = token.split('.')[1]; |
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.
You might try here http://exploringjs.com/es6/ch_destructuring.html#_array-destructuring
src/webui/utils/login.js
Outdated
return true; | ||
} | ||
// Report as expire before (real expire time - 30s) | ||
const jsTimestamp = payload.exp * 1000 - 30000; |
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 not 100% sure what you are doing here. 🤔
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.
It's from the start, Even I could not figure out this.
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.
Oh, The Horror! 😆
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.
What unit is payload.exp
in? Milliseconds or seconds?
Could be a little clearer with:
(payload.exp - 30) * 1000; // seconds
or
payload.exp - (30 * 1000) // milliseconds
test/unit/webui/app.spec.js
Outdated
const generateTokenWithTimeRange = (limit = 0) => { | ||
const payload = { | ||
username: 'verdaccio', | ||
exp: Number.parseInt(addHours(new Date(), limit).getTime() / 1000, 10) |
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.
Number.parseInt(addHours(new Date(), limit).getTime() / 1000, 10)
is duplicated above
test/unit/webui/utils/login.spec.js
Outdated
|
||
import {isTokenExpire, makeLogin} from '../../../../src/webui/utils/login'; | ||
|
||
const generateTokenWithTimeRange = (limit = 0) => { |
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.
const generateTokenWithTimeRange = (li
is also duplicated
test/unit/webui/utils/login.spec.js
Outdated
username: 'verdaccio', | ||
exp: Number.parseInt(addHours(new Date(), limit).getTime() / 1000, 10) | ||
}; | ||
return `xxxxxx.${Base64.encode(JSON.stringify(payload))}.xxxxxx`; |
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.
xxxx.${Base64.encode(JSON.stringify(payload))}.xxx
seems to be dupe as well, perhaps some helper 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.
I'll go the second one later after @priscilawebdev and @sergiohgz do their overview. I still have to test it.
I'll implement the changes 🔢 |
|
||
constructor(props) { | ||
super(props); | ||
this.handleLogout = this.handleLogout.bind(this); |
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.
Why do not you use arrow functions?..so we can get rid of your constructor...Less code => better
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.
@priscilawebdev We have a talk about class properties, they cause some troubles in tests, check enzymejs/enzyme#944 (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.
@sergiohgz What a pity! this is sad! 😢
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 can not remove bind
of these methods from constructor as enzyme
keep giving an error for access of this
.
} | ||
|
||
componentDidMount() { | ||
this.loadLogo(); |
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.
Sorry, but why do we need to make a request for a logo? I believe the logo should not be changed...I've seen that we're loading the logo according to the URL, but this is something that should be changed in the new UI, right?
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 will change that soon. Ignore it for now.
src/webui/app.js
Outdated
const token = storage.getItem('token'); | ||
const username = storage.getItem('username'); | ||
|
||
if (isTokenExpire(token) || username === undefined) { |
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.
isn't it better to use "!!username" ?
src/webui/app.js
Outdated
if (isTokenExpire(token) || username === undefined) { | ||
this.handleLogout(); | ||
} else { | ||
this.setState({user: {username, token}}); |
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.
Why 2 setState here? I don't know what you want to achieve, but isn't it better to do as bellow?:
this.setState({
user: {username, token}
}, () => {
this.setState({isUserLoggedIn: true})
});
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.
There is no need to chain them, you can unify in one setState like:
this.setState({
user: { username, token },
isUserLoggedIn: true,
});
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.
@sergiohgz @juanpicado Yes I know, but as I didn't know what he wanted to do...for sure he has some reason..
src/webui/app.js
Outdated
storage.removeItem('username'); | ||
storage.removeItem('token'); | ||
this.setState({user: {}}); | ||
this.setState({isUserLoggedIn: false}); |
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.
Again 2 setState...Why?
src/webui/utils/login.js
Outdated
export async function makeLogin(username, password) { | ||
const payload = {}; | ||
|
||
if (isString(username) === false && isString(password) === false) { |
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 prefer to use 'Logical NOT (!)' operator here...
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.
but I found this more readable 📖 I think, @juanpicado Also uses this style.
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.
The problem with !
(NOT) is that sometimes it is hard to identify with i
. eg !isString()
That's why I am preferring === false
.
Your point is valid too 😃
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.
@ayusharma yes..I agree...it depends on your preference 👍
src/webui/utils/login.js
Outdated
type: 'error', | ||
description: 'Something went wrong.' | ||
}; | ||
return payload; |
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.
we are not using immutability 😕...+1more thing to think about the new UI
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.
@priscilawebdev @sergiohgz Could you please help me here. I am not getting the immutability thing here. May be I can initalise the payload object as
const payload = {
error: null,
token: null,
username: null
};
cc. @juanpicado
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.
Me neither.
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.
@ayusharma @juanpicado the const payload has a reference that can’t change, so it's immutable. However the value itself can indeed be modified. If the object were immutable, this wouldn't be possible.
There's a cheap and simple way to turn a mutable object/array/function into an "immutable value": Object.freeze
Object.freeze(..) provides shallow, naive immutability. You'll have to walk the entire object/array structure manually and apply Object.freeze(..) to each sub-object/array if you want a deeply immutable value.
Pragmatically, you should probably use a library that already does this well! There are a lot of nice libraries to help you with immutability. One great option is Immutable.js, which provides a variety of data structures, including List (like array) and Map (like object).
ps: some contents of my answer were taken from the book Functional-Light-JS -> Kyle Simpson
Hope it helps 👍
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.
Thanks :) I understand your concern now. It clicked me after so much time. Nice explanation.
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.
@priscilawebdev I have updated the code. Please review again.
src/webui/utils/login.js
Outdated
|
||
try { | ||
const credentials = {username, password}; | ||
const resp = await API.request(`login`, 'POST', { |
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.
you could destructure your resp: const { username, token } =
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 am avoiding destructuring here due to variable name collision.
- Args are
username
&password
- response of
API.req...
will also generateusername
&password
;
src/webui/utils/login.js
Outdated
} | ||
|
||
try { | ||
const credentials = {username, password}; |
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.
you could pass it directly on the body
@@ -26,7 +27,13 @@ const register = (url, method = 'get', options = {}) => { | |||
}); | |||
} | |||
|
|||
throw Error('Not found'); | |||
if (url === 'packages' && method.toLocaleLowerCase() === 'get') { |
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.
arrow functions 🙏
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.
hahaha... Okay.
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.
Tip: I learned this in Getify's workshop.
We should limit the use of arrow functions. It indicates lexical scoping of this and if we will use arrow functions everywhere it might give the wrong indication to the reader of the code.
I know we (@verdaccio/core-team ) are using these arrow functions everywhere (in this PR as well) but we can try to limit the use of them in the future.
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.
Some notes:
https://github.com/getify/You-Dont-Know-JS/blob/master/es6%20%26%20beyond/ch2.md#arrow-functions
So now we can conclude a more nuanced set of rules for when => is appropriate and not:
If you have a short, single-statement inline function expression, where the only statement is a return of some computed value, and that function doesn't already make a this reference inside it, and there's no self-reference (recursion, event binding/unbinding), and you don't reasonably expect the function to ever be that way, you can probably safely refactor it to be an => arrow function.
If you have an inner function expression that's relying on a var self = this hack or a .bind(this) call on it in the enclosing function to ensure proper this binding, that inner function expression can probably safely become an => arrow function.
If you have an inner function expression that's relying on something like var args = Array.prototype.slice.call(arguments) in the enclosing function to make a lexical copy of arguments, that inner function expression can probably safely become an => arrow function.
For everything else -- normal function declarations, longer multistatement function expressions, functions that need a lexical name identifier self-reference (recursion, etc.), and any other function that doesn't fit the previous characteristics -- you should probably avoid => function syntax.
https://nodejs.org/api/events.html#events_passing_arguments_and_this_to_listeners
It is possible to use ES6 Arrow Functions as listeners, however, when doing so, the this keyword will no longer reference the EventEmitter instance:
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.
@juanpicado @ayusharma interesting! Thanks for sharing...
3bc0282
to
c333184
Compare
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.
👍 tested ! works fine
@sergiohgz @priscilawebdev I need your approval here. Please verify the things before approving. |
jest.config.js
Outdated
@@ -40,7 +40,7 @@ module.exports = { | |||
'<rootDir>/test', | |||
], | |||
moduleNameMapper: { | |||
'\\.(scss)$': '<rootDir>/node_modules/identity-obj-proxy', | |||
'\\.(scss|css)$': '<rootDir>/node_modules/identity-obj-proxy', |
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.
A slightly shorter form is \\.s?css$'
src/webui/modules/home/index.js
Outdated
@@ -31,8 +31,8 @@ export default class Home extends React.Component { | |||
this.loadPackages(); | |||
} | |||
|
|||
|
|||
componentDidUpdate(prevProps, prevState) { // eslint-disable-line no-unused-vars | |||
// eslint-disable-next-line no-unused-vars |
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.
IS this necessary? Looks like both vars are used
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.
Nice catch. I'll change.
return ( | ||
<div> | ||
<Fragment> |
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.
Do we have support for shorthand fragment syntax? (<> </>
)
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.
We have support for shorthand.
src/webui/utils/login.js
Outdated
return true; | ||
} | ||
// Report as expire before (real expire time - 30s) | ||
const jsTimestamp = payload.exp * 1000 - 30000; |
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.
What unit is payload.exp
in? Milliseconds or seconds?
Could be a little clearer with:
(payload.exp - 30) * 1000; // seconds
or
payload.exp - (30 * 1000) // milliseconds
'Content-Type': HEADERS.JSON | ||
} | ||
}); | ||
const 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.
nitpick: You could return directly in a few places.
return {
username: response.username,
token: response.token,
};
return {
error: {
title: 'Unable to login',
...
}
};
return await API.request('logo');
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.
const {username, token} = response
return { username, token };
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.
Done
test/unit/webui/app.spec.js
Outdated
const { doLogin } = wrapper.instance(); | ||
await doLogin('sam', '12345'); | ||
const result = { | ||
description: 'bad username/password, access denied', |
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.
Do we have constants.js
on UI?
'bad username/password, access denied'
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.
Done
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.
👀
3672650
to
9a381e3
Compare
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.
👍
) * fix: login without reload (verdaccio#678) (verdaccio#679) * fix: implements code review suggestions (verdaccio#914) * refactor: adds scope to the app * refactor: handles null value from localstorage for username * refactor: removes text type from <Input /> * refactor: replaces isNull with isNil * refactor: improves makeLogin method * refactor: adds error from api constant * fix: updates error using API_ERROR constant in tests * refactor: updates regex for moduleMapper in jest config
) * fix: login without reload (verdaccio#678) (verdaccio#679) * fix: implements code review suggestions (verdaccio#914) * refactor: adds scope to the app * refactor: handles null value from localstorage for username * refactor: removes text type from <Input /> * refactor: replaces isNull with isNil * refactor: improves makeLogin method * refactor: adds error from api constant * fix: updates error using API_ERROR constant in tests * refactor: updates regex for moduleMapper in jest config
) * fix: login without reload (verdaccio#678) (verdaccio#679) * fix: implements code review suggestions (verdaccio#914) * refactor: adds scope to the app * refactor: handles null value from localstorage for username * refactor: removes text type from <Input /> * refactor: replaces isNull with isNil * refactor: improves makeLogin method * refactor: adds error from api constant * fix: updates error using API_ERROR constant in tests * refactor: updates regex for moduleMapper in jest config
) * fix: login without reload (verdaccio#678) (verdaccio#679) * fix: implements code review suggestions (verdaccio#914) * refactor: adds scope to the app * refactor: handles null value from localstorage for username * refactor: removes text type from <Input /> * refactor: replaces isNull with isNil * refactor: improves makeLogin method * refactor: adds error from api constant * fix: updates error using API_ERROR constant in tests * refactor: updates regex for moduleMapper in jest config
) * fix: login without reload (verdaccio#678) (verdaccio#679) * fix: implements code review suggestions (verdaccio#914) * refactor: adds scope to the app * refactor: handles null value from localstorage for username * refactor: removes text type from <Input /> * refactor: replaces isNull with isNil * refactor: improves makeLogin method * refactor: adds error from api constant * fix: updates error using API_ERROR constant in tests * refactor: updates regex for moduleMapper in jest config
) * fix: login without reload (verdaccio#678) (verdaccio#679) * fix: implements code review suggestions (verdaccio#914) * refactor: adds scope to the app * refactor: handles null value from localstorage for username * refactor: removes text type from <Input /> * refactor: replaces isNull with isNil * refactor: improves makeLogin method * refactor: adds error from api constant * fix: updates error using API_ERROR constant in tests * refactor: updates regex for moduleMapper in jest config
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
Type: Bug
The following has been addressed in the PR:
Description:
UNSAFE_
.@juanpicado @sergiohgz @priscilawebdev
Please run this branch on the local machine to ensure everything runs fine. Also, review everything carefully because it's a very big refactoring.
Thanks & Have fun 👍
Resolves #678 #679