Skip to content
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: gracefully handle invalid JSON in data-style attribute (#1393) #1394

Merged
merged 1 commit into from
May 19, 2020

Conversation

georgel-pop-lr
Copy link
Member

Is fixing #1393 If data-style attribute is not a valid JSON an exception will be thrown and the code will stop working.

@wincent
Copy link
Contributor

wincent commented May 18, 2020

Thanks @georgel-pop-lr. Can you please run yarn format on this (that's what CI is complaining about) and do a git commit --amend and force push to update this with a new title (both the commit subject line and the PR title need to start with "fix: " otherwise the semantic PR bot will complain (not sure why it hasn't yet). You could use a subject/title like this:

fix: gracefully handle invalid JSON in data-style attribute (#1393)

@georgel-pop-lr georgel-pop-lr changed the title Fixes #1393 If data-style attribute is not a valid JSON an exception w… fix: gracefully handle invalid JSON in data-style attribute (#1393) May 18, 2020
@georgel-pop-lr
Copy link
Member Author

Thanks @georgel-pop-lr. Can you please run yarn format on this (that's what CI is complaining about) and do a git commit --amend and force push to update this with a new title (both the commit subject line and the PR title need to start with "fix: " otherwise the semantic PR bot will complain (not sure why it hasn't yet). You could use a subject/title like this:

fix: gracefully handle invalid JSON in data-style attribute (#1393)

Thanks @wincent, hope is fine now.

Comment on lines 418 to 421
} catch (e) {
console.warn(
'Unable to parse data-styles attribute!'
);
Copy link
Contributor

@wincent wincent May 18, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple things here:

  1. Linter may warn about unused variable e here, so either use it (in the log statement) or mark it as intentionally unused by starting it with an underscore (ie. _error). Note also that we try to avoid abbreviations if we can (ie. prefer error over e), except for things like i for loop variables.
  2. We generally try to avoid anything that might resemble console spew in Liferay projects because it can be hard for customers to distinguish between bugs and mere diagnostic information (see guidelines on this); sometimes they see console output as a sign of bugginess and any "helpful" output we might include actually just confuses them. And they may be alarmed to see a message ending in an exclamation point. As such, I'd actually just swallow this error rather than log it — in some browsers, the stack trace that comes with console.warn looks particularly scary.

Copy link
Contributor

@wincent wincent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lg2m

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants