Skip to content

Commit 774fb5c

Browse files
skovhusclaude
andauthored
refactor: replace template-literal input/link emitters with AST builders (#356)
* refactor: replace template-literal input/link emitters with AST builders The input and link wrapper emitters used 16 template-literal branches (8 each, 2×2×2 for allowClassNameProp × allowAsProp × emitTypes) plus ~240 lines of post-processing injection helpers (walkAstBfs, injectForwardedAsHandling, injectExplicitRefForwarding, etc.) that existed only because template literals can't be built incrementally. Replaced with a shared `buildAttrWrapperBody()` function that uses the same AST builder pattern as the other emitters, removing all post-processing helpers except `injectDestructureProps` (still needed by the enum variant emitter). 854 → 665 lines (-189 lines, -22%). https://claude.ai/code/session_01CEoafpHoAaRMVmm1WtcXu1 * refactor: merge two nearly identical branches in SFP emitter The shouldForwardProp emitter had two branches (allowClassNameProp/allowStyleProp vs not) that duplicated ~110 lines of body-building logic. Unified them into a single flow with conditional attrs processing, reducing the file by ~96 lines. https://claude.ai/code/session_01CEoafpHoAaRMVmm1WtcXu1 * refactor: deduplicate shared patterns across codebase - Extract isValidIdentifierName to string-utils.ts (was defined 6 times) - Consolidate resolveThemeAst in value-patterns.ts to reuse resolveThemeFromPropsMember from template-literals.ts - Extract resolveTplBranch and applyResolvedEntries helpers in css-helper-conditional.ts to deduplicate template literal branch handling https://claude.ai/code/session_01CEoafpHoAaRMVmm1WtcXu1 * fix: deduplicate destructure props to prevent duplicate bindings buildDestructurePatternProps now skips destructureProps entries that already exist in baseProps. This prevents duplicate bindings like `const { href, href, ...rest } = props` when pseudo guard props overlap with intrinsic props (e.g. href/target on links, type/readOnly on inputs). https://claude.ai/code/session_01CEoafpHoAaRMVmm1WtcXu1 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent d585982 commit 774fb5c

14 files changed

+626
-876
lines changed
Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,56 @@
1+
import { describe, expect, it } from "vitest";
2+
import jscodeshift from "jscodeshift";
3+
import { buildDestructurePatternProps } from "../internal/emit-wrappers/jsx-builders.js";
4+
import { patternProp } from "../internal/transform-utils.js";
5+
6+
const j = jscodeshift.withParser("tsx");
7+
const pp = (name: string) => patternProp(j, name);
8+
9+
describe("buildDestructurePatternProps", () => {
10+
it("skips destructureProps that already exist in baseProps", () => {
11+
// Simulates the link emitter where baseProps already has "href" and "target"
12+
// and pseudoGuardProps also contains "href" from a guard condition.
13+
const baseProps = [pp("href"), pp("target"), pp("children")];
14+
15+
const result = buildDestructurePatternProps(j, pp, {
16+
baseProps,
17+
destructureProps: ["href", "newProp"],
18+
includeRest: true,
19+
restId: j.identifier("rest"),
20+
});
21+
22+
const names = result.map((p) => {
23+
if (p.type === "RestElement") {
24+
return "...rest";
25+
}
26+
const key = (p as { key?: { name?: string } }).key;
27+
return key?.name ?? "?";
28+
});
29+
30+
// "href" should NOT be duplicated; "newProp" should be added
31+
expect(names).toEqual(["href", "target", "children", "newProp", "...rest"]);
32+
});
33+
34+
it("skips destructureProps that match 'as' prop with AssignmentPattern value", () => {
35+
// The "as" prop in baseProps uses j.property.from() with an AssignmentPattern value
36+
const asProp = j.property.from({
37+
kind: "init",
38+
key: j.identifier("as"),
39+
value: j.assignmentPattern(j.identifier("Component"), j.literal("div")),
40+
shorthand: false,
41+
});
42+
const baseProps = [asProp, pp("className")];
43+
44+
const result = buildDestructurePatternProps(j, pp, {
45+
baseProps,
46+
destructureProps: ["as", "extraProp"],
47+
});
48+
49+
const names = result.map((p) => {
50+
const key = (p as { key?: { name?: string } }).key;
51+
return key?.name ?? "?";
52+
});
53+
54+
expect(names).toEqual(["as", "className", "extraProp"]);
55+
});
56+
});

src/internal/emit-wrappers/emit-intrinsic-should-forward-prop.ts

Lines changed: 49 additions & 145 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import type { StyledDecl } from "../transform-types.js";
88
import { getBridgeClassVar } from "../utilities/bridge-classname.js";
99
import { buildStyleFnConditionExpr } from "../utilities/jscodeshift-utils.js";
10+
import { isValidIdentifierName } from "../utilities/string-utils.js";
1011
import { type ExpressionKind, type InlineStyleProp, type WrapperPropDefaults } from "./types.js";
1112
import { SX_PROP_TYPE_TEXT, type JsxAttr, type StatementKind } from "./wrapper-emitter.js";
1213
import { emitStyleMerging } from "./style-merger.js";
@@ -150,10 +151,9 @@ export function emitShouldForwardPropWrappers(ctx: EmitIntrinsicContext): void {
150151
!!dropPrefixFromFilter &&
151152
(usedAttrs.has("*") ||
152153
[...usedAttrs].some((n) => n.startsWith(dropPrefixFromFilter) && !extraProps.has(n)));
153-
const isValidIdentifier = (name: string): boolean => /^[$A-Z_][0-9A-Z_$]*$/i.test(name);
154154
const knownPrefixProps = dropPrefixFromFilter
155155
? [...extraProps].filter(
156-
(p: string) => p.startsWith(dropPrefixFromFilter) && isValidIdentifier(p),
156+
(p: string) => p.startsWith(dropPrefixFromFilter) && isValidIdentifierName(p),
157157
)
158158
: [];
159159
const knownPrefixPropsSet = new Set(knownPrefixProps);
@@ -183,7 +183,7 @@ export function emitShouldForwardPropWrappers(ctx: EmitIntrinsicContext): void {
183183
const staticVariantPropTypes = buildStaticVariantPropTypes(d);
184184
const lines: string[] = [];
185185
for (const p of extraProps) {
186-
if (!isValidIdentifier(p)) {
186+
if (!isValidIdentifierName(p)) {
187187
continue;
188188
}
189189
const variantType = variantDimByProp.get(p);
@@ -219,7 +219,7 @@ export function emitShouldForwardPropWrappers(ctx: EmitIntrinsicContext): void {
219219
const staticVariantPropTypes = buildStaticVariantPropTypes(d);
220220
const lines: string[] = [];
221221
for (const p of extraProps) {
222-
if (!isValidIdentifier(p) || explicitPropNames.has(p) || compoundWhenKeys.has(p)) {
222+
if (!isValidIdentifierName(p) || explicitPropNames.has(p) || compoundWhenKeys.has(p)) {
223223
continue;
224224
}
225225
const variantType = variantDimByProp.get(p);
@@ -600,7 +600,6 @@ export function emitShouldForwardPropWrappers(ctx: EmitIntrinsicContext): void {
600600
const refId = j.identifier("ref");
601601
const restId = j.identifier("rest");
602602
const forwardedAsId = j.identifier("forwardedAs");
603-
const isVoidTag = tagName === "input";
604603
const { hasAny: hasLocalUsage } = emitter.getJsxCallsites(d.localName);
605604

606605
const shouldIncludeRest = shouldIncludeRestForProps({
@@ -633,124 +632,10 @@ export function emitShouldForwardPropWrappers(ctx: EmitIntrinsicContext): void {
633632
(d.supportsExternalStyles ?? false) ||
634633
(!shouldOmitRestSpread && shouldIncludeRest);
635634

636-
if (!allowClassNameProp && !allowStyleProp) {
637-
const isVoid = VOID_TAGS.has(tagName);
638-
// When allowAsProp is true, include children support even for void tags
639-
// because the user might use `as="textarea"` which requires children
640-
const includeChildrenInner = allowAsProp || !isVoid;
641-
const patternProps = emitter.buildDestructurePatternProps({
642-
baseProps: [
643-
...(allowAsProp ? [asDestructureProp(tagName)] : []),
644-
...(includesForwardedAs ? [ctx.patternProp("forwardedAs", forwardedAsId)] : []),
645-
...(includeChildrenInner ? [ctx.patternProp("children", childrenId)] : []),
646-
...((d.supportsRefProp ?? false) ? [ctx.patternProp("ref", refId)] : []),
647-
],
648-
destructureProps: destructureParts,
649-
propDefaults,
650-
includeRest,
651-
restId,
652-
});
653-
const declStmt = j.variableDeclaration("const", [
654-
j.variableDeclarator(j.objectPattern(patternProps as any), propsId),
655-
]);
656-
657-
// Generate cleanup loop for prefix props when:
658-
// - There's a dropPrefix (like "$" for transient props)
659-
// - Either: local usage of unknown prefix props, OR exported/extended component
660-
// (external callers or extending wrappers may pass unknown prefix props)
661-
// - Rest spread is included
662-
const needsCleanupLoop =
663-
dropPrefix &&
664-
(isExportedComponent || (d.supportsExternalStyles ?? false) || shouldAllowAnyPrefixProps) &&
665-
includeRest;
666-
const cleanupPrefixStmt = needsCleanupLoop
667-
? buildPrefixCleanupStatements(j, restId, dropPrefix)
668-
: null;
669-
670-
const { attrsInfo, staticClassNameExpr } = emitter.splitAttrsInfo(
671-
d.attrsInfo,
672-
getBridgeClassVar(d),
673-
);
674-
const { attrsInfo: attrsInfoRaw, forwardedAsStaticFallback } = splitForwardedAsStaticAttrs({
675-
attrsInfo,
676-
includeForwardedAs: includesForwardedAs,
677-
});
678-
const attrsInfoWithoutForwardedAsStatic = filterAttrsForShouldForwardProp(
679-
attrsInfoRaw,
680-
d.shouldForwardProp,
681-
);
682-
const merging = emitStyleMerging({
683-
j,
684-
emitter,
685-
styleArgs,
686-
classNameId,
687-
styleId,
688-
allowClassNameProp,
689-
allowStyleProp,
690-
inlineStyleProps: (d.inlineStyleProps ?? []) as InlineStyleProp[],
691-
staticClassNameExpr,
692-
isIntrinsicElement: !allowAsProp,
693-
});
694-
695-
const openingAttrs: JsxAttr[] = [
696-
...emitter.buildAttrsFromAttrsInfo({
697-
attrsInfo: attrsInfoWithoutForwardedAsStatic,
698-
propExprFor: (prop) => j.identifier(prop),
699-
}),
700-
...((d.supportsRefProp ?? false)
701-
? [j.jsxAttribute(j.jsxIdentifier("ref"), j.jsxExpressionContainer(refId))]
702-
: []),
703-
...(includeRest ? [j.jsxSpreadAttribute(restId)] : []),
704-
...(includesForwardedAs
705-
? [
706-
j.jsxAttribute(
707-
j.jsxIdentifier("as"),
708-
j.jsxExpressionContainer(
709-
buildForwardedAsValueExpr(forwardedAsId, forwardedAsStaticFallback),
710-
),
711-
),
712-
]
713-
: []),
714-
];
715-
emitter.appendMergingAttrs(openingAttrs, merging);
716-
717-
const jsx = emitter.buildJsxElement({
718-
tagName: allowAsProp ? "Component" : tagName,
719-
attrs: openingAttrs,
720-
includeChildren: includeChildrenInner,
721-
childrenExpr: childrenId,
722-
});
723-
724-
const fnBodyStmts: StatementKind[] = [declStmt];
725-
if (cleanupPrefixStmt) {
726-
fnBodyStmts.push(...cleanupPrefixStmt);
727-
}
728-
if (needsUseTheme) {
729-
fnBodyStmts.push(buildUseThemeDeclaration(j, emitter.themeHook.functionName));
730-
}
731-
if (merging.sxDecl) {
732-
fnBodyStmts.push(merging.sxDecl);
733-
}
734-
fnBodyStmts.push(j.returnStatement(jsx as any));
735-
736-
emitted.push(
737-
withLeadingComments(
738-
emitter.buildWrapperFunction({
739-
localName: d.localName,
740-
params: [propsParamId],
741-
bodyStmts: fnBodyStmts,
742-
typeParameters:
743-
allowAsProp && emitTypes ? buildPolymorphicTypeParams(j, tagName) : undefined,
744-
}),
745-
d,
746-
),
747-
);
748-
continue;
749-
}
750-
751635
// When allowAsProp is true, include children support even for void tags
752636
// because the user might use `as="textarea"` which requires children
753-
const includeChildrenOuter = allowAsProp || !isVoidTag;
637+
const isVoid = VOID_TAGS.has(tagName);
638+
const includeChildren = allowAsProp || !isVoid;
754639
const sxId = j.identifier("sx");
755640
if (allowSxProp) {
756641
styleArgs.push(sxId);
@@ -761,7 +646,7 @@ export function emitShouldForwardPropWrappers(ctx: EmitIntrinsicContext): void {
761646
...(allowAsProp ? [asDestructureProp(tagName)] : []),
762647
...(includesForwardedAs ? [ctx.patternProp("forwardedAs", forwardedAsId)] : []),
763648
...(allowClassNameProp ? [ctx.patternProp("className", classNameId)] : []),
764-
...(includeChildrenOuter ? [ctx.patternProp("children", childrenId)] : []),
649+
...(includeChildren ? [ctx.patternProp("children", childrenId)] : []),
765650
...(allowStyleProp ? [ctx.patternProp("style", styleId)] : []),
766651
...((d.supportsRefProp ?? false) ? [ctx.patternProp("ref", refId)] : []),
767652
...(allowSxProp ? [ctx.patternProp("sx", sxId)] : []),
@@ -781,18 +666,32 @@ export function emitShouldForwardPropWrappers(ctx: EmitIntrinsicContext): void {
781666
// - Either: local usage of unknown prefix props, OR exported/extended component
782667
// (external callers or extending wrappers may pass unknown prefix props)
783668
// - Rest spread is included
784-
const needsCleanupLoopOuter =
669+
const needsCleanupLoop =
785670
dropPrefix &&
786671
(isExportedComponent || (d.supportsExternalStyles ?? false) || shouldAllowAnyPrefixProps) &&
787672
includeRest;
788-
const cleanupPrefixStmt = needsCleanupLoopOuter
673+
const cleanupPrefixStmt = needsCleanupLoop
789674
? buildPrefixCleanupStatements(j, restId, dropPrefix)
790675
: null;
791676

792-
// Extract static className and bridge class for the style merger
793-
const { staticClassNameExpr } = emitter.splitAttrsInfo(d.attrsInfo, getBridgeClassVar(d));
677+
const { attrsInfo, staticClassNameExpr } = emitter.splitAttrsInfo(
678+
d.attrsInfo,
679+
getBridgeClassVar(d),
680+
);
681+
682+
// When no className/style props, process attrs for JSX rendering and extract forwardedAs fallback
683+
let attrsInfoForJsx: StyledDecl["attrsInfo"] | undefined;
684+
let forwardedAsStaticFallback: unknown;
685+
if (!allowClassNameProp && !allowStyleProp) {
686+
const { attrsInfo: attrsInfoRaw, forwardedAsStaticFallback: fallback } =
687+
splitForwardedAsStaticAttrs({
688+
attrsInfo,
689+
includeForwardedAs: includesForwardedAs,
690+
});
691+
attrsInfoForJsx = filterAttrsForShouldForwardProp(attrsInfoRaw, d.shouldForwardProp);
692+
forwardedAsStaticFallback = fallback;
693+
}
794694

795-
// Use the style merger helper
796695
const merging = emitStyleMerging({
797696
j,
798697
emitter,
@@ -807,28 +706,34 @@ export function emitShouldForwardPropWrappers(ctx: EmitIntrinsicContext): void {
807706
isIntrinsicElement: !allowAsProp,
808707
});
809708

810-
// Build attrs: {...rest} then {...mergedStylexProps(...)} so stylex styles override
811-
const openingAttrs: JsxAttr[] = [];
812-
if (d.supportsRefProp ?? false) {
813-
openingAttrs.push(j.jsxAttribute(j.jsxIdentifier("ref"), j.jsxExpressionContainer(refId)));
814-
}
815-
if (includeRest) {
816-
openingAttrs.push(j.jsxSpreadAttribute(restId));
817-
}
818-
if (includesForwardedAs) {
819-
openingAttrs.push(
820-
j.jsxAttribute(
821-
j.jsxIdentifier("as"),
822-
j.jsxExpressionContainer(buildForwardedAsValueExpr(forwardedAsId)),
823-
),
824-
);
825-
}
709+
const openingAttrs: JsxAttr[] = [
710+
...(attrsInfoForJsx
711+
? emitter.buildAttrsFromAttrsInfo({
712+
attrsInfo: attrsInfoForJsx,
713+
propExprFor: (prop) => j.identifier(prop),
714+
})
715+
: []),
716+
...((d.supportsRefProp ?? false)
717+
? [j.jsxAttribute(j.jsxIdentifier("ref"), j.jsxExpressionContainer(refId))]
718+
: []),
719+
...(includeRest ? [j.jsxSpreadAttribute(restId)] : []),
720+
...(includesForwardedAs
721+
? [
722+
j.jsxAttribute(
723+
j.jsxIdentifier("as"),
724+
j.jsxExpressionContainer(
725+
buildForwardedAsValueExpr(forwardedAsId, forwardedAsStaticFallback),
726+
),
727+
),
728+
]
729+
: []),
730+
];
826731
emitter.appendMergingAttrs(openingAttrs, merging);
827732

828733
const jsx = emitter.buildJsxElement({
829734
tagName: allowAsProp ? "Component" : tagName,
830735
attrs: openingAttrs,
831-
includeChildren: includeChildrenOuter,
736+
includeChildren,
832737
childrenExpr: childrenId,
833738
});
834739

@@ -844,7 +749,6 @@ export function emitShouldForwardPropWrappers(ctx: EmitIntrinsicContext): void {
844749
}
845750
fnBodyStmts.push(j.returnStatement(jsx as any));
846751

847-
// Add type parameters when allowAsProp is true
848752
emitted.push(
849753
withLeadingComments(
850754
emitter.buildWrapperFunction({

src/internal/emit-wrappers/emit-intrinsic-simple.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
import type { JSCodeshift } from "jscodeshift";
88
import type { StyledDecl } from "../transform-types.js";
99
import { getBridgeClassVar } from "../utilities/bridge-classname.js";
10+
import { isValidIdentifierName } from "../utilities/string-utils.js";
1011
import {
1112
collectInlineStylePropNames,
1213
type ExpressionKind,
@@ -650,11 +651,10 @@ export function emitSimpleExportedIntrinsicWrappers(ctx: EmitIntrinsicContext):
650651
for (const k of compoundVariantWhenKeys) {
651652
keys.delete(k);
652653
}
653-
const isValidIdentifier = (name: string): boolean => /^[$A-Z_][0-9A-Z_$]*$/i.test(name);
654654
const filtered = [...keys].filter(
655655
(k) =>
656656
k &&
657-
isValidIdentifier(k) &&
657+
isValidIdentifierName(k) &&
658658
k !== "children" &&
659659
k !== "className" &&
660660
k !== "style" &&

0 commit comments

Comments
 (0)