Improve whitespace handling during canonicalization#19986
Conversation
This time around prettyfying things, not correctness
|
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)
WalkthroughThe arbitrary-value printer now uses a shared 🚥 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/candidate.ts (1)
7-7: Consider using a type-only import forValueFunctionNode.At Line 7,
ValueFunctionNodeis used only as a type (satisfies ValueFunctionNode). While the current configuration does not require it, usingimport typeclarifies intent and is a TypeScript best practice.Suggested change
-import { ValueFunctionNode } from './value-parser' +import type { ValueFunctionNode } from './value-parser'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tailwindcss/src/candidate.ts` at line 7, Change the import of ValueFunctionNode to a type-only import since it is only used as a type (you can see it used with "satisfies ValueFunctionNode"); update the import statement in packages/tailwindcss/src/candidate.ts to use "import type { ValueFunctionNode }" so the compiler and bundlers treat it as a type-only import and clarify intent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/tailwindcss/src/candidate.ts`:
- Line 7: Change the import of ValueFunctionNode to a type-only import since it
is only used as a type (you can see it used with "satisfies ValueFunctionNode");
update the import statement in packages/tailwindcss/src/candidate.ts to use
"import type { ValueFunctionNode }" so the compiler and bundlers treat it as a
type-only import and clarify intent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 67390443-ce28-4561-804f-0251b2bd0ae5
📒 Files selected for processing (2)
packages/tailwindcss/src/candidate.tspackages/tailwindcss/src/canonicalize-candidates.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/tailwindcss/src/candidate.ts (1)
1101-1134: Broaden predecessor detection to cover compact operator/function forms.The current check only inspects
idx - 2, so cases where the symbol is immediately adjacent (no separator) can skip wrapping unexpectedly.♻️ Suggested patch
- let previousPrevious = parentArray[idx - 2] - if (previousPrevious && !symbols.has(previousPrevious.value)) return + let tokenBeforeFunction = + previous?.kind === 'separator' ? parentArray[idx - 2] : previous + if (tokenBeforeFunction && !symbols.has(tokenBeforeFunction.value)) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/tailwindcss/src/candidate.ts` around lines 1101 - 1134, The predecessor detection currently only checks parentArray[idx - 2] (previousPrevious) which misses compact forms where a symbol is adjacent without an intervening separator; replace that single-index check with a backwards scan from idx - 1 to find the nearest non-separator token (skip separators like ',' or other separator nodes) and then test symbols.has(found.value) to decide wrapping; update the logic around node.kind/node.value.startsWith('--') and the use of idx/parentArray so that if no symbol is found you return (no wrap) and if a symbol is found you proceed to the existing WalkAction.ReplaceSkip with the ValueFunctionNode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/tailwindcss/src/candidate.ts`:
- Around line 1101-1134: The predecessor detection currently only checks
parentArray[idx - 2] (previousPrevious) which misses compact forms where a
symbol is adjacent without an intervening separator; replace that single-index
check with a backwards scan from idx - 1 to find the nearest non-separator token
(skip separators like ',' or other separator nodes) and then test
symbols.has(found.value) to decide wrapping; update the logic around
node.kind/node.value.startsWith('--') and the use of idx/parentArray so that if
no symbol is found you return (no wrap) and if a symbol is found you proceed to
the existing WalkAction.ReplaceSkip with the ValueFunctionNode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 1b75fe17-1fd6-4195-a3f8-f65bf750a5df
📒 Files selected for processing (1)
packages/tailwindcss/src/candidate.ts
This PR fixes a printing bug during canonicalization where it converts:
into:
This is because the
_was marked as insignificant and therefore removed. This PR fixes that and maintains the whitespace (_) characters when needed.Additionally, in the comments of the linked issue somebody mentioned that:
was turned into:
...and while that's still correct and parseable, it's not the prettiest.
This PR will still get rid of the whitespace, but introduce wrapping parens
(…)instead, in case readability is not ideal.In this case, we will turn it into:
Of course there are some cases where we don't need to introduce
(…)unnecessarily:shadow-[inset_0px_1px_--theme(--color-white/15%)]would not be turned intoshadow-[inset_0px_1px_(--theme(--color-white/15%))]because no readability is gained when it's part of a normal space separated listm-[--spacing(12.34)]would not be turned intom-[(--spacing(12.34))]because there is nothing else it can conflict withm-[calc(--spacing(12.34)*2)]would not be turned intom-[calc((--spacing(12.34))*2)]because it's the first argument and doesn't conflict with the*m-[min(100%,--spacing(12.34))]would not be turned intom-[min(100%,(--spacing(12.34)))]because a,doesn't cause readability issuesFixes: tailwindlabs/tailwindcss-intellisense#1544
Test plan