[material-ui][ListItem] Removing deprecated props

[thathva]

• [x] I have followed (at least) the PR section of the contributing guide.

Removed the deprecated props from Material-UI ListItem API component (Closes Issue #41296). Modified test cases, documentation and proptypes. Removed props:

• autoFocus
• button
• disabled
• selected

[mui-bot]

Netlify deploy preview

• docs/data/material/migration/migrating-to-v6/migrating-to-v6.md

ListItem: parsed: -19.53% :heart_eyes:, gzip: -17.97% :heart_eyes: @material-ui/core: parsed: -0.29% :heart_eyes:, gzip: -0.16% :heart_eyes:

Bundle size report

Details of bundle changes (Toolpad) Details of bundle changes

Generated by :no_entry_sign: dangerJS against ac75cc689a583ad1422230ad2a3684b52f24541a

[thathva]

I have made the changes as commented. Let me know if there are any more changes!

[DiegoAndai]

Nicely done @thathva!

Regarding the migration guide:

We need to add these removals to the guide in this PR. But the guide was added after the thathva:material-ui-listitem-remove-props branch was created, so the files do not exist. That is why you'll have to do:

1. git checkout next
2. git pull upstream next
3. git checkout material-ui-listitem-remove-props
4. git merge next

Which will bring the latest changes of the next branch into the material-ui-listitem-remove-props branch. Then you'll be able to find the /docs/data/material/migration/migration-v5/migration-v5.md file and add the removals to it. You can follow this similar migration guide to follow: https://github.com/mui/material-ui/blob/next/docs/data/system/migration/migration-v5/migration-v5.md

[thathva]

Thank you for the references and the detailed steps @DiegoAndai! I have updated the migration document.

[thathva]

Hi @DiegoAndai Sure, I will rephrase it as you suggested. And the codemod, yes I would love to add that! If there is a guide, I can refer that and try it out.

[DiegoAndai]

And the codemod, yes I would love to add that! If there is a guide, I can refer that and try it out.

I'll set it up today/tomorrow and reach out 😊

[DiegoAndai]

Hey @thathva, I set up the codemods for v6 🎉

First you'll have to merge next into your branch:

1. git checkout next
2. git pull upstream
3. git checkout material-ui-listitem-remove-props
4. git merge next

Then, read the codemods contributing guide.

The codemod we will add should be named list-item-button-prop. It should do the following:

1. Find all ListItem components with the button prop
2. Replace them with ListItemButton, removing the button prop but retaining all other props
3. Add the ListItemButton import to the file if it doesn't exist
4. If there are no other ListItem components in the file, remove the ListItem import

The codemod should be added to the v6.0.0 codemods folder.

For further guidance, I recommend going through:

• The jscodeshift documentation
• The deprecations avatar-props codemod which shows you how to find a certain prop on a certain component

Please reach out if you're stuck and need help 😊

I will be on vacation starting tomorrow (March 28th) until April 8th, so I won't attend to notifications during that time. @siriwatknp may I ask you to provide guidance in the meantime?

[thathva]

Awesome, thanks for the guide @DiegoAndai! I will take a look and reach out if I have any questions. Have a great vacation!

[thathva]

Hey @siriwatknp I apologize for the delay in the PR, I have been occupied with few things. I am facing a bit of issues with the codemod, so would appreciate some help. The code is the first version that I was hoping to get it to work before I could refactor. I have the code below for the codemod, and the test cases, which seem to fail. I was wondering if you could help me out if I am going in the right direction?

list-item-button-prop.js

import findComponentJSX from '../../util/findComponentJSX';

/**
 * @param {import('jscodeshift').FileInfo} file
 * @param {import('jscodeshift').API} api
 */
export default function transformer(file, api, options) {
  const j = api.jscodeshift;
  const root = j(file.source);
  const printOptions = options.printOptions;

  //Rename components that have ListItem button to ListItemButton
  findComponentJSX(j, { root, componentName: 'ListItem' }, (elementPath) => {
    const index = elementPath.node.openingElement.attributes.findIndex(
      (attr) => attr.type === 'JSXAttribute' && attr.name.name === 'button',
    );
    if (index !== -1) {
      elementPath.node.openingElement.name.name = 'ListItemButton';
      elementPath.node.openingElement.attributes.splice(index, 1);
    }
  });

  //Find if there are ListItem imports/named imports.
  let containsListItemImport = root.find(j.ImportDeclaration).filter(path => path.node.source.value === '@mui/material');
  let containsListItemNamedImport = containsListItemImport.find(j.ImportSpecifier).filter(path => path.node.imported.name === 'ListItem');
  let containsListItem = false;

  //Find components that use ListItem. If they do, we shouldn't remove it
  findComponentJSX(j, { root, componentName: 'ListItem' }, (elementPath) => {
    if(elementPath.node.openingElement.name.name === 'ListItem') {
      containsListItem = true;
    }
  });

  //Remove ListItem import if there is no usage
  if(containsListItemNamedImport.length === 0 || !containsListItem) {
    root.find(j.ImportDeclaration).filter(path => path.node.source.value === '@mui/material/ListItem').remove();
  }

  //If ListItemButton does not already exist, add it at the end
  let imports = root.find(j.ImportDeclaration).filter(path => path.node.source.value === '@mui/material/ListItemButton');

  if(imports.length === 0) {
    let lastImport = root.find(j.ImportDeclaration).at(-1);

    // Insert the import for 'ListItemButton' after the last import declaration
    lastImport.insertAfter(
      j.importDeclaration(
        [j.importDefaultSpecifier(j.identifier('ListItemButton'))],
        j.stringLiteral('@mui/material/ListItemButton')
      )
    );
  }

  return root.toSource(printOptions);
}

actual.js

import ListItem from '@mui/material/ListItem';
import {ListItem as MyListItem} from '@mui/material';

<ListItem button/>;

<MyListItem button/>;

expected.js

import ListItemButton from '@mui/material/ListItem';
import {ListItemButton as MyListItemButton} from '@mui/material';

<ListItemButton />;
<MyListItemButton />;

The test case to transform the props is failing with these results:

+expected 
-actual

-import {ListItem as MyListItem} from '@mui/material';
-
-import ListItemButton from "@mui/material/ListItemButton";
-
-
-
-<ListItemButton />;
-
-
-
-<ListItemButton />;
+import {ListItemButton as MyListItemButton} from '@mui/material';
+import ListItemButton from '@mui/material/ListItem';
+
+<ListItemButton />;
+<MyListItemButton />;

[DiegoAndai]

Hey @thathva, I'm back from vacation so I can help with this.

The code is the first version that I was hoping to get it to work before I could refactor.

Could you commit this first version? That way it will be easier to review.

In the following code:

  //Remove ListItem import if there is no usage
  if(containsListItemNamedImport.length === 0 || !containsListItem) {
    root.find(j.ImportDeclaration).filter(path => path.node.source.value === '@mui/material/ListItem').remove();
  }

We're only removing the @mui/material/ListItem import, that's why the

import {ListItem as MyListItem} from '@mui/material';

is not removed. We have to cover that case as well.

[thathva]

Hey @DiegoAndai Thank you for your guidance again! I have pushed the changes. I have used similar test cases as other codemods, and I am failing 2 of the 6 test cases, which one of them is because of the named import. Would appreciate your help!

[thathva]

Hey @DiegoAndai! Thank you for your inputs! I used the code you mentioned and was able to fix that test case. I made the following assumptions:

1. Renaming the imported name from ListItem to ListItemButton since we don't need to worry about the alias of that component.
2. Using the same assumption, not changing the alias in the code, but just removing the button prop from it. Let me know if this sounds good. ~~Apart from this, there is a test case failing for the theme, which I am not familiar with. Can you direct me to any other codemod that modifies the theme? To my understanding, we would need to remove the defaultProps from MuiListItem and replace it with MuiListItemButton. The test case:~~ I was able to fix all the test cases. But, I still have some doubt over the correctness of the actual.js and expected.js files for themes and props. Can you let me know if this looks good, so that I could perform the codemod for the entire codebase?

[thathva]

The props test cases works as expected, but I am having trouble with the theme test cases. I am able to add the MuiListItemButton object, but it is being adding as a child to MuiListItem. Do you have any suggestions for this?

fn({
MuiListItem: {
defaultProps: {
anotherProp: 'value'
-    },
-
-    MuiListItemButton: {
-      defaultProps: {
-        anotherProp: 'value',
-        autoFocus: true
-      }
}
+  },
+  MuiListItemButton: {
+    defaultProps: {
+      anotherProp: 'value',
+      autoFocus: true
+    }
}
});

[thathva]

Hi @DiegoAndai,

I wanted to apologize for the long delay in addressing this issue. I got caught up with some things so I wasn't able to focus on it. Given the complexity and the time it has taken, would it be possible to consider closing the deprecations part of this issue for now? I was wondering if the codemod transformations be moved to a separate issue. Let me know what you think, thanks!

[mnajdova]

@DiegoAndai I improved a lot the codemod in regards to the replacement of the open tags & removing the non-used imports. I am not sure about the theming codemod, honestly I would just drop that logic. I am letting you do the final review.

[mnajdova]

@thathva thanks for kicking off this effort. I pushed few commits and merged it :)

[thathva]

Thank you so much @mnajdova and I apologize for not finishing it completely. The changes you made looks great!