Skip to content

fix: avoid ambiguous migration class rewrites #11

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Naloam merged 1 commit into from
Apr 2, 2026
Merged

fix: avoid ambiguous migration class rewrites #11

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@ Phase 1 and Phase 2 are delivered. Phase 3 ecosystem work is now underway with m
- Dynamic variable references (e.g., `className={someVar}`) and complex member expressions are still left untouched.
- Wrapper calls can be rewritten when their nested arguments are supported class expressions.
- Existing CSS Modules imports are reused, and duplicate side-effect style imports are removed during migration.
- Ambiguous class names coming from multiple imported style modules are left untouched instead of being guessed.
- Spread operators and deeply nested expressions may not be fully rewritten.
- Only `.css` and `.scss` source files are processed.

Expand Down
1 change: 1 addition & 0 deletions packages/cli/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,3 +56,4 @@ Suggests CSS Modules migration for a component directory. Pass `--apply` to exec
- Dynamic variable references and complex member expressions are left untouched.
- Wrapper calls can still be rewritten when their nested arguments match supported class patterns.
- Existing CSS Modules imports are reused, and duplicate side-effect style imports are removed during migration.
- Ambiguous class names coming from multiple imported style modules are left untouched instead of being guessed.
2 changes: 1 addition & 1 deletion packages/core/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,4 +32,4 @@ Supported: static `class`, object `:class`, array `:class`, mixed static + dynam

### Limitations

This is a targeted migration helper, not a general AST transformer. Dynamic variable references and complex member expressions are left untouched. Wrapper calls are only rewritten when their nested arguments match supported class patterns.
This is a targeted migration helper, not a general AST transformer. Dynamic variable references and complex member expressions are left untouched. Wrapper calls are only rewritten when their nested arguments match supported class patterns, and ambiguous class names across multiple imported style modules are intentionally left unchanged.
48 changes: 36 additions & 12 deletions packages/core/src/migrate/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -218,6 +218,28 @@ function toVueStyleAccess(moduleAccessor: string, className: string): string {
return `${moduleAccessor}["${className}"]`;
}

function registerClassExpression(
classToExpr: Map<string, string>,
ambiguousClasses: Set<string>,
className: string,
expression: string,
): void {
if (ambiguousClasses.has(className)) {
return;
}

const existingExpression = classToExpr.get(className);
if (!existingExpression) {
classToExpr.set(className, expression);
return;
}

if (existingExpression !== expression) {
classToExpr.delete(className);
ambiguousClasses.add(className);
}
}

