PostCSS style transformation: fail gracefully instead of throwing an error #56093
Conversation
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
zaguiini
added
[Type] Bug
|
@strarsis I'd appreciate a review here. |
|
Size Change: +83 B (0%) Total Size: 1.7 MB
ℹ️ View Unchanged
|
|
@zaguiini: Great addition! 👍 |
afercia
reviewed
Nov 15, 2023
| ].filter( Boolean ) | ||
| ).process( css, {} ).css; // use sync PostCSS API | ||
| } catch ( error ) { | ||
| if ( ! ( error instanceof CssSyntaxError ) ) { |
There was a problem hiding this comment.
I'm not familiar enough with postCSS to know when an error is not a CssSyntaxError. In what scenario this condition will be true and the console warn be useful?
afercia
reviewed
Nov 15, 2023
| @@ -4,6 +4,28 @@ | |||
| import transformStyles from '../transform-styles'; | |||
|
|
|||
| describe( 'transformStyles', () => { | |||
| it( 'should not throw error in case of invalid css', () => { | |||
There was a problem hiding this comment.
This test is welcome but it actually doesn't not test what was broken in the UI of the custom CSS advanced panel. It just tests the return value is null when there is a CSS error. It doesn't make sure the UI works as expected.
|
Quickly tested and it does fix the issue. I'd recommend to add an E2E test for the feature that was actually broken in
|
|
On a related note: Previously, when there was an error in the CSS added to the textarea, the console warn gave some hint on the type of error:
We could argue that wasn't that useful to average users. However, now there won't be anything in the console. I read there are nice ways to handle the error output, see https://postcss.org/docs/postcss-runner-guidelines#output Input: Dupming this to the console: That would provide some more useful information to users. It would also highlight which line(s) of the source CSS errored:
|
Thanks. @afercia I do not have the bandwidth to add the E2E test, so the best I can do right now is to fix the unit test and merge this PR. Would you be able to add the E2E test as a follow-up PR? |
zaguiini
force-pushed
the
fix/transform-styles-custom-css-input-tooltip
branch
from
ce181e0 to
6fb144f
Compare
|
@afercia, we have that now: 
I have updated the tests and rebased the branch with |
zaguiini
force-pushed
the
fix/transform-styles-custom-css-input-tooltip
branch
from
6fb144f to
bf10159
Compare
This was referenced Nov 22, 2023
@zaguiini I'd really love to change the current red icon to a standard notice because the red icon isn't accessible at all. I will create a new issue and link to this PR as a reminder that an E2E test should be added as well. I like the |
jsnajdr
approved these changes
Nov 22, 2023
fullofcaffeine
pushed a commit
that referenced
this pull request
Nov 22, 2023
…error (#56093) * PostCSS style transformation: fail gracefully instead of throwing error * Display better warning messages
fullofcaffeine
pushed a commit
that referenced
this pull request
Nov 23, 2023
…error (#56093) * PostCSS style transformation: fail gracefully instead of throwing error * Display better warning messages
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes #56082.
What?
#49521 introduces a regression in which invalid CSS throws an error. This PR fixes the problem by letting the transformation fail gracefully.
Why?
#49521 changed the internal CSS transformer to fix a couple of issues, but invalid CSS handling was not exhaustively tested.
How?
I'm wrapping the transformation function with a try-catch block and returning null in case it fails to parse the CSS.
Testing Instructions
Follow the instructions in #56082.