[material-ui][ListItem] Removing deprecated props #41566

thathva · 2024-03-20T01:31:22Z

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 · 2024-03-20T01:33:51Z

Netlify deploy preview

docs/data/material/migration/migration-v5/migration-v5.md

ListItem: parsed: -22.93% 😍, gzip: -21.11% 😍
@material-ui/core: parsed: -0.08% 😍, gzip: -0.08% 😍

Bundle size report

Details of bundle changes (Toolpad)
Details of bundle changes

Generated by 🚫 dangerJS against c1179d4

DiegoAndai

Hey @thathva, thanks for working on this, here's my initial review.

Besides the comments, we should add these removals as breaking changes in https://github.com/DiegoAndai/material-ui/blob/next/docs/data/material/migration/migration-v5/migration-v5.md. You might have to rebase/merge next into your branch.

packages/mui-material/src/ListItem/ListItem.d.ts

test/integration/material-ui/components.spec.tsx

packages/mui-material/src/ListItem/ListItem.spec.tsx

thathva · 2024-03-20T23:49:37Z

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

DiegoAndai · 2024-03-22T15:27:06Z

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:

git checkout next
git pull upstream next
git checkout material-ui-listitem-remove-props
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 · 2024-03-22T23:52:14Z

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

DiegoAndai

Hey @thathva, thanks for incorporating the suggested changes 🙌🏼

I left a suggestion on the migration guide structure and text.

Besides that, I think we should add a codemod for this breaking change in this same PR. The codemod will replace all occurrences of <ListItem button /> to <ListItemButton />. Would you be willing to work on it? If so, I can set it up and guide you through it.

docs/data/material/migration/migration-v5/migration-v5.md

thathva · 2024-03-25T17:48:38Z

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 · 2024-03-26T17:44:11Z

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 · 2024-03-27T14:11:23Z

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

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

git checkout next
git pull upstream
git checkout material-ui-listitem-remove-props
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:

Find all ListItem components with the button prop
Replace them with ListItemButton, removing the button prop but retaining all other props
Add the ListItemButton import to the file if it doesn't exist
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 · 2024-03-27T14:29:39Z

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

thathva · 2024-04-05T23:40:35Z

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 · 2024-04-09T21:22:13Z

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 · 2024-04-10T01:15:54Z

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!

DiegoAndai

Hey @thathva! Here are some suggestions to move forward

DiegoAndai · 2024-04-10T20:04:41Z

packages/mui-codemod/src/v6.0.0/list-item-button-prop/list-item-button-prop.js

+  //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').find(j.ImportSpecifier)
+    // .filter(path => path.node.imported.name === 'ListItem').remove();
+    root.find(j.ImportDeclaration).filter(path => path.node.source.value === '@mui/material/ListItem').remove();
+  }


Let's try this (I haven't tried it so it might need some fixing):

Suggested change

//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').find(j.ImportSpecifier)

// .filter(path => path.node.imported.name === 'ListItem').remove();

root.find(j.ImportDeclaration).filter(path => path.node.source.value === '@mui/material/ListItem').remove();

}

//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;

}

});

//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;

// Remove ListItem imports if there is no usage

if(!containsListItem) {

// Remove named imports

root.find(j.ImportDeclaration).filter(path => path.node.source.value === '@mui/material').find(j.ImportSpecifier).filter(path => path.node.imported.name === 'ListItem').remove();

// Remove default imports

root.find(j.ImportDeclaration).filter(path => path.node.source.value === '@mui/material/ListItem').remove();

}

DiegoAndai · 2024-04-10T20:05:21Z

packages/mui-codemod/src/v6.0.0/all/deprecations-all.js

