-
Notifications
You must be signed in to change notification settings - Fork 13
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 YAML Config Editing Console Runtime Errors by Switching to Yaml Library #289
Fix YAML Config Editing Console Runtime Errors by Switching to Yaml Library #289
Conversation
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
<div className={`${font.className} ${errorsStyle}`}> | ||
<p className="max-w-[100%]">{`${jsYamlError.reason} ${ | ||
jsYamlError.mark && `(Line ${jsYamlError.mark.line})` | ||
<p className="max-w-[100%]">{`${yamlError.code} ${ |
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.
Question: Why are we using the short version here? IMHO that is hard to understand, and we seem have enough room in the console for the long version?
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.
Sure, it will be change to the long version
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 long version message is pushed and committed.
let jsonData: any; | ||
try { | ||
jsonData = YAML.parse(config, { logLevel: "error", schema: "failsafe" }) as any; | ||
} catch (e) {} |
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.
Question: This silently catches and fully ignores the error. That seems dangerous to me. Why do we need to do 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.
I thought we were already displaying errors in the validation console; when editing the config, the console is overflowing with errors.
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.
Could you please document this in the code in both locations? Because this empty catch block very much looks like a bug otherwise :)
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 related comments added to the code
const jsonData = useMemo(() => { | ||
try { | ||
return YAML.parse(value, { logLevel: "error", schema: "failsafe" }) as IConfig; | ||
} catch (error: unknown) {} |
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 question as above.
…rrors-when-editing-the-yaml-config
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
LGTM 👍 |
This pull request addresses the issue of multiple runtime errors generated during YAML config editing, attributed to the JS-yaml library. To enhance the user experience and provide more detailed error information, this PR replaces JS-yaml with the Yaml library. The Yaml library not only eliminates runtime errors but also includes comprehensive error details such as start line, end line, start column, and end column, facilitating better debugging and validation of YAML configurations.
Also, in the Yaml library, there are two options for displaying validation error messages:
1- Short Message:
2- Long Message:
For this pull request, a short message is implemented, but it can be changed as per the reviewers' request.