fix(fonts): handle nested-brace options objects in self-hosting transform#723
fix(fonts): handle nested-brace options objects in self-hosting transform#723james-elicx merged 2 commits intomainfrom
Conversation
…form
namedCallRe and memberCallRe used \{[^}]*\} which stops at the first
closing brace, so calls like Inter({ axes: { wght: 400 } }) were silently
skipped and fell back to CDN delivery.
Replace both regexes with a lookahead-only pattern plus a
findBalancedObject() helper that tracks brace depth and skips string
literals, correctly extracting the full options object regardless of
nesting depth.
Adds two regression tests: one for the named-import path and one for the
default-import proxy (namespace member) path.
|
/bigbonk review |
commit: |
|
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: LGTM
Clean fix for a real silent-failure bug. The old \{[^}]*\} regex in both namedCallRe and memberCallRe would stop at the first }, so any font call with nested objects (e.g. axes: { wght: 400 }) silently skipped self-hosting. The new approach — lookahead-only regex + findBalancedObject() with depth tracking and string-literal skipping — handles this correctly.
What I verified
- All 62 tests in
font-google.test.tspass, including the two new regression tests findBalancedObjectcorrectly handles: nested braces, single/double/backtick string literals with escape sequences, leading whitespace, and unbalanced input (returns null)- Both the named-import path (
Inter({...})) and namespace-member path (fonts.Inter({...})) are covered - The close-paren scanning after the balanced object correctly tolerates whitespace between
}and) - No risk of infinite loops — the scanner is bounded by
code.length
Minor observations (non-blocking)
-
Comments and regex literals inside options:
findBalancedObjectdoesn't skip//,/* */comments or regex literals containing braces. A brace inside a comment or regex would affect the depth count. This is extremely unlikely in font option objects — noting it only for completeness. -
Template literal
${...}expressions: Backtick strings are handled by skipping until the closing backtick, which means${expr}with braces inside is correctly treated as part of the string. However, nested backticks (tagged templates inside template expressions) could theoretically confuse the scanner. Again, not realistic for font options. -
Code duplication: The close-paren scanning logic (whitespace skip +
)check +callEndcomputation) is identical between thenamedCallReandmemberCallReloops. Could be a small helper, but it's fine as-is for two call sites. -
Nested function allocation:
findBalancedObjectis defined insideif (isBuild)within thetransformhandler, so it's re-created on every transform call during builds. Could be hoisted to module or closure scope. Negligible perf impact since very few files importnext/font/google.
|
Review posted. The PR is approved — it's a clean fix for a real silent-failure bug where nested braces in font options (e.g. |
…dup, hoisting
- Fix _findBalancedObject to correctly skip ${...} interpolations inside
template literals. The previous backtick skipper treated any '}' as the
end of the string, which would prematurely exit for values like
`val ${nested}`. Now tracks an exprDepth counter inside interpolations.
- Extract _findCallEnd(code, objEnd) helper that skips whitespace and
returns the index after ')'. Removes ~20 lines of duplicated logic
between the named-call and member-call branches.
- Hoist both helpers to module level (top-level exported functions) so
they are not recreated on every transform() call.
- Add tests: integration test for string values containing brace chars
(Inter({ label: "font {bold}" })), plus unit describe blocks for
_findBalancedObject and _findCallEnd covering string/template/escape/
interpolation/whitespace/null edge cases.
|
/bigbonk review |
There was a problem hiding this comment.
Review: Approved
Clean, well-scoped fix for a real silent-failure bug. The old \{[^}]*\} regex in both namedCallRe and memberCallRe would terminate at the first }, silently skipping self-hosting for any font call with nested objects (e.g. axes: { wght: 400 }). The fix is sound.
What I verified
- All 77 tests pass (including the new regression tests and the
_findBalancedObject/_findCallEndunit tests) _findBalancedObjectcorrectly handles: nested braces, single/double/backtick string literals with escape sequences,${...}interpolations, leading whitespace, and unbalanced input (returnsnull)- Both code paths are covered: named imports (
Inter({...})) and namespace member calls (fonts.Inter({...})) - The close-paren scanning via
_findCallEndis clean and correctly handles whitespace between}and) - The regex change from capturing the object to a lookahead-only pattern (
(?=\{)) is the right approach — it lets the balanced scanner do the heavy lifting without duplicating logic - Extracting
_findBalancedObjectand_findCallEndas named exported functions (rather than inlining them) makes them independently testable, which the new unit test block takes advantage of
One thing worth noting
The _findBalancedObject function is exported at the top level of index.ts with an underscore-prefix convention for test access. This is consistent with how _parseStaticObjectLiteral, _stripServerExports, etc. are already exported — no issue there.
No concerns. Ship it.
|
I've reviewed and approved PR #723. Here's the summary: The fix is correct. The old The replacement approach — a lookahead-only regex ( |
1 participant
Summary
namedCallReandmemberCallReused\{[^}]*\}which stops at the first}, silently skipping self-hosting for any font call whose options contain a nested object (e.g.Inter({ subsets: ["latin"], axes: { wght: 400 } }))findBalancedObject()helper that tracks brace depth and skips string literals