+ * @param {import('jscodeshift').API} api
+ */
+export default function deprecationsAll(file, api, options) {
+  file.source = transformerListItemButtonProps(file, api, options);


This should be added to the v6-all one, as it's not a deprecation. And we should remove this file.

thathva · 2024-04-11T23:54:03Z

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:

Renaming the imported name from ListItem to ListItemButton since we don't need to worry about the alias of that component.
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?

DiegoAndai

Hey! Here's my thoughts about the test cases

DiegoAndai · 2024-04-15T20:24:21Z

packages/mui-codemod/src/v6.0.0/list-item-button-prop/test-cases/actual.js

+
+<ListItem button/>;
+
+<MyListItem button/>;


Let's add another component with the button prop which shouldn't be removed, to test that case:

// ... import AnotherComponent from "ui"; // ... <AnotherComponent button />

DiegoAndai · 2024-04-15T20:29:51Z

packages/mui-codemod/src/v6.0.0/list-item-button-prop/test-cases/theme.actual.js

+fn({
+    MuiListItem: {
+      defaultProps: {
+        button
+      }
+    }
+  });
+


I think the transform should remove the MuiListItem completely, but:

Remove the button and any other of the removed props

If the button props was found, copy the values to a new MuiListItemButton entry

So in this example the actual should be

fn({ MuiListItem: { defaultProps: { anotherProp: 'value', button, } } });

And the expected:

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

I think the transform should remove the MuiListItem completely

I meant:

I think the transform shouldn't remove the MuiListItem completely

thathva · 2024-04-17T00:56:35Z

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
+    }
}
});

DiegoAndai

Hey @thathva, sorry for the delay.

DiegoAndai · 2024-04-24T16:55:20Z

packages/mui-codemod/src/v6.0.0/list-item-button-prop/list-item-button-prop.js

+        // Copy properties from MuiListItem's defaultProps except 'button'
+        const newButtonProps = defaultPropsNode.value.properties.filter(prop => prop.key.name !== 'button');
+
+        // Add autoFocus:true to newButtonProps


By the testcase, I assumed that we would need to move all of MuiListItem props to MuiListItemButton prop. So we would only need to remove button, and copy over the remaining props to MuiListItemButton correct? Or would it just be removing button and MuiListItemButton would by default have the anotherProp key?

we would need to move all of MuiListItem props to MuiListItemButton prop

Yes, we should move MuiListItem props into MuiListItemButton if those props apply to MuiListItemButton.

So we would only need to remove button, and copy over the remaining props to MuiListItemButton correct?

Correct

My question is, why always adding autoFocus: true? Shouldn't we add it only if it's present in MuiListItem's defaultProps?

Okay got it! I made a mistake in assuming autoFocus: true would be a default property. I will make the changes accordingly.

DiegoAndai · 2024-04-24T17:05:01Z

packages/mui-codemod/src/v6.0.0/list-item-button-prop/list-item-button-prop.js