function removeSideEffectImport(content: string, importPath: string): string {
const escapedPath = escapeRegExp(importPath);
const sideEffectImportPattern = new RegExp(
Expand Down Expand Up @@ -1787,7 +1809,9 @@ export async function applyMigrationSuggestions(
let content = await readFile(sourceFile, "utf8");
const before = content;
const classToExpr = new Map<string, string>();
const ambiguousReactClasses = new Set<string>();
const vueClassToExpr = new Map<string, string>();
const ambiguousVueClasses = new Set<string>();

for (const suggestion of suggestions) {
const sourceDir = dirname(sourceFile);
Expand All @@ -1803,12 +1827,12 @@ export async function applyMigrationSuggestions(
content = ensured.content;
if (ensured.alias) {
for (const className of suggestion.classNames) {
if (!classToExpr.has(className)) {
classToExpr.set(
className,
toStyleAccess(ensured.alias, className),
);
}
registerClassExpression(
classToExpr,
ambiguousReactClasses,
className,
toStyleAccess(ensured.alias, className),
);
}
}
}
Expand All @@ -1820,12 +1844,12 @@ export async function applyMigrationSuggestions(
const moduleAccessor =
getVueModuleAccessor(content, newImportPath) ?? "$style";
for (const className of suggestion.classNames) {
if (!vueClassToExpr.has(className)) {
vueClassToExpr.set(
className,
toVueStyleAccess(moduleAccessor, className),
);
}
registerClassExpression(
vueClassToExpr,
ambiguousVueClasses,
className,
toVueStyleAccess(moduleAccessor, className),
);
}
}
}
Expand Down
97 changes: 97 additions & 0 deletions packages/core/tests/migrate/suggestions.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -304,6 +304,51 @@ describe("migrate helpers", () => {
}
});

it("should leave ambiguous react class names untouched across multiple module imports", async () => {
const root = await mkdtemp(
resolve(tmpdir(), "recss-core-migrate-react-ambiguous-"),
);

try {
await mkdir(resolve(root, "src/components"), { recursive: true });
await writeFile(
resolve(root, "src/components/base.scss"),
".card { color: red; }",
"utf8",
);
await writeFile(
resolve(root, "src/components/theme.scss"),
".card { color: blue; }\n.accent { color: green; }",
"utf8",
);
await writeFile(
resolve(root, "src/components/Card.tsx"),
[
'import "./base.scss";',
'import "./theme.scss";',
'export const Card = () => <div className="card accent" />;',
"",
].join("\n"),
"utf8",
);

const suggestions = await buildMigrationSuggestions(root);
await applyMigrationSuggestions(root, suggestions);

const rewritten = await readFile(
resolve(root, "src/components/Card.tsx"),
"utf8",
);
expect(rewritten).toContain('import styles from "./base.module.scss";');
expect(rewritten).toContain('import styles2 from "./theme.module.scss";');
expect(rewritten).toContain('className={["card", styles2.accent].join(" ")}');
expect(rewritten).not.toContain("styles.card");
expect(rewritten).not.toContain("styles2.card");
Comment on lines +342 to +346

Copilot AI Apr 2, 2026

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new React ambiguity test hard-codes the generated import aliases (styles/styles2) and expects styles2.accent. Alias assignment depends on the order suggestions are processed (which can vary with filesystem readdir ordering), so this assertion can be flaky. Consider making the test order-insensitive by extracting the alias used for ./theme.module.scss from the rewritten imports and asserting that .accent uses that alias, without assuming a specific alias name.

Suggested change
expect(rewritten).toContain('import styles from "./base.module.scss";');
expect(rewritten).toContain('import styles2 from "./theme.module.scss";');
expect(rewritten).toContain('className={["card", styles2.accent].join(" ")}');
expect(rewritten).not.toContain("styles.card");
expect(rewritten).not.toContain("styles2.card");
// Extract aliases for the generated CSS module imports to make the test
// independent of alias naming and filesystem ordering.
const importRegex =
/import\s+(\w+)\s+from\s+"(.+?\.module\.scss)";/g;
const aliasesByPath: Record<string, string> = {};
let match: RegExpExecArray | null;
// eslint-disable-next-line no-cond-assign
while ((match = importRegex.exec(rewritten)) !== null) {
const [, alias, path] = match;
aliasesByPath[path] = alias;
}
const baseAlias = aliasesByPath["./base.module.scss"];
const themeAlias = aliasesByPath["./theme.module.scss"];
expect(baseAlias).toBeTruthy();
expect(themeAlias).toBeTruthy();
// Ensure the accent class comes from the theme module alias, and the
// ambiguous "card" class remains a plain string.
expect(rewritten).toContain(
`className={["card", ${themeAlias}.accent].join(" ")}`,
);
// No alias (from either module) should be used for ".card".
for (const alias of Object.values(aliasesByPath)) {
expect(rewritten).not.toContain(`${alias}.card`);
}

Copilot uses AI. Check for mistakes.
} finally {
await rm(root, { recursive: true, force: true });
}
});

it("should rewrite react template literals with mapped classes", async () => {
const root = await mkdtemp(
resolve(tmpdir(), "recss-core-migrate-template-"),
Expand Down Expand Up @@ -886,6 +931,58 @@ describe("migrate helpers", () => {
}
});

it("should leave ambiguous vue class names untouched across multiple module accessors", async () => {
const root = await mkdtemp(
resolve(tmpdir(), "recss-core-migrate-vue-ambiguous-"),
);

try {
await mkdir(resolve(root, "src/components"), { recursive: true });
await writeFile(
resolve(root, "src/components/base.scss"),
".card { color: red; }",
"utf8",
);
await writeFile(
resolve(root, "src/components/theme.scss"),
".card { color: blue; }\n.accent { color: green; }",
"utf8",
);
await writeFile(
resolve(root, "src/components/Card.vue"),
[
"<template>",
' <section class="card accent" />',
"</template>",
"",
'<style src="./base.scss"></style>',
'<style module="theme" src="./theme.scss"></style>',
"",
].join("\n"),
"utf8",
);

const suggestions = await buildMigrationSuggestions(root);
await applyMigrationSuggestions(root, suggestions);

const rewritten = await readFile(
resolve(root, "src/components/Card.vue"),
"utf8",
);
expect(rewritten).toContain(
'<style module src="./base.module.scss"></style>',
);
expect(rewritten).toContain(
'<style module="theme" src="./theme.module.scss"></style>',
);
expect(rewritten).toContain(`<section :class='["card", $theme.accent]' />`);
expect(rewritten).not.toContain("$style.card");
expect(rewritten).not.toContain("$theme.card");
} finally {
await rm(root, { recursive: true, force: true });
}
});

it("should rewrite nested react wrapper calls around supported class expressions", async () => {
const root = await mkdtemp(
resolve(tmpdir(), "recss-core-migrate-react-wrapper-call-"),
Expand Down
Loading