Skip to content

Fix/codemod top level imports 45051 #46405

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.

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

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

AidanLDev
Copy link

Summary

Fixes #45051 - Fixed logical error in top-level-imports codemod that was incorrectly transforming color imports.

Description

The v5.0.0/top-level-imports codemod was incorrectly transforming imports from @mui/material/colors (e.g., import { grey } from '@mui/material/colors') to @mui/material (e.g., import { grey } from '@mui/material'). This transformation is invalid because color exports like grey, blue, etc. are not available from the main @mui/material package.

Root Cause

The issue was in the whitelist check condition:

// Before (buggy)
if (!whitelist.has(specifier.imported.name) == null && ...)

// After (fixed) 
if (!whitelist.has(specifier.imported.name) && ...)

@zannager zannager added the package: codemod Specific to @mui/codemod label Jun 23, 2025
@zannager zannager requested a review from DiegoAndai June 23, 2025 15:34
Copy link
Member

@DiegoAndai DiegoAndai left a comment

Choose a reason for hiding this comment

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

Thanks for submitting this @AidanLDev! The fix seems correct and the tests as well.

May I ask you to move the expected input and output test content to separate files? Similar to how it's done for other tests. You can create separate files for this test in particular, or add it to the existing files.

@AidanLDev
Copy link
Author

Good point, I will update the tests to follow the same style as the other ones :)

@@ -59,3 +58,7 @@ import {
ClickAwayListener,
ListSubheader,
} from '@mui/material';
import { createTheme } from '@mui/material/styles';
Copy link
Member

Choose a reason for hiding this comment

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

Why is createTheme moved? Is createTheme not included on the whitelist?

Copy link
Author

Choose a reason for hiding this comment

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

Good question, that's right it's not currently in the whitelist so to get the tests passing I moved it outside of the @mui/material import.

Copy link
Member

Choose a reason for hiding this comment

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

@siriwatknp do you know if this is correct?

Comment on lines 45 to 64

it('should preserve color imports and styles imports', () => {
const input = read('./top-level-imports.test/actual.js');
const expected = read('./top-level-imports.test/expected.js');
const actual = transform({ source: input, path: 'test.js' }, { jscodeshift }, {});
expect(trim(actual)).to.equal(
trim(expected),
'Color imports and styles imports should be preserved',
);
});

it('should not transform individual color imports', () => {
const input = read('./top-level-imports.test/actual.js');
const expected = read('./top-level-imports.test/expected.js');
const actual = transform({ source: input, path: 'test.js' }, { jscodeshift }, {});
expect(trim(actual)).to.equal(
trim(expected),
'Individual color imports should not be transformed',
);
});
Copy link
Member

Choose a reason for hiding this comment

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

Because we added the imports to the existing actual/expected files, there's no need to add this. Those files are already tested above.

Copy link
Author

Choose a reason for hiding this comment

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

Great point, those two new tests are redundant in that case. I'll remove them

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: codemod Specific to @mui/codemod
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[codemod] v5 top-level-imports changing color imports
3 participants