+  });
+
+  let hasButton = false;
+  root.find(j.ObjectProperty).forEach((path) => {


May I ask you to refactor to use the findComponentDefaultProps util?

That returns a collection of defaultProps paths, given a component name. You can follow this example as a guideline: https://github.com/mui/material-ui/blob/next/packages/mui-codemod/src/deprecations/utils/movePropIntoSlots.js

This will make it easier to guide and debug 😊

Sure, I will take a look at that. That is helpful!

thathva · 2024-06-17T22:34:22Z

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!

Removing deprecated props from material-ui ListItem

2efb7a1

thathva changed the title ~~[material-ui][ListItem] Removing deprecated props from material-ui ListItem~~ [material-ui][ListItem] Removing deprecated props Mar 20, 2024

danilo-leal added component: list This is the name of the generic UI component, not the React module! package: material-ui Specific to @mui/material labels Mar 20, 2024

DiegoAndai self-requested a review March 20, 2024 15:17

DiegoAndai reviewed Mar 20, 2024

View reviewed changes

packages/mui-material/src/ListItem/ListItem.d.ts Outdated Show resolved Hide resolved

test/integration/material-ui/components.spec.tsx Outdated Show resolved Hide resolved

packages/mui-material/src/ListItem/ListItem.spec.tsx Show resolved Hide resolved

Removed ListItem's dependency on ExtendButtonBase

6a09a81

Yenuga, Thathva Sri Sai Reddy added 3 commits March 22, 2024 18:48

Merge branch 'next' into material-ui-listitem-remove-props

5fde6a0

Updated migration document

5a84d05

Adding space between headings in markdown

418bf7a

DiegoAndai reviewed Mar 25, 2024

View reviewed changes

docs/data/material/migration/migration-v5/migration-v5.md Outdated Show resolved Hide resolved

Rephrasing breaking changes section in migration doc

8f8db62

DiegoAndai mentioned this pull request Mar 26, 2024

[codemod] Setup v6 codemod structure #41668

Merged

Yenuga, Thathva Sri Sai Reddy added 2 commits April 9, 2024 20:17

Merge branch 'next' into material-ui-listitem-remove-props

503d663

Codemod changes for converting ListItem to ListItemButton

6e14621

DiegoAndai reviewed Apr 10, 2024

View reviewed changes

Fixing failed test case related to component

86242b4

Fixing failing theme related test case

9b31019

DiegoAndai reviewed Apr 15, 2024

View reviewed changes

Updated test cases for codemod

0947ef8

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 22, 2024

DiegoAndai reviewed Apr 24, 2024

View reviewed changes

Merge branch 'next' into material-ui-listitem-remove-props

7f6f9f4

github-actions bot added PR: out-of-date The pull request has merge conflicts and can't be merged and removed PR: out-of-date The pull request has merge conflicts and can't be merged labels Jun 13, 2024

Merge branch 'next' into material-ui-listitem-remove-props

c1179d4

github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 16, 2024

github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Jun 25, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[material-ui][ListItem] Removing deprecated props #41566

[material-ui][ListItem] Removing deprecated props #41566

thathva commented Mar 20, 2024 •

edited

Loading

mui-bot commented Mar 20, 2024 •

edited

Loading

DiegoAndai left a comment

thathva commented Mar 20, 2024 •

edited

Loading

DiegoAndai commented Mar 22, 2024

thathva commented Mar 22, 2024 •

edited

Loading

DiegoAndai left a comment

thathva commented Mar 25, 2024

DiegoAndai commented Mar 26, 2024

DiegoAndai commented Mar 27, 2024

thathva commented Mar 27, 2024

thathva commented Apr 5, 2024

DiegoAndai commented Apr 9, 2024

thathva commented Apr 10, 2024

DiegoAndai left a comment

DiegoAndai Apr 10, 2024

DiegoAndai Apr 10, 2024

thathva commented Apr 11, 2024 •

edited

Loading

DiegoAndai left a comment

DiegoAndai Apr 15, 2024

DiegoAndai Apr 15, 2024

DiegoAndai Apr 24, 2024

thathva commented Apr 17, 2024

DiegoAndai left a comment

DiegoAndai Apr 24, 2024

thathva Apr 24, 2024

DiegoAndai Apr 24, 2024

thathva Apr 24, 2024

DiegoAndai Apr 24, 2024

thathva Apr 24, 2024

thathva commented Jun 17, 2024

[material-ui][ListItem] Removing deprecated props #41566

Are you sure you want to change the base?

[material-ui][ListItem] Removing deprecated props #41566

Conversation

thathva commented Mar 20, 2024 • edited Loading

mui-bot commented Mar 20, 2024 • edited Loading

Netlify deploy preview

Bundle size report

DiegoAndai left a comment

Choose a reason for hiding this comment

thathva commented Mar 20, 2024 • edited Loading

DiegoAndai commented Mar 22, 2024

thathva commented Mar 22, 2024 • edited Loading

DiegoAndai left a comment

Choose a reason for hiding this comment

thathva commented Mar 25, 2024

DiegoAndai commented Mar 26, 2024

DiegoAndai commented Mar 27, 2024

thathva commented Mar 27, 2024

thathva commented Apr 5, 2024

DiegoAndai commented Apr 9, 2024

thathva commented Apr 10, 2024

DiegoAndai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thathva commented Apr 11, 2024 • edited Loading

DiegoAndai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thathva commented Apr 17, 2024

DiegoAndai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thathva commented Jun 17, 2024

thathva commented Mar 20, 2024 •

edited

Loading

mui-bot commented Mar 20, 2024 •

edited

Loading

thathva commented Mar 20, 2024 •

edited

Loading

thathva commented Mar 22, 2024 •

edited

Loading

thathva commented Apr 11, 2024 •

edited

Loading