Improve parsing of shadow values#20080
Conversation
This will now instead parse the shadow values and tries to identify each argument of the value. This also augments teh existing code by tracking some of the known functions that return length units, and also color functions that return colors. With this approach, we know for use that our built-in `--spacing(…)` returns a length, and `--alpha(…)` returns a color.
We can re-use the existing `isColor`, but it's also checking for hex colors and color functions using a regex and we _know_ that we are not dealing with color functions so this additional work is unnecessary.
|
Caution Review failedPull request was closed or merged during review No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
WalkthroughThis PR refactors shadow color replacement from string/regex parsing to an AST-driven approach using 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tailwindcss/src/utils/replace-shadow-colors.test.ts (1)
44-95: ⚡ Quick winAdd one mixed-case regression case.
This change relies on case-insensitive detection, but the new table only exercises lowercase inputs. A single
BlAcKorRGB(...)case here would lock that contract in.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/tailwindcss/src/utils/replace-shadow-colors.test.ts` around lines 44 - 95, The tests only exercise lowercase color inputs but replaceShadowColors must be case-insensitive; add a mixed-case color variant (e.g., "BlAcK" and/or "RgB(0, 0, 0)" or "RGB(0,0,0)" ) to the color lists used in the cartesian loop and the it.each array so the test suite verifies mixed-case detection; update the color entries in the test cases that build inputs for replaceShadowColors (referencing replaceShadowColors and the test blocks that iterate over the color arrays) to include one mixed-case example.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@packages/tailwindcss/src/utils/replace-shadow-colors.test.ts`:
- Around line 44-95: The tests only exercise lowercase color inputs but
replaceShadowColors must be case-insensitive; add a mixed-case color variant
(e.g., "BlAcK" and/or "RgB(0, 0, 0)" or "RGB(0,0,0)" ) to the color lists used
in the cartesian loop and the it.each array so the test suite verifies
mixed-case detection; update the color entries in the test cases that build
inputs for replaceShadowColors (referencing replaceShadowColors and the test
blocks that iterate over the color arrays) to include one mixed-case example.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 25b011b0-1438-402b-af38-68aaef5fc9de
📒 Files selected for processing (4)
packages/tailwindcss/src/utilities.test.tspackages/tailwindcss/src/utils/is-color.tspackages/tailwindcss/src/utils/replace-shadow-colors.test.tspackages/tailwindcss/src/utils/replace-shadow-colors.ts
Confidence Score: 5/5Safe to merge — the change is well-scoped, the new classification logic is exhaustively tested, and the fallback behaviour (returning the shadow unchanged when ambiguous) is conservative and correct. The logic is straightforward and the three-state model (definite color → replace immediately; lengths ≥ 2, unknowns = 0 → append currentcolor; unknowns = 1 → treat the sole unknown as the color; otherwise → leave unchanged) matches the full test matrix added by this PR. The removal of the stateful /g flag on the LENGTH regex also fixes a latent correctness issue. No code paths produce unexpected output. No files require special attention. Reviews (3): Last reviewed commit: "update changelog" | Re-trigger Greptile |
Here is everything you need to know about this update. Please take a good look at what changed and the test results before merging this pull request. ### What changed? #### ✳️ eslint (9.35.0 → 9.36.0) · [Repo](https://github.com/eslint/eslint) · [Changelog](https://github.com/eslint/eslint/blob/main/CHANGELOG.md) <details> <summary>Release Notes</summary> <h4><a href="https://github.com/eslint/eslint/releases/tag/v9.36.0">9.36.0</a></h4> <blockquote><h2 dir="auto">Features</h2> <ul dir="auto"> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/47afcf668df65eac68d7b04145d037037010a076"><code class="notranslate">47afcf6</code></a> feat: correct <code class="notranslate">preserve-caught-error</code> edge cases (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20109">#20109</a>) (Francesco Trotta)</li> </ul> <h2 dir="auto">Bug Fixes</h2> <ul dir="auto"> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/75b74d865d3b8e7fa3bcf5ad29f4bf6d18d1310e"><code class="notranslate">75b74d8</code></a> fix: add missing rule option types (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20127">#20127</a>) (ntnyq)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/1c0d85049e3f30a8809340c1abc881c63b7812ff"><code class="notranslate">1c0d850</code></a> fix: update <code class="notranslate">eslint-all.js</code> to use <code class="notranslate">Object.freeze</code> for <code class="notranslate">rules</code> object (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20116">#20116</a>) (루밀LuMir)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/7d61b7fadc9c5c6f2b131e37e8a3cffa5aae8ee6"><code class="notranslate">7d61b7f</code></a> fix: add missing scope types to <code class="notranslate">Scope.type</code> (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20110">#20110</a>) (Pixel998)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/7a670c301b58609017ce8cfda99ee81f95de3898"><code class="notranslate">7a670c3</code></a> fix: correct rule option typings in <code class="notranslate">rules.d.ts</code> (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20084">#20084</a>) (Pixel998)</li> </ul> <h2 dir="auto">Documentation</h2> <ul dir="auto"> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/b73ab12acd3e87f8d8173cda03499f6cd1f26db6"><code class="notranslate">b73ab12</code></a> docs: update examples to use <code class="notranslate">defineConfig</code> (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20131">#20131</a>) (sethamus)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/31d93926990fba536846ec727d7a2625fc844649"><code class="notranslate">31d9392</code></a> docs: fix typos (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20118">#20118</a>) (Pixel998)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/c7f861b3f8c1ac961b4cd4f22483798f3324c62b"><code class="notranslate">c7f861b</code></a> docs: Update README (GitHub Actions Bot)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/6b0c08b106aa66f2e9fa484282f0eb63c64a1215"><code class="notranslate">6b0c08b</code></a> docs: Update README (GitHub Actions Bot)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/91f97c50468fbdc089c91e99c2ea0fe821911df2"><code class="notranslate">91f97c5</code></a> docs: Update README (GitHub Actions Bot)</li> </ul> <h2 dir="auto">Chores</h2> <ul dir="auto"> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/12411e8d450ed26a5f7cca6a78ec05323c9323e8"><code class="notranslate">12411e8</code></a> chore: upgrade @eslint/js@9.36.0 (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20139">#20139</a>) (Milos Djermanovic)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/488cba6b391b97b2cfc74bbb46fdeacb1361949e"><code class="notranslate">488cba6</code></a> chore: package.json update for @eslint/js release (Jenkins)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/bac82a2a9c80a3f69087852758d7737aea371f09"><code class="notranslate">bac82a2</code></a> ci: simplify renovate configuration (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/19907">#19907</a>) (唯然)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/c00bb37d62c1bcc0a37f094371be9c40064009f1"><code class="notranslate">c00bb37</code></a> ci: bump actions/labeler from 5 to 6 (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20090">#20090</a>) (dependabot[bot])</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/fee751dc8aeab54547af4538332ea5c069ef28b6"><code class="notranslate">fee751d</code></a> refactor: use <code class="notranslate">defaultOptions</code> in rules (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20121">#20121</a>) (Pixel998)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/1ace67d9f7903adc3d3f09868aa05b673e7d3f3b"><code class="notranslate">1ace67d</code></a> chore: update example to use <code class="notranslate">defineConfig</code> (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20111">#20111</a>) (루밀LuMir)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/4821963bf765532069c49e9da9ecbe9485b073fc"><code class="notranslate">4821963</code></a> test: add missing loc information to error objects in rule tests (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20112">#20112</a>) (루밀LuMir)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/b42c42e7cd3ac9ee1b5a15f16ff25b325d0482e4"><code class="notranslate">b42c42e</code></a> chore: disallow use of deprecated <code class="notranslate">type</code> property in core rule tests (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20094">#20094</a>) (Milos Djermanovic)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/7bb498d720dcd054cc042ca4b60b138d8485f07c"><code class="notranslate">7bb498d</code></a> test: remove deprecated <code class="notranslate">type</code> property from core rule tests (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20093">#20093</a>) (Pixel998)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/e10cf2ab42fe5b481d980dc652f7504414747733"><code class="notranslate">e10cf2a</code></a> ci: bump actions/setup-node from 4 to 5 (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20089">#20089</a>) (dependabot[bot])</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/5cb0ce48ef6cfbbe6d09131c33a53f9d66fe9bd4"><code class="notranslate">5cb0ce4</code></a> refactor: use <code class="notranslate">meta.defaultOptions</code> in <code class="notranslate">preserve-caught-error</code> (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20080">#20080</a>) (Pixel998)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/f9f7cb578dced3c14f635e17c75aa6744d291f4d"><code class="notranslate">f9f7cb5</code></a> chore: package.json update for eslint-config-eslint release (Jenkins)</li> <li> <a href="https://bounce.depfu.com/github.com/eslint/eslint/commit/81764b298065a328038cd067bc8fedef97e57500"><code class="notranslate">81764b2</code></a> chore: update <code class="notranslate">eslint</code> peer dependency in <code class="notranslate">eslint-config-eslint</code> (<a href="https://bounce.depfu.com/github.com/eslint/eslint/pull/20079">#20079</a>) (Milos Djermanovic)</li> </ul></blockquote> <p><em>Does any of this look wrong? <a href="https://depfu.com/packages/npm/eslint/feedback">Please let us know.</a></em></p> </details> <details> <summary>Commits</summary> <p><a href="https://github.com/eslint/eslint/compare/8401101d1e3e3e4e1edc2a9e59cafc9956bf2610...b4857e54e54b5dba96d156cd8d8b4d42dc5a3bf4">See the full diff on Github</a>. The new version differs by 25 commits:</p> <ul> <li><a href="https://github.com/eslint/eslint/commit/b4857e54e54b5dba96d156cd8d8b4d42dc5a3bf4"><code>9.36.0</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/5878a4243f623b46b476eb81043d06244eae5877"><code>Build: changelog update for 9.36.0</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/12411e8d450ed26a5f7cca6a78ec05323c9323e8"><code>chore: upgrade @eslint/js@9.36.0 (#20139)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/488cba6b391b97b2cfc74bbb46fdeacb1361949e"><code>chore: package.json update for @eslint/js release</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/b73ab12acd3e87f8d8173cda03499f6cd1f26db6"><code>docs: update examples to use `defineConfig` (#20131)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/47afcf668df65eac68d7b04145d037037010a076"><code>feat: correct `preserve-caught-error` edge cases (#20109)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/75b74d865d3b8e7fa3bcf5ad29f4bf6d18d1310e"><code>fix: add missing rule option types (#20127)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/bac82a2a9c80a3f69087852758d7737aea371f09"><code>ci: simplify renovate configuration (tailwindlabs#19907)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/1c0d85049e3f30a8809340c1abc881c63b7812ff"><code>fix: update `eslint-all.js` to use `Object.freeze` for `rules` object (#20116)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/c00bb37d62c1bcc0a37f094371be9c40064009f1"><code>ci: bump actions/labeler from 5 to 6 (#20090)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/fee751dc8aeab54547af4538332ea5c069ef28b6"><code>refactor: use `defaultOptions` in rules (#20121)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/31d93926990fba536846ec727d7a2625fc844649"><code>docs: fix typos (#20118)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/7d61b7fadc9c5c6f2b131e37e8a3cffa5aae8ee6"><code>fix: add missing scope types to `Scope.type` (#20110)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/1ace67d9f7903adc3d3f09868aa05b673e7d3f3b"><code>chore: update example to use `defineConfig` (#20111)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/4821963bf765532069c49e9da9ecbe9485b073fc"><code>test: add missing loc information to error objects in rule tests (#20112)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/c7f861b3f8c1ac961b4cd4f22483798f3324c62b"><code>docs: Update README</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/6b0c08b106aa66f2e9fa484282f0eb63c64a1215"><code>docs: Update README</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/7a670c301b58609017ce8cfda99ee81f95de3898"><code>fix: correct rule option typings in `rules.d.ts` (#20084)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/b42c42e7cd3ac9ee1b5a15f16ff25b325d0482e4"><code>chore: disallow use of deprecated `type` property in core rule tests (#20094)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/7bb498d720dcd054cc042ca4b60b138d8485f07c"><code>test: remove deprecated `type` property from core rule tests (#20093)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/91f97c50468fbdc089c91e99c2ea0fe821911df2"><code>docs: Update README</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/e10cf2ab42fe5b481d980dc652f7504414747733"><code>ci: bump actions/setup-node from 4 to 5 (#20089)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/5cb0ce48ef6cfbbe6d09131c33a53f9d66fe9bd4"><code>refactor: use `meta.defaultOptions` in `preserve-caught-error` (tailwindlabs#20080)</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/f9f7cb578dced3c14f635e17c75aa6744d291f4d"><code>chore: package.json update for eslint-config-eslint release</code></a></li> <li><a href="https://github.com/eslint/eslint/commit/81764b298065a328038cd067bc8fedef97e57500"><code>chore: update `eslint` peer dependency in `eslint-config-eslint` (tailwindlabs#20079)</code></a></li> </ul> </details> ---  [Depfu](https://depfu.com) will automatically keep this PR conflict-free, as long as you don't add any commits to this branch yourself. You can also trigger a rebase manually by commenting with `@depfu rebase`. <details><summary>All Depfu comment commands</summary> <blockquote><dl> <dt>@depfu rebase</dt><dd>Rebases against your default branch and redoes this update</dd> <dt>@depfu recreate</dt><dd>Recreates this PR, overwriting any edits that you've made to it</dd> <dt>@depfu merge</dt><dd>Merges this PR once your tests are passing and conflicts are resolved</dd> <dt>@depfu cancel merge</dt><dd>Cancels automatic merging of this PR</dd> <dt>@depfu close</dt><dd>Closes this PR and deletes the branch</dd> <dt>@depfu reopen</dt><dd>Restores the branch and reopens this PR (if it's closed)</dd> <dt>@depfu pause</dt><dd>Ignores all future updates for this dependency and closes this PR</dd> <dt>@depfu pause [minor|major]</dt><dd>Ignores all future minor/major updates for this dependency and closes this PR</dd> <dt>@depfu resume</dt><dd>Future versions of this dependency will create PRs again (leaves this PR as is)</dd> </dl></blockquote> </details> Co-authored-by: depfu[bot] <23717796+depfu[bot]@users.noreply.github.com>
This PR fixes an issue where a
calc(…)value used in a shadow value was incorrectly marked as the color of that shadow.We have this feature where we can have colored shadows, this requires us to replace the color in a value with a
var(--tw-shadow-color, <original-value-here>)such that we can swap out the color.For this, we have to parse the value and figure out what the color part.
This PR uses the
ValueParserto parse values, then figure out what the color part is. The biggest reason for this is that we know what a "function" is and what a normal "word" is, which allows us to do more fine-grained checks on a per-type basis.We tracked a few more functions that we know produce color values, and a few cases that we know produce length values so therefore can't be the color.
Some examples:
calc(…),min(…),max(…),clamp(…),--spacing(…)— these all produce length-values.color(…),color-mix(…),rgba?(…),--alpha(…)… — these all produce color values.Notice that we added
--spacing(…)and--alpha(…)as well, these are custom functions that Tailwind CSS provides, but we know what it will eventually map to internally.Last but not least, we also detect named colors and hex-based colors.
Fixes: #20065
Closes: #20074
Test plan
.drop-shadow-calc { - --tw-drop-shadow-size: drop-shadow(0 0 calc(1 * var(--spacing)) var(--tw-drop-shadow-color, black)); + --tw-drop-shadow-size: drop-shadow(0 0 var(--tw-drop-shadow-color, calc(1 * var(--spacing))) black); --tw-drop-shadow: drop-shadow(var(--drop-shadow-calc)); filter: var(--tw-blur, ) var(--tw-brightness, ) var(--tw-contrast, ) var(--tw-grayscale, ) var(--tw-hue-rotate, ) var(--tw-invert, ) var(--tw-saturate, ) var(--tw-sepia, ) var(--tw-drop-shadow, ); }