feat(fontless): custom prefix support for processCSSVariables#733
feat(fontless): custom prefix support for processCSSVariables#733PatrikBird wants to merge 4 commits intounjs:mainfrom
Conversation
✅ Deploy Preview for fontless ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds support for a custom string prefix for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan for PR comments
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/fontless/src/utils.ts (1)
186-196:⚠️ Potential issue | 🟡 MinorEdge case: empty string prefix behaves like
truemode.When
processCSSVariablesis set to an empty string'', the condition!node.property.startsWith('--')will match all CSS variables (same astruemode). If this is intentional, consider documenting it; otherwise, guard against empty strings.🛡️ Optional guard against empty string prefix
|| (typeof options.processCSSVariables === 'string' && options.processCSSVariables !== 'font-prefixed-only' + && options.processCSSVariables !== '' && !node.property.startsWith(`--${options.processCSSVariables}`))))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fontless/src/utils.ts` around lines 186 - 196, The conditional in the block guarding CSS variable processing treats an empty string value for options.processCSSVariables like true because `--${options.processCSSVariables}` becomes `--`; update the compound condition (the checks referencing node.property, options.processCSSVariables and this.atrule?.name) to explicitly reject empty-string prefixes by requiring string-mode to be non-empty (e.g. change the typeof branch to `typeof options.processCSSVariables === 'string' && options.processCSSVariables.length > 0 && options.processCSSVariables !== 'font-prefixed-only' && !node.property.startsWith(\`--${options.processCSSVariables}\`)` or add `&& options.processCSSVariables !== ''`), so empty string is not treated as the `true`/`--` mode.
🧹 Nitpick comments (2)
packages/fontless/test/parse.spec.ts (1)
270-302: Good test coverage for core scenarios.The tests cover the essential cases: matching prefix, non-matching prefix, and ensuring
font-family/fontproperties are always processed regardless of prefix.Consider adding edge case tests for robustness:
- Empty string prefix behavior
- Prefix matching is case-sensitive (e.g.,
'My-App'vs'my-app')- Prefix that is a substring of another variable name (e.g., prefix
'app'should match--app-fontbut not--my-app-font)📝 Example additional test for substring matching
it('should only match variables starting with the exact prefix', async () => { const result = await transformWithPrefix( `:root { --app-font: 'CustomFont'; --my-app-font: 'OtherFont' }`, 'app' ) expect(result).toContain(`src: url("/customfont.woff2")`) expect(result).not.toContain(`src: url("/otherfont.woff2")`) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fontless/test/parse.spec.ts` around lines 270 - 302, Add edge-case tests to parse.spec.ts covering empty string prefix, case-sensitivity, and substring-prefix avoidance by extending the existing transformWithPrefix usage: add a test that calls transformWithPrefix with '' (empty) to assert expected behavior (either processes all --font-* or none per implementation), add a test that passes a differently cased prefix (e.g., 'My-App' vs 'my-app') to assert case-sensitive matching, and add the substring test shown in the review (call transformWithPrefix with prefix 'app' and assert it matches --app-font but not --my-app-font); place them alongside the other it(...) cases so they exercise the same parsing logic and assertions around presence/absence of `@font-face` and specific src URLs.packages/fontless/src/types.ts (1)
183-188: Remove the deprecatedexperimental.processCSSVariablesfield—it's dead code.This nested field is never referenced in the codebase; only the top-level
processCSSVariablesoption (line 170) is actively used. Since it's already marked deprecated with a clear migration path to the top-level option, removing it simplifies the type definition without any loss of functionality.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fontless/src/types.ts` around lines 183 - 188, Remove the deprecated nested field experimental.processCSSVariables from the type definitions: locate the property named processCSSVariables that is documented as deprecated (the "experimental.processCSSVariables" nested field in the types.ts snippet) and delete that property and its JSDoc comment block so only the top-level processCSSVariables option remains; ensure no other references to experimental.processCSSVariables exist in the same type/interface and update any exported types or comments accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/fontless/src/utils.ts`:
- Around line 186-196: The conditional in the block guarding CSS variable
processing treats an empty string value for options.processCSSVariables like
true because `--${options.processCSSVariables}` becomes `--`; update the
compound condition (the checks referencing node.property,
options.processCSSVariables and this.atrule?.name) to explicitly reject
empty-string prefixes by requiring string-mode to be non-empty (e.g. change the
typeof branch to `typeof options.processCSSVariables === 'string' &&
options.processCSSVariables.length > 0 && options.processCSSVariables !==
'font-prefixed-only' &&
!node.property.startsWith(\`--${options.processCSSVariables}\`)` or add `&&
options.processCSSVariables !== ''`), so empty string is not treated as the
`true`/`--` mode.
---
Nitpick comments:
In `@packages/fontless/src/types.ts`:
- Around line 183-188: Remove the deprecated nested field
experimental.processCSSVariables from the type definitions: locate the property
named processCSSVariables that is documented as deprecated (the
"experimental.processCSSVariables" nested field in the types.ts snippet) and
delete that property and its JSDoc comment block so only the top-level
processCSSVariables option remains; ensure no other references to
experimental.processCSSVariables exist in the same type/interface and update any
exported types or comments accordingly.
In `@packages/fontless/test/parse.spec.ts`:
- Around line 270-302: Add edge-case tests to parse.spec.ts covering empty
string prefix, case-sensitivity, and substring-prefix avoidance by extending the
existing transformWithPrefix usage: add a test that calls transformWithPrefix
with '' (empty) to assert expected behavior (either processes all --font-* or
none per implementation), add a test that passes a differently cased prefix
(e.g., 'My-App' vs 'my-app') to assert case-sensitive matching, and add the
substring test shown in the review (call transformWithPrefix with prefix 'app'
and assert it matches --app-font but not --my-app-font); place them alongside
the other it(...) cases so they exercise the same parsing logic and assertions
around presence/absence of `@font-face` and specific src URLs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 9a8abe67-ff0b-4c64-9df8-68bc66c13acf
📒 Files selected for processing (3)
packages/fontless/src/types.tspackages/fontless/src/utils.tspackages/fontless/test/parse.spec.ts
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #733 +/- ##
==========================================
+ Coverage 74.82% 75.13% +0.31%
==========================================
Files 13 13
Lines 556 563 +7
Branches 190 190
==========================================
+ Hits 416 423 +7
Misses 140 140 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
gioboa
left a comment
gioboa
left a comment
There was a problem hiding this comment.
Thanks for your help @PatrikBird
Can you double check the code rabbit suggestions?
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/fontless/src/utils.ts (1)
186-195: Logic is correct; note on prefix matching behavior.The implementation correctly handles:
- Empty string as disabled (
''short-circuits at line 189)- Custom prefix exclusion of
'font-prefixed-only'to avoid conflicts- Template literal construction
--${prefix}One edge case to consider: the prefix check
startsWith(--${options.processCSSVariables})matches any variable beginning with--<prefix>, including variables without a trailing delimiter. For example, prefix'my-app'would match both--my-app-fooand--my-appbar.If the intent per issue
#732is strictly--my-app-*patterns (with trailing hyphen), users should pass'my-app-'as the prefix. This behavior may be worth documenting if not already covered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fontless/src/utils.ts` around lines 186 - 195, The current variable-prefix check on node.property using startsWith(`--${options.processCSSVariables}`) will match prefixes without a delimiter (e.g., `--my-appbar`); if the intent is to only match dash-delimited namespaces like `--my-app-foo` change the condition in the CSS variable branch (the part referencing options.processCSSVariables and node.property) to require a trailing hyphen when options.processCSSVariables is a custom string (i.e., use startsWith(`--${options.processCSSVariables}-`) for that branch), and update any associated tests/docs to reflect that users should not need to include the trailing dash in the special case only if you choose to keep the previous behavior as-is.
🤖 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/fontless/src/utils.ts`:
- Around line 186-195: The current variable-prefix check on node.property using
startsWith(`--${options.processCSSVariables}`) will match prefixes without a
delimiter (e.g., `--my-appbar`); if the intent is to only match dash-delimited
namespaces like `--my-app-foo` change the condition in the CSS variable branch
(the part referencing options.processCSSVariables and node.property) to require
a trailing hyphen when options.processCSSVariables is a custom string (i.e., use
startsWith(`--${options.processCSSVariables}-`) for that branch), and update any
associated tests/docs to reflect that users should not need to include the
trailing dash in the special case only if you choose to keep the previous
behavior as-is.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e1795c0f-28cc-45ac-8b7d-f228958c3695
📒 Files selected for processing (3)
packages/fontless/src/types.tspackages/fontless/src/utils.tspackages/fontless/test/parse.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/fontless/src/types.ts
- packages/fontless/test/parse.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/fontless/test/parse.spec.ts (2)
304-307: Also verify''does not disable regular font parsing.This only proves that custom properties are skipped. Adding a
font-familyorfontassertion here would guard against''accidentally behaving like a full opt-out instead of “no CSS variable processing”.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fontless/test/parse.spec.ts` around lines 304 - 307, Update the test in parse.spec.ts so it also verifies regular font parsing still runs when prefix is '' by including a standard font declaration (e.g., add a rule with font-family or font: using the same CustomFont) in the input to transformWithPrefix and then assert the output contains the generated `@font-face` (or the expected font-family output). Reference the existing test and function transformWithPrefix: keep the custom-property check (expect(...).not.toContain('@font-face')) but add an assertion like expect(result).toContain('@font-face') or expect(result).toContain("font-family: 'CustomFont'") to ensure '' only disables CSS variable processing and does not opt out of normal font parsing.
297-302: Add a direct regression test for'font-prefixed-only'.This case only validates the new
"font"prefix path. Since the public API still promises the legacy'font-prefixed-only'behavior, it should be asserted separately so a runtime regression in the string handling can’t slip through.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fontless/test/parse.spec.ts` around lines 297 - 302, Add a regression test asserting legacy prefix behavior: create a new test in parse.spec.ts that mirrors the existing 'should process --font-* variables only when prefix is "font"' case but calls transformWithPrefix with the legacy prefix string "font-prefixed-only" and asserts that the output contains the `@font-face` and the expected src for /customfont.woff2 and does not contain /otherfont.woff2; use the same input CSS and expectations as the existing test but pass "font-prefixed-only" to transformWithPrefix to ensure the legacy path still works (refer to transformWithPrefix and the existing test name to locate where to add it).packages/fontless/src/utils.ts (1)
186-196: Logic is correct; consider extracting for readability.The condition correctly handles all cases:
false,'',true,'font-prefixed-only', and custom string prefixes. The trailing dash in--${prefix}-appropriately prevents prefix collisions (e.g., prefix'my'won't match--my-app-foo).However, this 10-line nested boolean expression is difficult to parse. Extracting into a helper function would improve maintainability.
♻️ Suggested refactor to improve readability
+function shouldSkipDeclaration( + property: string, + processCSSVariables: FontFamilyInjectionPluginOptions['processCSSVariables'], + isInFontFace: boolean, +): boolean { + if (isInFontFace) return true + if (property === 'font-family' || property === 'font') return false + + if (processCSSVariables === false || processCSSVariables === '') return true + if (processCSSVariables === true) return !property.startsWith('--') + if (processCSSVariables === 'font-prefixed-only') return !property.startsWith('--font') + if (typeof processCSSVariables === 'string') { + return !property.startsWith(`--${processCSSVariables}-`) + } + return true +} + // Then in processNode: - if (( - (node.property !== 'font-family' && node.property !== 'font') - && (options.processCSSVariables === false - || options.processCSSVariables === '' - || (options.processCSSVariables === 'font-prefixed-only' && !node.property.startsWith('--font')) - || (options.processCSSVariables === true && !node.property.startsWith('--')) - || (typeof options.processCSSVariables === 'string' - && options.processCSSVariables !== 'font-prefixed-only' - && !node.property.startsWith(`--${options.processCSSVariables}-`)))) - || this.atrule?.name === 'font-face') { + if (shouldSkipDeclaration(node.property, options.processCSSVariables, this.atrule?.name === 'font-face')) { return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/fontless/src/utils.ts` around lines 186 - 196, The long nested condition around node.property, options.processCSSVariables and this.atrule?.name === 'font-face' should be extracted into a small helper to improve readability and maintainability; create a function (e.g., shouldProcessCSSVariable or isProcessCSSVariableEnabled) that takes node.property and options.processCSSVariables (and optionally this.atrule) and returns the boolean result currently computed, then replace the inline expression with a call to that helper from the existing location so the logic is unchanged but easier to read and test.
🤖 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/fontless/src/utils.ts`:
- Around line 186-196: The long nested condition around node.property,
options.processCSSVariables and this.atrule?.name === 'font-face' should be
extracted into a small helper to improve readability and maintainability; create
a function (e.g., shouldProcessCSSVariable or isProcessCSSVariableEnabled) that
takes node.property and options.processCSSVariables (and optionally this.atrule)
and returns the boolean result currently computed, then replace the inline
expression with a call to that helper from the existing location so the logic is
unchanged but easier to read and test.
In `@packages/fontless/test/parse.spec.ts`:
- Around line 304-307: Update the test in parse.spec.ts so it also verifies
regular font parsing still runs when prefix is '' by including a standard font
declaration (e.g., add a rule with font-family or font: using the same
CustomFont) in the input to transformWithPrefix and then assert the output
contains the generated `@font-face` (or the expected font-family output).
Reference the existing test and function transformWithPrefix: keep the
custom-property check (expect(...).not.toContain('@font-face')) but add an
assertion like expect(result).toContain('@font-face') or
expect(result).toContain("font-family: 'CustomFont'") to ensure '' only disables
CSS variable processing and does not opt out of normal font parsing.
- Around line 297-302: Add a regression test asserting legacy prefix behavior:
create a new test in parse.spec.ts that mirrors the existing 'should process
--font-* variables only when prefix is "font"' case but calls
transformWithPrefix with the legacy prefix string "font-prefixed-only" and
asserts that the output contains the `@font-face` and the expected src for
/customfont.woff2 and does not contain /otherfont.woff2; use the same input CSS
and expectations as the existing test but pass "font-prefixed-only" to
transformWithPrefix to ensure the legacy path still works (refer to
transformWithPrefix and the existing test name to locate where to add it).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f9b4067b-4f74-415c-95bc-9fe596c1ba20
📒 Files selected for processing (3)
packages/fontless/src/types.tspackages/fontless/src/utils.tspackages/fontless/test/parse.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/fontless/src/types.ts
|
Hey @gioboa |
gioboa
left a comment
gioboa
left a comment
There was a problem hiding this comment.
coderabbit is a bit picky, but usually stops giving suggestions after the necessary corrections. 🤣
|
In other words, we are pleasing the rabbit until it's satisfied? I am on it... 😆 |
Replace nested guard condition in processNode walker with a pure helper function that uses early returns for readability.
Add regression test for the literal "font-prefixed-only" string value and strengthen the empty prefix test to verify font-family still works.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/fontless/src/utils.ts`:
- Around line 73-89: The function shouldSkipDeclaration mishandles an undefined
processCSSVariables (it falls through to the final startsWith check and produces
`--undefined-`); update shouldSkipDeclaration to defensively handle
undefined/null for processCSSVariables (e.g., when processCSSVariables is
undefined or null return true/skip processing) before the other checks so the
subsequent string-check branches never construct `--undefined-`; this change
should be applied inside shouldSkipDeclaration and reference the
processCSSVariables parameter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5965662c-f16a-450e-b2e2-da50fd5701a1
📒 Files selected for processing (2)
packages/fontless/src/utils.tspackages/fontless/test/parse.spec.ts
…aration When processCSSVariables was undefined it fell through to the custom prefix branch and matched against the literal string "--undefined-". Use a falsy check so undefined/null are treated the same as false.
|
Now the bunny is giving me a false positive I think... the remark regarding the Edit: Ah, it says |
gioboa
left a comment
gioboa
left a comment
There was a problem hiding this comment.
LGTM 🚀 thanks @PatrikBird
2 participants
Hello 👋
The issue explains the details. Thanks for creating such lovely tools.
Resolves #732
Summary by CodeRabbit
New Features
Tests