fix: enforce strict syntax for @charset in no-invalid-at-rules
#192
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the no-invalid-at-rules rule to strictly validate @charset declarations, ensuring they use double quotes, exactly one space, and terminate with a semicolon.
- Introduced
charsetPatternandvalidateCharsetRuleto enforce syntax in the rule implementation - Added test cases covering valid and invalid
@charsetvariations - Updated documentation to detail the strict requirements and examples
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/rules/no-invalid-at-rules.test.js | Added valid/invalid test cases for @charset |
| src/rules/no-invalid-at-rules.js | Implemented validateCharsetRule and regex check |
| docs/rules/no-invalid-at-rules.md | Added note and examples for strict @charset |
Comments suppressed due to low confidence (1)
docs/rules/no-invalid-at-rules.md:75
- [nitpick] Describing
@charsetas "not an at-rule" is misleading; consider rephrasing to clarify that it is a special at-rule requiring placement as the very first statement in a stylesheet.
Note on `@charset`: Although it begins with an `@` symbol, it is not an at-rule. It is a specific byte sequence of the following form:
lumirlumir
added
the
accepted
|
ping @thecalamiity |
| { | ||
| code: "@charset 'UTF-8';", | ||
| output: '@charset "UTF-8";', | ||
| errors: [ | ||
| { | ||
| messageId: "invalidPrelude", | ||
| data: { | ||
| name: "charset", | ||
| prelude: "'UTF-8'", | ||
| expected: "<string>", | ||
| }, | ||
| line: 1, | ||
| column: 10, | ||
| endLine: 1, | ||
| endColumn: 17, | ||
| }, | ||
| ], | ||
| }, |
There was a problem hiding this comment.
In the case of a syntax error in @charset, the error message invalidPrelude seems incorrect, such as in this test, the message would be:
Invalid prelude ''UTF-8'' found for at-rule '@charset'. Expected '<string>'.
Here it says, <string> is expected to be used, which is already used here, but with wrong quotes.
And in the case of @charset "UTF-8", where a semicolon is missing, it has the same error message.
What do you think about adding an error message only to report syntax errors?
There was a problem hiding this comment.
Are you suggesting that instead of reporting invalidPrelude, we should report a single, more general error for all these cases? Something like:
invalidCharsetSyntax: "Invalid @charset syntax. Expected: @charset \"<encoding-name>\"; (double quotes, exactly one space after @charset, and a semicolon at the end)."
There was a problem hiding this comment.
Yes, but i feel this error message seems a bit descriptive; instead, we can just suggest the correct syntax, like for -
@charset "UTF-8" message can be something like this:
Invalid @charset syntax. Expected '@charset "UTF-8";'
Please feel free to choose the words that you find more accurate here.
There was a problem hiding this comment.
Just updated the @charset error message - PTAL when you can.
| end: { | ||
| line: loc.start.line, | ||
| column: loc.start.column + name.length + 1, |
There was a problem hiding this comment.
Should be end?
| end: { | |
| line: loc.start.line, | |
| column: loc.start.column + name.length + 1, | |
| end: { | |
| line: loc.end.line, | |
| column: loc.end.column + name.length + 1, |
There was a problem hiding this comment.
Using start is correct because we only want to highlight the at-rule name (@charset).
There was a problem hiding this comment.
Shouldn't we report the whole node?
@charse "utf-8"
^^^^^^^^^^^^^^^
@charset "";
^^^^^^^^^^^^
| const encoding = | ||
| preludeText.match(charsetEncodingPattern)?.[1] ?? "UTF-8"; |
There was a problem hiding this comment.
Why do we need ?? "UTF-8"? In which case will we reach the right operand?
There was a problem hiding this comment.
When the encoding is missing, e.g., @charset "";
There was a problem hiding this comment.
Can we add this test case? empty string.
Prerequisites checklist
What is the purpose of this pull request?
To enforce strict syntax requirements for the
@charsetrule. This ensures that only valid@charsetdeclarations are accepted.What changes did you make? (Give an overview)
@charsetsyntax. Now enforces:@charset.@charsetusages.@charset.Related Issues
Fixes #185
Is there anything you'd like reviewers to focus on?