fix: the ASCII whitespaces are preserved so can not be escaped#1668
Open
JounQin wants to merge 3 commits into
Open
fix: the ASCII whitespaces are preserved so can not be escaped#1668JounQin wants to merge 3 commits into
JounQin wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds coverage and updates escaping behavior for CSS Modules local identifiers, particularly around reserved/whitespace characters, to address issue #1626.
Changes:
- Added a regression test + fixtures for
"modules" optionissue #1626. - Changed
escapeLocalIdentto escape reserved/whitespace characters using hex-style escape sequences (and updated snapshots accordingly). - Updated spellchecker config to ignore generated test outputs.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| test/modules-option.test.js | Adds a new regression test for issue #1626 under the "modules" option suite. |
| test/fixtures/modules/issue-1626/source.js | Adds a JS entry fixture that imports the CSS fixture for issue #1626. |
| test/fixtures/modules/issue-1626/source.css | Adds a CSS selector fixture intended to cover escaping around spaces/special chars. |
| test/snapshots/modules-option.test.js.snap | Updates snapshots to reflect the new escaping output and adds snapshots for issue #1626. |
| src/utils.js | Changes escapeLocalIdent escaping strategy for reserved/whitespace characters. |
| .cspell.json | Ignores test/outputs in spellchecking. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+262
to
+265
| const reversedChartMapper = {}; | ||
|
|
||
| // eslint-disable-next-line no-control-regex | ||
| const filenameReservedRegex = /[<>:"/\\|?*]/g; | ||
| const filenameReservedRegex = /([<>:"/\\|?*\s])/g; |
Comment on lines
+275
to
+289
| .replace(filenameReservedRegex, (_, $1) => { | ||
| // normalize Windows path `\` as unix `/` for constancy | ||
| const char = $1 === "\\" ? "/" : $1; | ||
|
|
||
| if (reversedChartMapper[char]) { | ||
| return reversedChartMapper[char]; | ||
| } | ||
|
|
||
| const hex = char.charCodeAt(0).toString(16).toUpperCase(); | ||
| const escaped = `\\C${hex}`; | ||
|
|
||
| reversedChartMapper[char] = escaped; | ||
|
|
||
| return escaped; | ||
| }) |
|
|
||
| // eslint-disable-next-line no-control-regex | ||
| const filenameReservedRegex = /[<>:"/\\|?*]/g; | ||
| const filenameReservedRegex = /([<>:"/\\|?*\s])/g; |
|
|
||
| return escaped; | ||
| }) | ||
| .replace(reControlChars, "-") |
Comment on lines
+1
to
+7
| a-b { | ||
| color: red; | ||
| } | ||
|
|
||
| a\C20b /* i.e. with spaces */ { | ||
| color: red; | ||
| } |
| return path.sep === "\\" ? file.replace(/\\/g, "/") : file; | ||
| } | ||
|
|
||
| const reversedChartMapper = {}; |
6 tasks
Comment on lines
+262
to
+265
| const reversedChartMapper = {}; | ||
|
|
||
| // eslint-disable-next-line no-control-regex | ||
| const filenameReservedRegex = /[<>:"/\\|?*]/g; | ||
| const filenameReservedRegex = /([<>:"/\\|?*\s])/g; |
Comment on lines
+275
to
+289
| .replace(filenameReservedRegex, (_, $1) => { | ||
| // normalize Windows path `\` as unix `/` for constancy | ||
| const char = $1 === "\\" ? "/" : $1; | ||
|
|
||
| if (reversedChartMapper[char]) { | ||
| return reversedChartMapper[char]; | ||
| } | ||
|
|
||
| const hex = char.charCodeAt(0).toString(16).toUpperCase(); | ||
| const escaped = `\\C${hex}`; | ||
|
|
||
| reversedChartMapper[char] = escaped; | ||
|
|
||
| return escaped; | ||
| }) |
Comment on lines
+5
to
+6
| a\C20b /* i.e. with spaces */ { | ||
| color: red; |
|
|
||
| // eslint-disable-next-line no-control-regex | ||
| const filenameReservedRegex = /[<>:"/\\|?*]/g; | ||
| const filenameReservedRegex = /([<>:"/\\|?*\s])/g; |
| return path.sep === "\\" ? file.replace(/\\/g, "/") : file; | ||
| } | ||
|
|
||
| const reversedChartMapper = {}; |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1668 +/- ##
==========================================
+ Coverage 96.25% 96.28% +0.02%
==========================================
Files 10 10
Lines 1201 1210 +9
Branches 463 465 +2
==========================================
+ Hits 1156 1165 +9
Misses 40 40
Partials 5 5 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Member
Author
alexander-akait
left a comment
alexander-akait
left a comment
Member
There was a problem hiding this comment.
Let's fix copilot review and we can merge, note - generation classes with such characters is a little bit different in experiments.css than here, but it is fine
This file contains hidden or 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
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.
This PR contains a:
Motivation / Use-Case
close #1626
Breaking Changes
N/A
Additional Info
related https://infra.spec.whatwg.org/#ascii-whitespace
cc @alexander-akait