-
Notifications
You must be signed in to change notification settings - Fork 29
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
feat: login and logout commands #118
Conversation
@@ -14,15 +13,19 @@ export const loginWithToken = async (token: string, region: RegionCode) => { | |||
}) | |||
} | |||
catch (error) { | |||
if ((error as FetchError).response?.status === 401) { | |||
throw new Error(`The token provided ${chalk.bold(maskToken(token))} is invalid: ${chalk.bold(`401 ${(error as FetchError).data.error}`)} | |||
if (error instanceof FetchError) { |
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 is this not handled by handleAPIError
as the others?
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.
@edodusi because it has a custom message for 401
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't you do it directly in handleAPIError
by using a conditional with login_with_token
? This way it should be easier to maintain/extend
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 prefer not to make handleApiError
flexible, the scope of that function is to handle the error with general error messages.
What if I want to pass a custom message for 422 instead?
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 think the responsibility for printing messages should be of handleAPIError
, and the message to pick can depend on different factors: the http status, the action, the error, even the region potentially.
I would apply the single-responsibility principle, these actions should only handle the actual action to perform, not care about how errors are handled, that is the responsibility of another function or class.
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 @edodusi I don't think I follow.
The responsibility of handleAPIError
is to throw an instance of APIError
with the correspondent error message depending on the response status, it's not a factory.
export function handleAPIError(action: keyof typeof API_ACTIONS, error: Error): void {
if (error instanceof FetchError) {
const status = error.response?.status
switch (status) {
case 401:
throw new APIError('unauthorized', action, error)
case 422:
throw new APIError('invalid_credentials', action, error)
default:
throw new APIError('network_error', action, error)
}
}
else if (error.message.includes('NetworkError') || error.message.includes('Failed to fetch')) {
throw new APIError('network_error', action, error)
}
throw new APIError('generic', action, error)
}
Then this is caught by handleError
at top level, whose responsibility is to check what type of error it is to be able to call konsola with the right prefix in case the flag verbose is included.
export function handleError(error: Error, verbose = false): void {
// If verbose flag is true and the error has getInfo method
if (verbose && typeof (error as any).getInfo === 'function') {
const errorDetails = (error as any).getInfo()
if (error instanceof CommandError) {
konsola.error(`Command Error: ${error.getInfo().message}`, errorDetails)
}
else if (error instanceof APIError) {
konsola.error(`API Error: ${error.getInfo().cause}`, errorDetails)
}
else if (error instanceof FileSystemError) {
konsola.error(`File System Error: ${error.getInfo().cause}`, errorDetails)
}
else {
konsola.error(`Unexpected Error: ${error.message}`, errorDetails)
}
}
else {
// Print the message stack if it exists
if ((error as any).messageStack) {
const messageStack = (error as any).messageStack
messageStack.forEach((message: string, index: number) => {
konsola.error(message, null, {
header: index === 0,
margin: false,
})
})
konsola.br()
konsola.info('For more information about the error, run the command with the `--verbose` flag')
}
else {
konsola.error(error.message, null, {
header: true,
})
}
}
if (!process.env.VITEST) {
console.log('') // Add a line break for readability
process.exit(1) // Exit process if not in a test environment
}
}
Imagine you want to add an icon to each and every error messages (absurd idea), in this scenario you should be able to do this just by changing a single file, instead in this way you are forced to change at least two
Wouldn't be enough to change the error message format once on the konsola
file?
error: (message: string, info?: unknown, options?: KonsolaFormatOptions) => {
if (options?.header) {
const errorHeader = chalk.bgRed.bold.white(` Error `)
console.error(formatHeader(errorHeader))
console.log('') // Add a line break
}
console.error(`${chalk.red.bold('▲ error')} ${message}`, info || '')
if (options?.margin) {
console.error('') // Add a line break
}
},
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.
@edodusi think of it like this:
handleAPIError
throws anAPIError
depending on the HTTP response codehandleFilesystemError
throws anFileSystemError
depending on the filesystem operation codehandleError
top-level that catches all errors thrown in the CLI and sends them to thekonsola
konsola
prompt formatting and 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.
@alvarosabu if this last comment was true I would expect APIError
to be raised exclusively by handleAPIError
, and instead you throw it also in the action handler, that's what I'm saying.
I used the factory as an analogy as it's the class responsible for instantiating object, in this case it's responsible for throwing errors.
But the point is, and this is only because the code is well architected that this pops out to me, there is an exception to this pattern, that is that an action is throwing its custom errors instead of delegating this responsibility to handleAPIError
.
If the code was just made of catch blocks that throw errors with custom messages and log something and do their things I would not have argued, but since you have this design pattern and I think it's good and extensible and easy to maintain, then (to me) the logical consequence is to handle everything about errors in handleAPIError
.
You say this is an exception because it has to print its own custom message, ok, then if tomorrow we add another action we have to think where to put the custom message, instead a good design means we don't have to think, we simply follow, extend, etc...
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.
Hi @edodusi thanks for the explanation.
If the code were just made of catch blocks that throw errors with custom messages and log something and do their things I would not have argued, but since you have this design pattern. I think it's good and extensible and easy to maintain, then (to me) the logical consequence is to handle everything about errors in handleAPIError.
I think I now get completely your point thanks fro clarifying. I would rather treat the handleAPIError
as a utility rather than a required step in the flow of error handling, it's a DRY abstraction that solves 1 problem Apply generic status messages depending on the response status
Sometimes you need to use the utility, and sometimes you don't, really depends on the need, in this case, the need is to take control and send a custom message directly.
I personally think that passing the custom messages through the handleAPIError
would be cumbersome and will add complexity we don't need to that utility.
My next question would be, do you consider this a blocker for the merge? Or it's something we can discuss and evaluate 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.
I see your point. To me this is NOT a blocker, probably I just have a slightly different view of the architecture of this code but we can discuss, and probably with future improvements we will better align.
Thanks for the great discussion, I love to see deep thoughts on software design ❤️
You can resolve this conversation, and soon as we need to extend this behavior we will think of the best way.
Hi, this is the new implementation of the login/logout commands for the renovated version of the CLI. A lot of stuff is included in this PR, so bear with me I'm going to try to make the testing as easy as possible since there are different mechanisms to authenticated involved.
How to test this PR
pnpm install
pnpm run build:stub
<-- Important you test it with stub for nowOnce build without issues
pnpm run dev <command>
that you want to test, expnpm run dev login --region us
Unit test
There are 9 test files with 43 tests in total including mocks for filesystem, and with a coverage of around > 80% (There are some modules like the
session
module that I need to increase the percentage)You can run the tests:
And coverage
Testing checklist
I'm including the same manual testing checklist I used during TDD
Login
storyblok-cli login
login default
login
(interactive mode)With email
If correct
.netrc
file on~/.netrc
with the following properties:api.storyblok.com
if region iseu
us
[app.storyblokchina.cn](http://app.storyblokchina.cn/)
if region iscn
[api-ca.storyblok.com](http://api-ca.storyblok.com/)
if region isca
[api-ap.storyblok.com](http://api-ap.storyblok.com/)
if region isap
Successfully logged in with email
+ emailWith Token (SSO)
If correct:
.netrc
file on~/.netrc
with the following properties:api.storyblok.com
if region iseu
api-us.storyblok.com
if region isus
app.storyblokchina.cn
if region iscn
api-ca.storyblok.com
if region isca
api-ap.storyblok.com
if region isap
eu
unless--region
is usedSuccessfully logged in with token
If error
The token provided YFFZ**************************************************************************************************** is invalid: 401 Unauthorized
login with token
login --token
If correct:
.netrc
file on~/.netrc
with the following properties:api.storyblok.com
if region iseu
- [ ]
api-us.storyblok.com
if region isus
- [ ]
app.storyblokchina.cn
if region iscn
- [ ]
api-ca.storyblok.com
if region isca
- [ ]
api-ap.storyblok.com
if region isap
eu
unless--region
is usedSuccessfully logged in with token
If error
No response from server, please check if you are correctly connected to internet
if connection failsError logging with token
if else.login with region
login --region
should prompt a message if user is already logged in
should get credentials without login if (Prompt user is already logged in)
STORYBLOK_LOGIN
,STORYBLOK_TOKEN
,STORYBLOK_REGION
are presentTRAVIS_TORYBLOK_LOGIN
,TRAVIS_STORYBLOK_TOKEN
,TRAVIS_STORYBLOK_REGION
are presentLogout
storyblok-cli logout
.netrc
fileYou are already logged out. If you want to login, please use the login command.