Skip to content

Commit 48bc37f

Browse files
skovhusclaude
andcommitted
fix: transient prop rename ordering, promoted merge guard, and event handler scoping
Three regression fixes: 1. Transient prop rename now respects JSX attribute ordering — only $-props appearing AFTER the last spread at every call site are safe to rename, since spreads can override earlier explicit attrs at runtime. 2. __promotedMergeArgs is skipped when matchedCombinedStyleKey is set, preventing runtime crashes from calling a static style object as a function. 3. Event handler type annotations are now only added for polymorphic (as-prop) components where the element type varies at runtime, not all intrinsic components. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent c533518 commit 48bc37f

12 files changed

Lines changed: 304 additions & 106 deletions

src/__tests__/transform.test.ts

Lines changed: 10 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -4678,7 +4678,7 @@ export const App = () => (
46784678
});
46794679

46804680
describe("event handler annotation typing", () => {
4681-
it("annotates onChange handlers with intrinsic element types", () => {
4681+
it("does not annotate event handlers on non-polymorphic intrinsic components", () => {
46824682
const source = `
46834683
import React from "react";
46844684
import styled from "styled-components";
@@ -4709,11 +4709,12 @@ export const App = () => (
47094709
);
47104710

47114711
expect(result.code).not.toBeNull();
4712-
expect(result.code).toContain("React.ChangeEvent<HTMLSelectElement>");
4713-
expect(result.code).toContain("React.ChangeEvent<HTMLInputElement>");
4712+
// Non-polymorphic components: TypeScript infers event types, no annotation needed
4713+
expect(result.code).not.toContain("React.ChangeEvent");
4714+
expect(result.code).toContain("(e) => console.log(e.target.value)");
47144715
});
47154716

4716-
it("wraps unparenthesized arrow params in parens when adding type annotations", () => {
4717+
it("does not add type annotations on non-polymorphic components", () => {
47174718
const source = `
47184719
import React from "react";
47194720
import styled from "styled-components";
@@ -4737,13 +4738,11 @@ export const App = () => (
47374738
);
47384739

47394740
expect(result.code).not.toBeNull();
4740-
// Must have parentheses around the typed parameter — not `e: Type =>`
4741-
expect(result.code).toContain(
4742-
"(e: React.KeyboardEvent<HTMLDivElement>) => e.stopPropagation()",
4743-
);
4744-
expect(result.code).toContain("(e: React.MouseEvent<HTMLDivElement>) => console.log(e)");
4745-
// Must NOT have the broken unparenthesized form
4746-
expect(result.code).not.toContain("{e: React.");
4741+
// Non-polymorphic: no type annotations added
4742+
expect(result.code).not.toContain("React.KeyboardEvent");
4743+
expect(result.code).not.toContain("React.MouseEvent");
4744+
expect(result.code).toContain("e => e.stopPropagation()");
4745+
expect(result.code).toContain("e => console.log(e)");
47474746
});
47484747
});
47494748

src/internal/transform-steps/analyze-before-emit.ts

Lines changed: 167 additions & 56 deletions
Original file line numberDiff line numberDiff line change
@@ -655,18 +655,27 @@ export function analyzeBeforeEmitStep(ctx: TransformContext): StepResult {
655655
}
656656
// Block renames where the stripped name already appears as a JSX attribute at
657657
// a call site (e.g., <Input size={5} $size="lg" /> — renaming $size → size
658-
// would create duplicate attributes). Also block ALL renames when any call site
659-
// uses spread attributes, since the spread may contain $-prefixed keys at runtime.
660-
const callSiteHasSpread = collectCallSiteAttrNames(root, j, decl.localName, existingPropNames);
661-
if (callSiteHasSpread) {
662-
continue;
663-
}
658+
// would create duplicate attributes). When a call site uses spread attributes,
659+
// only allow renames for $-prefixed props that are explicitly passed at ALL
660+
// spread-containing sites (explicit attrs override spread values, so the rename
661+
// is safe).
662+
const { hasSpread, explicitTransientAtSpreadSites } = collectCallSiteAttrNames(
663+
root,
664+
j,
665+
decl.localName,
666+
existingPropNames,
667+
);
664668
const renames = new Map<string, string>();
665669
for (const prop of transientProps) {
666670
const stripped = prop.slice(1);
667-
if (!existingPropNames.has(stripped)) {
668-
renames.set(prop, stripped);
671+
if (existingPropNames.has(stripped)) {
672+
continue;
669673
}
674+
// When spreads exist, only rename props explicitly passed at all spread sites
675+
if (hasSpread && !explicitTransientAtSpreadSites?.has(prop)) {
676+
continue;
677+
}
678+
renames.set(prop, stripped);
670679
}
671680
if (renames.size > 0) {
672681
// Don't rename props when the propsType references a named type (interface
@@ -1055,10 +1064,15 @@ export function analyzeBeforeEmitStep(ctx: TransformContext): StepResult {
10551064
if (decl.promotedStyleProps?.length) {
10561065
for (const entry of decl.promotedStyleProps) {
10571066
if (entry.mergeIntoBase) {
1058-
// Merge static properties into the component's existing style object.
1059-
const existing = ctx.resolvedStyleObjects.get(decl.styleKey);
1060-
if (existing && typeof existing === "object" && !isAstNode(existing)) {
1061-
Object.assign(existing as Record<string, unknown>, entry.styleValue);
1067+
if (isAstNode(entry.styleValue)) {
1068+
// Dynamic merge: replace the base entry with the arrow function.
1069+
ctx.resolvedStyleObjects.set(decl.styleKey, entry.styleValue);
1070+
} else {
1071+
// Static merge: merge properties into the component's existing style object.
1072+
const existing = ctx.resolvedStyleObjects.get(decl.styleKey);
1073+
if (existing && typeof existing === "object" && !isAstNode(existing)) {
1074+
Object.assign(existing as Record<string, unknown>, entry.styleValue);
1075+
}
10621076
}
10631077
} else {
10641078
ctx.resolvedStyleObjects.set(entry.styleKey, entry.styleValue);
@@ -1257,27 +1271,50 @@ function collectResolverImportNames(ctx: TransformContext): Set<string> {
12571271
/**
12581272
* Collects non-`$`-prefixed attribute names from JSX call sites of a component.
12591273
* Returns true if any call site uses a JSX spread attribute (e.g., `{...props}`),
1260-
* which means the spread may contain `$`-prefixed keys at runtime — all renames
1261-
* must be blocked to prevent mismatches.
1274+
* which means the spread may contain `$`-prefixed keys at runtime — renames are
1275+
* only safe for `$`-props that appear **after** the last spread at every call site,
1276+
* since in JSX later attributes override earlier ones.
12621277
*/
1278+
interface CallSiteAttrResult {
1279+
hasSpread: boolean;
1280+
/**
1281+
* `$`-prefixed props explicitly passed at every spread-containing call site.
1282+
* Renames for these are safe even with spreads (explicit attrs override spread values).
1283+
* `null` when no spread sites exist.
1284+
*/
1285+
explicitTransientAtSpreadSites: Set<string> | null;
1286+
}
1287+
12631288
function collectCallSiteAttrNames(
12641289
root: ReturnType<JSCodeshift>,
12651290
j: JSCodeshift,
12661291
componentName: string,
12671292
names: Set<string>,
1268-
): boolean {
1293+
): CallSiteAttrResult {
12691294
let hasSpread = false;
1295+
const spreadSiteTransientProps: Set<string>[] = [];
12701296
const collectFromElement = (openingElement: { attributes?: unknown[] }) => {
1297+
let siteHasSpread = false;
1298+
const siteTransientAfterSpread = new Set<string>();
12711299
for (const attr of (openingElement as any).attributes ?? []) {
12721300
if (attr.type === "JSXSpreadAttribute") {
12731301
hasSpread = true;
1302+
siteHasSpread = true;
1303+
// Any $-props seen BEFORE this spread are overridden by the spread,
1304+
// so clear them — only props AFTER the last spread are safe to rename.
1305+
siteTransientAfterSpread.clear();
12741306
} else if (attr.type === "JSXAttribute" && attr.name?.type === "JSXIdentifier") {
12751307
const name: string = attr.name.name;
1276-
if (!name.startsWith("$")) {
1308+
if (name.startsWith("$")) {
1309+
siteTransientAfterSpread.add(name);
1310+
} else {
12771311
names.add(name);
12781312
}
12791313
}
12801314
}
1315+
if (siteHasSpread) {
1316+
spreadSiteTransientProps.push(siteTransientAfterSpread);
1317+
}
12811318
};
12821319
root
12831320
.find(j.JSXElement, {
@@ -1291,7 +1328,22 @@ function collectCallSiteAttrNames(
12911328
name: { type: "JSXIdentifier", name: componentName },
12921329
} as any)
12931330
.forEach((p: any) => collectFromElement(p.node));
1294-
return hasSpread;
1331+
if (!hasSpread) {
1332+
return { hasSpread: false, explicitTransientAtSpreadSites: null };
1333+
}
1334+
// Intersect: find $-prefixed props that appear at ALL spread-containing sites
1335+
if (spreadSiteTransientProps.length === 0) {
1336+
return { hasSpread: true, explicitTransientAtSpreadSites: new Set() };
1337+
}
1338+
const intersection = new Set(spreadSiteTransientProps[0]);
1339+
for (let i = 1; i < spreadSiteTransientProps.length; i++) {
1340+
for (const prop of intersection) {
1341+
if (!spreadSiteTransientProps[i]!.has(prop)) {
1342+
intersection.delete(prop);
1343+
}
1344+
}
1345+
}
1346+
return { hasSpread: true, explicitTransientAtSpreadSites: intersection };
12951347
}
12961348

12971349
/**
@@ -2246,25 +2298,67 @@ function analyzePromotableStyleProps(
22462298
}
22472299
} else if (hasDynamic) {
22482300
// Mixed static+dynamic or all-dynamic: create a dynamic style function.
2249-
const styleKey = generatePromotedDynamicStyleKey(
2250-
decl.styleKey,
2251-
usedKeyNames,
2252-
site.children,
2253-
);
2254-
usedKeyNames.add(styleKey);
2255-
22562301
// Build static part of the style object and collect dynamic params.
2257-
const staticObj: Record<string, unknown> = {};
2302+
const inlineStaticObj: Record<string, unknown> = {};
22582303
const dynamicParams: Array<{ cssProp: string; expr: unknown }> = [];
22592304

22602305
for (const p of site.properties) {
22612306
if (p.staticValue !== null) {
2262-
staticObj[p.key] = coerceToStringForStyleX(p.key, p.staticValue);
2307+
inlineStaticObj[p.key] = coerceToStringForStyleX(p.key, p.staticValue);
22632308
} else {
22642309
dynamicParams.push({ cssProp: p.key, expr: p.dynamicExpr });
22652310
}
22662311
}
22672312

2313+
// Check if we can merge the base static styles into this dynamic function.
2314+
// This produces a single style entry instead of separate static + dynamic keys.
2315+
// Bail when base values include non-primitive, non-AST objects (e.g., pseudo-selector maps).
2316+
const baseObj = resolvedStyleObjects.get(decl.styleKey);
2317+
const baseIsSimpleObject =
2318+
baseObj &&
2319+
typeof baseObj === "object" &&
2320+
!isAstNode(baseObj) &&
2321+
Object.values(baseObj as Record<string, unknown>).every(
2322+
(v) =>
2323+
typeof v === "string" ||
2324+
typeof v === "number" ||
2325+
typeof v === "boolean" ||
2326+
isAstNode(v),
2327+
);
2328+
// Don't merge if another styled component extends this one — converting
2329+
// the base style to a function would break the child's static style reference.
2330+
const isExtendedByOther = styledDecls.some(
2331+
(d) => d !== decl && d.base.kind === "component" && d.base.ident === decl.localName,
2332+
);
2333+
const canMergeDynamic =
2334+
usageCount <= 1 &&
2335+
!decl.isExported &&
2336+
!isExtendedByOther &&
2337+
baseIsSimpleObject &&
2338+
!hasPropertyOverlap(inlineStaticObj, baseObj as Record<string, unknown>);
2339+
2340+
// Collect all static properties (base + inline) for the merged function body.
2341+
// Dynamic params override base properties with the same key, so filter them out.
2342+
const dynamicPropKeys = new Set(dynamicParams.map((dp) => dp.cssProp));
2343+
const mergedStaticProps: Array<{ key: string; value: unknown }> = [];
2344+
if (canMergeDynamic) {
2345+
for (const [k, v] of Object.entries(baseObj as Record<string, unknown>)) {
2346+
if (!dynamicPropKeys.has(k)) {
2347+
mergedStaticProps.push({ key: k, value: v });
2348+
}
2349+
}
2350+
}
2351+
for (const [k, v] of Object.entries(inlineStaticObj)) {
2352+
mergedStaticProps.push({ key: k, value: v });
2353+
}
2354+
2355+
const styleKey = canMergeDynamic
2356+
? decl.styleKey
2357+
: generatePromotedDynamicStyleKey(decl.styleKey, usedKeyNames, site.children);
2358+
if (!canMergeDynamic) {
2359+
usedKeyNames.add(styleKey);
2360+
}
2361+
22682362
// Build the ArrowFunctionExpression AST node.
22692363
// Use CSS property names as function parameter names for self-documenting code.
22702364
// Deduplicate parameters with the same CSS property name.
@@ -2300,43 +2394,60 @@ function analyzePromotableStyleProps(
23002394
return id;
23012395
});
23022396

2303-
// Build object expression body
2304-
const bodyProperties = site.properties.map((p) => {
2305-
if (p.staticValue !== null) {
2306-
// Static property
2307-
const val =
2308-
typeof p.staticValue === "string"
2309-
? j.stringLiteral(p.staticValue)
2310-
: typeof p.staticValue === "number"
2311-
? j.numericLiteral(p.staticValue)
2312-
: j.booleanLiteral(p.staticValue as boolean);
2313-
return j.property("init", j.identifier(p.key), val);
2314-
} else {
2315-
// Dynamic property — param name matches CSS property for shorthand: { left }
2397+
// Build object expression body with merged base + inline properties
2398+
const bodyProperties: ReturnType<typeof j.property>[] = [];
2399+
for (const sp of mergedStaticProps) {
2400+
const val = isAstNode(sp.value)
2401+
? (sp.value as ExpressionKind) // Already an AST node — use directly
2402+
: typeof sp.value === "string"
2403+
? j.stringLiteral(sp.value)
2404+
: typeof sp.value === "number"
2405+
? j.numericLiteral(sp.value)
2406+
: j.booleanLiteral(sp.value as boolean);
2407+
bodyProperties.push(j.property("init", j.identifier(sp.key), val));
2408+
}
2409+
for (const p of site.properties) {
2410+
if (p.dynamicExpr !== null) {
23162411
const prop = j.property("init", j.identifier(p.key), j.identifier(p.key));
23172412
(prop as any).shorthand = true;
2318-
return prop;
2413+
bodyProperties.push(prop);
23192414
}
2320-
});
2415+
}
23212416

23222417
const fnNode = j.arrowFunctionExpression(params, j.objectExpression(bodyProperties));
23232418

2324-
// Store the AST node directly in resolvedStyleObjects (emitter handles AST nodes).
2325-
promotedEntries.push({
2326-
styleKey,
2327-
styleValue: fnNode as unknown as Record<string, unknown>,
2328-
});
2419+
if (canMergeDynamic) {
2420+
// Replace the base static entry with the merged function.
2421+
promotedEntries.push({
2422+
styleKey: decl.styleKey,
2423+
styleValue: fnNode as unknown as Record<string, unknown>,
2424+
mergeIntoBase: true,
2425+
});
2426+
// Tag JSX: merge consumes the style attr, and the base key becomes a fn call.
2427+
(site.opening as any).__promotedMergeIntoBase = true;
2428+
(site.opening as any).__promotedMergeArgs = dynamicParams.map((dp) =>
2429+
STYLEX_STRING_ONLY_CSS_PROPS.has(dp.cssProp)
2430+
? j.callExpression(j.identifier("String"), [dp.expr as ExpressionKind])
2431+
: dp.expr,
2432+
);
2433+
} else {
2434+
// Store the AST node directly in resolvedStyleObjects (emitter handles AST nodes).
2435+
promotedEntries.push({
2436+
styleKey,
2437+
styleValue: fnNode as unknown as Record<string, unknown>,
2438+
});
23292439

2330-
// Tag the JSX node with the style key and call arguments.
2331-
(site.opening as any).__promotedStyleKey = styleKey;
2332-
// The call args are the actual expressions from the style object.
2333-
// For string-only CSS props (e.g. gridRow), wrap in String() to coerce numeric values.
2334-
const callArgs = dynamicParams.map((dp) =>
2335-
STYLEX_STRING_ONLY_CSS_PROPS.has(dp.cssProp)
2336-
? j.callExpression(j.identifier("String"), [dp.expr as ExpressionKind])
2337-
: dp.expr,
2338-
);
2339-
(site.opening as any).__promotedStyleArgs = callArgs;
2440+
// Tag the JSX node with the style key and call arguments.
2441+
(site.opening as any).__promotedStyleKey = styleKey;
2442+
// The call args are the actual expressions from the style object.
2443+
// For string-only CSS props (e.g. gridRow), wrap in String() to coerce numeric values.
2444+
const callArgs = dynamicParams.map((dp) =>
2445+
STYLEX_STRING_ONLY_CSS_PROPS.has(dp.cssProp)
2446+
? j.callExpression(j.identifier("String"), [dp.expr as ExpressionKind])
2447+
: dp.expr,
2448+
);
2449+
(site.opening as any).__promotedStyleArgs = callArgs;
2450+
}
23402451
}
23412452
}
23422453

src/internal/transform-steps/post-process.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -153,11 +153,14 @@ export function postProcessStep(ctx: TransformContext): StepResult {
153153
.some((p: { node?: { attributes?: unknown[] } }) => hasAsAttr(p.node?.attributes as any));
154154
};
155155

156+
// Only annotate event handlers on polymorphic `as` components where
157+
// the element type (and thus event types) varies at runtime.
158+
// Non-polymorphic components have stable types that TypeScript infers.
156159
const convertedNames = new Set<string>();
157160
const componentTagMap = new Map<string, string>();
158161
for (const decl of styledDecls) {
159162
const isPolymorphic = !!decl.supportsAsProp || hasLocalAsUsage(decl.localName);
160-
if (decl.base.kind === "intrinsic" && !isPolymorphic) {
163+
if (decl.base.kind === "intrinsic" && isPolymorphic) {
161164
convertedNames.add(decl.localName);
162165
componentTagMap.set(decl.localName, decl.base.tagName);
163166
}

0 commit comments

Comments
 (0)