Skip to content
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

[codemod] Add accordion props deprecation #40771

Merged
merged 15 commits into from
Jan 30, 2024

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jan 24, 2024

part of #40417

transformation:

 <Accordion
-    TransitionComponent={CustomTransition}
-    TransitionProps={{ unmountOnExit: true }}
+    slots={{ transition: CustomTransition }}
+    slotProps={{ transition: { unmountOnExit: true } }}
 />

existing slots and slotProps are also handled.

Note

Adjusting the codemod structure a bit. It was really painful in v5 to navigate between codemod (the implementation and test cases are separated in different folder.

Moving forward, I think the implementation and test cases should be in the same folder. This is just a DX improvement for our team, there is no impact on consumers.

So from:

/src
  /version
    /transformation.test
      actual.js
      expected.js
    …a lot of folder
    transformation.js
    transformation.test.js

to:

/src
  /version
    /transformation
      /test-cases
        actual.js
        expected.js
      transformation
      transformation.test.js
      index.js
    …a lot of folder
      

@siriwatknp siriwatknp added component: accordion This is the name of the generic UI component, not the React module! package: codemod Specific to @mui/codemod labels Jan 24, 2024
@mui-bot
Copy link

mui-bot commented Jan 24, 2024

Netlify deploy preview

https://deploy-preview-40771--material-ui.netlify.app/

Bundle size report

No bundle size changes (Toolpad)
No bundle size changes

Generated by 🚫 dangerJS against 22fbadb

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 working on this @siriwatknp!

The new file restructure makes sense to me.

@mnajdova, this is the way we envision deprecation codemods working: Deprecations might not be tied to a specific version, for example, the Accordion ones from this PR will exist in v5 and v6. The idea is to have a deprecations codemod that holds all current deprecations of the stable version.

  • Whenever we add a deprecation, we add the respective codemod to the deprecations codemods.
  • Whenever a deprecated API is removed and becomes a breaking change, that codemod is copied over to the version of the breaking change. For example, if TransitionComponent is removed in v7, the accordion-props codemod is copied to the v7 codemods. When v7 becomes the stable version, that codemod is removed from the deprecation codemods.

Users on the stable version would be able to run deprecations/all codemod any time they need to, as these should be idempotent. This way we can also start testing codemods as soon as possible, when the deprecation comes out.

The same goes for the migration guide.

What do you think?

@mnajdova
Copy link
Member

Makes sene! 👍

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.

Hey! I left a couple of comments 😊

Also a question: I tried running the codemod locally to modify the test cases (just to try it out), and it modifies tests-cases/actual.js as expected, but it doesn't modify test-cases/theme.actual.js. You know why that might be?

Copy link
Member

Choose a reason for hiding this comment

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

This file should be index.js, right? Or have an index file that reexports it. Otherwise, it won't be found here when running transform deprecations/all

const actual = transform(
{
source: read('./test-cases/actual.js'),
path: require.resolve('./test-cases/actual.js'),
Copy link
Member

Choose a reason for hiding this comment

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

What's the difference between the source and the path properties? I couldn't find any reference to path inside the transform function.

@DiegoAndai
Copy link
Member

Another question regarding codemods, the <path> provided is traversed recursively? I mean given

my-library/
├── my-components/
│   ├── my-accordion.tsx

If I run the codemod on the my-library path, would my-accordion.tsx be modified?

@siriwatknp
Copy link
Member Author

Another question regarding codemods, the <path> provided is traversed recursively? I mean given

my-library/
├── my-components/
│   ├── my-accordion.tsx

If I run the codemod on the my-library path, would my-accordion.tsx be modified?

Yes, that's right.

@DiegoAndai
Copy link
Member

I tried running the codemod locally to modify the test cases (just to try it out), and it modifies tests-cases/actual.js as expected, but it doesn't modify test-cases/theme.actual.js. You know why that might be?

I figured it out: the difference lies in the AST parsing. For some reason, when I run the codemod locally the TransitionComponent is considered an ObjectProperty instead of a Property (related issue). For some reason, it must be running with a different parser, babel, maybe. I think the correct type is Property, as far as I understand, jscodeshift should be using recast under the hood, and recast users Property, not ObjectProperty. Maybe it's a limitation of running the codemod locally, so we should merge as is (after fixing this) and see if it works when bundled.

@siriwatknp
Copy link
Member Author

siriwatknp commented Jan 26, 2024

I tried running the codemod locally to modify the test cases (just to try it out), and it modifies tests-cases/actual.js as expected, but it doesn't modify test-cases/theme.actual.js. You know why that might be?

I figured it out: the difference lies in the AST parsing. For some reason, when I run the codemod locally the TransitionComponent is considered an ObjectProperty instead of a Property (related issue). For some reason, it must be running with a different parser, babel, maybe. I think the correct type is Property, as far as I understand, jscodeshift should be using recast under the hood, and recast users Property, not ObjectProperty. Maybe it's a limitation of running the codemod locally, so we should merge as is (after fixing this) and see if it works when bundled.

I figured out that the test is not correct, it should use @babel/parser (since we default the codemod parser to tsx) but instead rely on default recast. Anyway, fixed in a05e8a2.

@siriwatknp
Copy link
Member Author

@DiegoAndai I think it's good to go. I added a CONTRIBUTING.md in this PR as well because I always forget about the steps and script when testing locally.

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.

Looks good, there's two test cases that seem not to work

Copy link
Member

Choose a reason for hiding this comment

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

Very useful!

Comment on lines +12 to +14
// case 1: import ComponentName from '@mui/material/ComponentName';
// case 2: import { ComponentName } from '@mui/material';
// case 3: import { ComponentName as SomethingElse } from '@mui/material';
Copy link
Member

Choose a reason for hiding this comment

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

Nice that we cover these! Good catch

Comment on lines 14 to 15
<Accordion slots={{ root: 'div' }} slotProps={{ root: { className: 'foo' } }} />;
<MyAccordion slots={outerSlots} slotProps={outerSlotProps} />;
Copy link
Member

Choose a reason for hiding this comment

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

This looks like it's not working. The transition keys are not getting added.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice catch, fixed!

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 taking this 🙌🏼

@siriwatknp siriwatknp enabled auto-merge (squash) January 30, 2024 03:40
@siriwatknp siriwatknp merged commit c479c9b into mui:master Jan 30, 2024
18 checks passed
mostafa-rio pushed a commit to mostafa-rio/material-ui that referenced this pull request Feb 3, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: accordion This is the name of the generic UI component, not the React module! package: codemod Specific to @mui/codemod
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants