fix(turbopack): remove unpure selector err report & invalid selector err#71
fix(turbopack): remove unpure selector err report & invalid selector err#71fireairforce merged 1 commit intoutoofrom
Conversation
Summary of ChangesHello @fireairforce, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request aims to refine the error handling within Turbopack's CSS processing, specifically addressing issues related to selector parsing errors. By suppressing certain error reports and broadly ignoring Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request disables error reporting for 'unpure' and invalid CSS selectors in Turbopack. While this may resolve the immediate issues, the changes are quite broad and could have unintended side effects. Specifically, all selector-related errors and warnings from lightningcss are now being silenced, which could hide legitimate bugs from developers and make debugging difficult. Additionally, some code related to selector validation is now dead and should be removed rather than commented out or suppressed with #[allow(unused)].
| lightningcss::error::SelectorError::UnsupportedPseudoClass(name), | ||
| ) if name.as_ref() == "global" => { | ||
| // Ignore all SelectorError errors | ||
| lightningcss::error::ParserError::SelectorError(..) => { |
There was a problem hiding this comment.
This change now ignores all SelectorError warnings. While this may be intended to handle specific cases (like with CSS preprocessors), this is a very broad change that could hide legitimate selector issues from developers. This might make debugging CSS unexpectedly difficult. If possible, it would be safer to only ignore the specific selector errors that are causing problems, rather than all of them.
| if matches!(e.kind, lightningcss::error::ParserError::SelectorError(..)) { | ||
| return Ok(ParseCssResult::Unparsable.cell()); | ||
| } |
There was a problem hiding this comment.
Silently ignoring all SelectorError parsing errors and marking the file as unparsable could lead to a poor developer experience. Developers won't receive any feedback on why their CSS file is failing to parse, which could make debugging very difficult. Consider at least adding a debug! level log here to record the suppressed error, so that the information is not completely lost when troubleshooting.
| let mut validator = CssValidator { errors: Vec::new() }; | ||
| ss.visit(&mut validator).unwrap(); | ||
|
|
||
| for err in validator.errors { | ||
| err.report(source); | ||
| } | ||
| // TODO: remove pure selector | ||
| // for err in validator.errors { | ||
| // err.report(source); | ||
| // } |
There was a problem hiding this comment.
Instead of commenting out the error reporting, it would be cleaner to remove the now-unused code. Since the errors from CssValidator are no longer reported, the instantiation and use of CssValidator on lines 459-460 appear to be dead code and can be removed along with the commented-out loop.
| } | ||
|
|
||
| impl CssError { | ||
| #[allow(unused)] |
There was a problem hiding this comment.
While #[allow(unused)] silences the compiler warning, the underlying issue is that the report function, and likely the entire CssError enum, are no longer used after the changes in this PR. To improve maintainability, it would be better to remove the CssError enum and its impl block entirely, as they seem to be dead code now.
1 participant
For issue:
utooland/utoo#2268 (comment)
utooland/utoo#2268 (comment)