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(site-playground): add missing error handling to fix WSOD (#991) #1154

Merged
merged 1 commit into from Jul 2, 2021
Merged

fix(site-playground): add missing error handling to fix WSOD (#991) #1154

merged 1 commit into from Jul 2, 2021

Conversation

sigveio
Copy link
Collaborator

@sigveio sigveio commented Jun 30, 2021

This pull request aims to fix the playground WSOD reported in #991

I found two related issues leading to this behaviour:

  1. Uncaught errors thrown by prettier.format() when the "Format" button is used.
  2. The error object from caught postcss errors when using "Run" were being passed to setOutput() in order to output it to the output pane of the playground. Here it was passed in raw as EditorProps.value - which expected a string, not an object.

The two cases throws slightly different types of errors.

On most syntax errors, Prettier throws a SyntaxError wrapping some output from CssSyntaxError thrown by the css parser - which is quite verbose with numbered line output and such in the message. There are also some edge cases causing Prettier to throw different types of errors. And the line numbering from the SyntaxError does not correspond to the input lines.

While postcss throws a clean CssSyntaxError with good/usable information.

I therefore introduced a simple error handling function which conforms the errors to a short and concise format (using common factors), and outputs it as a red bar right above the code panes. So now the error will for the most part be identical whether one clicks "Run" or "Format"

Screenshot 2021-06-30 at 09 46 28

This is also more friendly for devices with small screen resolutions, where outputting errors to the output pane would be out of view and could cause confusion (i.e. "nothing happening" when clicking the buttons)

I tried to follow the general style/conventions of the existing code.

Closes #991

@codecov-commenter
Copy link

codecov-commenter commented Jun 30, 2021

Codecov Report

Merging #1154 (9ea7ea6) into master (ef098b1) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1154   +/-   ##
=======================================
  Coverage   96.43%   96.43%           
=======================================
  Files         116      116           
  Lines        3592     3592           
  Branches     1054     1054           
=======================================
  Hits         3464     3464           
  Misses        119      119           
  Partials        9        9           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ef098b1...9ea7ea6. Read the comment docs.

@sigveio
Copy link
Collaborator Author

sigveio commented Jun 30, 2021

In case anyone would be curious, the rationale for choosing this error format is:

From the SyntaxError wrapped CssSyntaxError (thrown from the Prettier call), we only really have the location and the already formatted error message available:

debug2

Which looks like this:

CssSyntaxError: Missed semicolon (3:14)
  1 | /* write your css below */
  2 | .crashes {
> 3 |   font-size: 15pt
    |              ^
  4 |   background: red;
  5 | }

While in the CssSyntaxError thrown by postcss, the order/format of things is slightly different:

CssSyntaxError: <css input>:3:14: Missed semicolon

But... here we do have access to the different parts as properties of the error:

debug1

So the easiest way to get them to conform to a common format, without too much fiddly and unnecessary processing, was to simply discard anything after the first line on the errors from Prettier and format the CssSyntaxError from postcss in the same order/style.

The line/column number is anyway available in the error, and the editor itself also attempts to highlight. So discarding the verbose output with the line/column indicator doesn't really lose any valuable information. Keeping the message consistent and concise seems more important, considering the location it's displayed.

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

Copy link
Collaborator

@ludofischer ludofischer left a comment

Choose a reason for hiding this comment

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

LGTM

padding: 0.3em 1em;
}

.inputError[data-theme='light'] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Nice touch!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks :)

@ludofischer ludofischer merged commit 3ebc3d9 into cssnano:master Jul 2, 2021
@sigveio sigveio deleted the 991-fix-playground-error branch July 2, 2021 18:49
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.

[Bug] : your playground crashes when clicking "run" on invalid css
4 participants