-
-
Notifications
You must be signed in to change notification settings - Fork 32
fix: enforce strict syntax for @charset in no-invalid-at-rules
#192
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: enforce strict syntax for @charset in no-invalid-at-rules
#192
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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:
nzakas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall, this looks really good. What do you think about adding autofix to this rule? It seems like it should possible to just reformat the @charset so that it's correct.
|
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using start is correct because we only want to highlight the at-rule name (@charset).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't we report the whole node?
@charse "utf-8"
^^^^^^^^^^^^^^^
@charset "";
^^^^^^^^^^^^
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We report each issue individually rather than flagging the entire at-rule in one go.
For example, if the name is invalid we report unknownAtRule, and if the prelude is missing we report missingPrelude, etc.
src/rules/no-invalid-at-rules.js
Outdated
| const encoding = | ||
| preludeText.match(charsetEncodingPattern)?.[1] ?? "UTF-8"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need ?? "UTF-8"? In which case will we reach the right operand?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When the encoding is missing, e.g., @charset "";
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we add this test case? empty string.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’ve added a test for the empty string case and changed the behavior so we no longer autofix when the encoding is missing or whitespace-only. Previously we defaulted to "UTF-8", but I found this surprising since it guessed a value the user didn’t provide. Now it just reports the issue without autofixing. What do you think of this approach?
snitin315
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks! Leaving it open for 2nd review
nzakas
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks.
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?