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
Avoid infinite loop when highlighting an empty input #14165
Conversation
fix(highlight): When s is an empty string, highlight will die cycle
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/50797/ |
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 wil good if it gets change if (shouldHighlight(options)) {
to if (shouldHighlight(options) && code) {
@@ -262,6 +262,7 @@ export function getChalk(options: Options) { | |||
* Highlight `code`. | |||
*/ | |||
export default function highlight(code: string, options: Options = {}): string { | |||
if (!code) return 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.
if (!code) return 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.
This should be removed.
@@ -262,6 +262,7 @@ export function getChalk(options: Options) { | |||
* Highlight `code`. | |||
*/ | |||
export default function highlight(code: string, options: Options = {}): string { | |||
if (!code) return code; | |||
if (shouldHighlight(options)) { |
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 (shouldHighlight(options)) { | |
if (shouldHighlight(options) && 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.
This would look more clean.
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 suffices to check empty string only code !== ""
because regexp#exec
always return non-null results for empty string.
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.
Yeah, empty string check will be more efficient and safe.
@@ -262,6 +262,7 @@ export function getChalk(options: Options) { | |||
* Highlight `code`. | |||
*/ | |||
export default function highlight(code: string, options: Options = {}): string { | |||
if (!code) return code; | |||
if (shouldHighlight(options)) { |
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 would look more clean.
@@ -262,6 +262,7 @@ export function getChalk(options: Options) { | |||
* Highlight `code`. | |||
*/ | |||
export default function highlight(code: string, options: Options = {}): string { | |||
if (!code) return 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.
This should be removed.
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 fine with the current approach, but feel free to merge the two if
statements. However, if you merge them the check should be code !== "" && shouldHighlight(options)
rather than shouldHighlight(options) && code !== ""
, because the code !== ""
check is less expensive.
Thanks for your PR! As a suggestion for the future, avoid opening PR from the |
fix(highlight): When text is an empty string, highlight will die cycle