-
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
Merged
Merged
Changes from 1 commit
Commits
Show all changes
48 commits
Select commit
Hold shift + click to select a range
5854401
feat: api client logic
alvarosabu 55f33fa
chore: lint fix
alvarosabu 81047b7
feat: netrc credentials logic
alvarosabu 316ce74
chore: remove unused imports on creds tests
alvarosabu 193e1b6
feat: migrate to inquirer/prompts
alvarosabu 303b248
feat: handling path for package.json on tests
alvarosabu 7254e74
feat: check region on login command
alvarosabu 56960ee
feat: login with token and with email
alvarosabu c908176
feat: improved error handling
alvarosabu fe4733d
tests: test coverage for login actions
alvarosabu 1cbd2be
test: use correct masktoken fn
alvarosabu c60f830
tests: removed unused ci option since token acts like one
alvarosabu 9740997
tests: login actions tests
alvarosabu 5755511
tests: login --token tests
alvarosabu 7c83aed
tests: login command tests with different login strategies
alvarosabu eda9600
tests: coverage
alvarosabu b883805
chore: minor test fix
alvarosabu ba7e913
tests: remove commented code
alvarosabu 33ec33a
feat: isAutorized
alvarosabu 9f49385
feat: logout command
alvarosabu 0989948
feat: session credentials handler and updated tests
alvarosabu 32ff580
chore: fix lint
alvarosabu fb0aa16
chore: improve auth code login message
alvarosabu 54df30e
chore: increase session coverage
alvarosabu 7e448ae
feat: narrow the typesafe of constants
alvarosabu 779d41a
feat(types): improved regions code typing
alvarosabu aece279
feat: set module resolution to node for tsconfig and add eslint flat …
alvarosabu d4ecf66
chore: force npm run dev to always stub the build for dev
alvarosabu 64ef7db
chore(tests): type assertion for mocks
alvarosabu dc677f9
chore: extremely weird choice of identation
alvarosabu 76cba51
feat: improve error handling
alvarosabu de2f3bb
tests: vi.mocked for the typescript win
alvarosabu 32eb04b
chore: remove unused code for linter
alvarosabu d786b9c
Merge branch 'next' into feature/login-cmd
alvarosabu 7449f5f
feat: improved error handling part 1, api errors and command errors
alvarosabu 90ccd57
tests: update tests and use msw for api mocks
alvarosabu 2b48927
chore: lint and removing unnecesary else
alvarosabu 4c3582a
chore: lint
alvarosabu 658202e
feat: refactor removeNetrcEntry to make machineName required
alvarosabu c1f0eef
feat: change order of `removeNetrcEntry` params
alvarosabu f7a4910
feat: remove casting in favor of runtime checking
alvarosabu 0201fad
feat: verbose mode
alvarosabu 432b641
feat: added correct flow when user doesnt require otp
alvarosabu 72d788f
feat: handle user cancelation error when inquirer is prompting
alvarosabu 1860665
feat: remove all netrc entries on logout
alvarosabu c63d382
feat: added http response code to api error verbose mode
alvarosabu 3235497
feat: remove unnecesary netrc handling warnings
alvarosabu c8472d0
feat: improved error handling for file permissions
alvarosabu File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,5 +1,6 @@ | ||
{ | ||
"name": "storyblok", | ||
"type": "module", | ||
"version": "0.0.0", | ||
"packageManager": "[email protected]", | ||
"description": "Storyblok CLI", | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file was deleted.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,64 @@ | ||
import { FetchError } from 'ofetch' | ||
|
||
export const API_ACTIONS = { | ||
login: 'login', | ||
login_with_token: 'Failed to log in with token', | ||
login_with_otp: 'Failed to log in with email, password and otp', | ||
login_email_password: 'Failed to log in with email and password', | ||
} as const | ||
|
||
export const API_ERRORS = { | ||
unauthorized: 'The user is not authorized to access the API', | ||
network_error: 'No response from server, please check if you are correctly connected to internet', | ||
invalid_credentials: 'The provided credentials are invalid', | ||
timeout: 'The API request timed out', | ||
} as const | ||
|
||
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) | ||
} | ||
else { | ||
alvarosabu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
throw new APIError('timeout', action, error) | ||
} | ||
} | ||
|
||
export class APIError extends Error { | ||
errorId: string | ||
cause: string | ||
code: number | ||
messageStack: string[] | ||
error: FetchError | undefined | ||
|
||
constructor(errorId: keyof typeof API_ERRORS, action: keyof typeof API_ACTIONS, error?: FetchError, customMessage?: string) { | ||
super(customMessage || API_ERRORS[errorId]) | ||
this.name = 'API Error' | ||
this.errorId = errorId | ||
this.cause = API_ERRORS[errorId] | ||
this.code = error?.response?.status || 0 | ||
this.messageStack = [API_ACTIONS[action], customMessage || API_ERRORS[errorId]] | ||
this.error = error | ||
} | ||
|
||
getInfo() { | ||
return { | ||
name: this.name, | ||
message: this.message, | ||
cause: this.cause, | ||
errorId: this.errorId, | ||
stack: this.stack, | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,14 @@ | ||
export class CommandError extends Error { | ||
constructor(message: string) { | ||
super(message) | ||
this.name = 'Command Error' | ||
} | ||
|
||
getInfo() { | ||
return { | ||
name: this.name, | ||
message: this.message, | ||
stack: this.stack, | ||
} | ||
} | ||
} |
File renamed without changes.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
import { konsola } from '..' | ||
import { APIError } from './api-error' | ||
import { CommandError } from './command-error' | ||
import { FileSystemError } from './filesystem-error' | ||
|
||
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) { | ||
console.log('error', error) | ||
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 | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,26 @@ | ||
const FS_ERRORS = { | ||
file_not_found: 'The file requested was not found', | ||
permission_denied: 'Permission denied while accessing the file', | ||
} | ||
|
||
export class FileSystemError extends Error { | ||
errorId: string | ||
cause: string | ||
|
||
constructor(message: string, errorId: keyof typeof FS_ERRORS) { | ||
super(message) | ||
this.name = 'File System Error' | ||
this.errorId = errorId | ||
this.cause = FS_ERRORS[errorId] | ||
} | ||
|
||
getInfo() { | ||
return { | ||
name: this.name, | ||
message: this.message, | ||
cause: this.cause, | ||
errorId: this.errorId, | ||
stack: this.stack, | ||
} | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,4 @@ | ||
export * from './api-error' | ||
export * from './command-error' | ||
export * from './error' | ||
export * from './filesystem-error' |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 withlogin_with_token
? This way it should be easier to maintain/extendThere 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 ofAPIError
with the correspondent error message depending on the response status, it's not a factory.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.Wouldn't be enough to change the error message format once on the
konsola
file?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 UIThere 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 byhandleAPIError
, 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.
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 problemApply 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.