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

[core] Introduce x-codemod package #6876

Merged
merged 26 commits into from
Nov 21, 2022

Conversation

LukasTy
Copy link
Member

@LukasTy LukasTy commented Nov 16, 2022

Add the initial "infra" for @mui/x-codemod package.
Added most of the available utils from material-ui, event though only one is used for now.

  • Copy relevant mui-codemod code
  • Add one simple codemod/transform
  • Create preset-safe
  • Refactor code to TypeScript (involved some file moving and the final build package structure became more simple)
  • Test by installing the locally built package and running provided x-codemod bin command
  • Test by installing the codesandbox built package and running provided x-codemod bin command
  • Test by installing the locally built package and running npx @mui/x-codemod
  • Test that various and multiple jscodeshift options can be passed in

@LukasTy LukasTy added the core Infrastructure work going on behind the scenes label Nov 16, 2022
.codesandbox/ci.json Outdated Show resolved Hide resolved
"@babel/core": "^7.20.2",
"@babel/runtime": "^7.20.1",
"@babel/traverse": "^7.20.1",
"jscodeshift": "0.13.1",
Copy link
Member Author

Choose a reason for hiding this comment

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

Locking version to the same as in material-ui as 0.14.0 produces unstable transformations...
I.e.:

return (
  <LocalizationProvider>
    stuff
  </LocalizationProvider>
);

becomes

return (
  (<LocalizationProvider>
    stuff
  </LocalizationProvider>)
);

"directory": "packages/x-codemod"
},
"license": "MIT",
"homepage": "https://github.com/mui/mui-x/tree/next/packages/x-codemod",
Copy link
Member Author

Choose a reason for hiding this comment

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

Ideally this should be updated once we do a stable release. 🙈

@mui-bot
Copy link

mui-bot commented Nov 16, 2022

Messages
📖 Netlify deploy preview: https://deploy-preview-6876--material-ui-x.netlify.app/

These are the results for the performance tests:

Test case Unit Min Max Median Mean σ
Filter 100k rows ms 742.8 1,344.1 742.8 970.72 224.342
Sort 100k rows ms 768.3 1,261.9 1,206.6 1,107.76 176.056
Select 100k rows ms 253.1 336.8 308.4 299.6 29.192
Deselect 100k rows ms 179.6 387.6 244.4 256 77.693

Generated by 🚫 dangerJS against 28da2a4

@siriwatknp
Copy link
Member

2 things that could be improved from the core repo:

  • DX for writing tests. My pain point from the core repo is that the codemod logic files are separated from the test data files. It is quite frustrating when the codemods grow at some point. Ideally, the logic and test data files should live in the same folder:
    codemod-a
      - codemod-a.js
      - codemod-a.test.js
      - ...actual and expected files
    codemod-b
      - ...similar structure
  • I remember that the community would like to pass some options to jscodeshift but it is not possible because @mui/codemod does not allow them to do that.

@flaviendelangle
Copy link
Member

Fully agree with Jun 1st point !

@LukasTy
Copy link
Member Author

LukasTy commented Nov 17, 2022

@siriwatknp Thank you for the suggestion on a better file structure.
I've made changes to follow the following pattern. What do you think?

codemod-a
  - index.ts
  - codemod-a.test.ts
  - actual.spec.js
  - expect.spec.js
codemod-b
  - ...similar structure

`build` contents will be published and there are only relevant files there
@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 17, 2022
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@siriwatknp
Copy link
Member

siriwatknp commented Nov 18, 2022

@siriwatknp Thank you for the suggestion on a better file structure. I've made changes to follow the following pattern. What do you think?

codemod-a
  - index.ts
  - codemod-a.test.ts
  - actual.spec.js
  - expect.spec.js
codemod-b
  - ...similar structure

🤩 Definitely better than the core repo. One note about the spec files because it is possible to have multiple test cases in a single codemod, it could look like this:

codemod-a
  - index.ts
  - codemod-a.test.ts
  - {meaningful name1}-actual.spec.js
  - {meaningful name1}-expect.spec.js
  - {meaningful name2}-actual.spec.js
  - {meaningful name2}-expect.spec.js

But for a single test case, it is enough with this:

codemod-a
  - index.ts
  - codemod-a.test.ts
  - actual.spec.js
  - expect.spec.js
codemod-b
  - ...similar structure

@LukasTy
Copy link
Member Author

LukasTy commented Nov 18, 2022

@siriwatknp
Question regarding:

I remember that the community would like to pass some options to jscodeshift but it is not possible because @mui/codemod does not allow them to do that.

Are you sure that it does not allow? There is even a mention of it in readme. 🤔
But the problem I've noticed after testing this behaviour is that it seems that only 1 option is supported at a time. 🤔
Passing more than 1 seems to ignore all the options.
I'm still investigating this weird behaviour.... 🤷

They can be passed in via `--jscodeshift` option
After changing folder structure to have index.js we can simply target the folder and it's index file will be executed
@LukasTy
Copy link
Member Author

LukasTy commented Nov 18, 2022

@siriwatknp
I've done quite a bit of testing and I think I've narrowed down the issue.
I've updated the code and examples/readme to make it more clear for users.
Let me know if you still see room for improvement. 😉

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Nov 18, 2022
Copy link
Member

@flaviendelangle flaviendelangle left a comment

Choose a reason for hiding this comment

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

Great work !

packages/x-codemod/README.md Outdated Show resolved Hide resolved
@LukasTy LukasTy merged commit 161b43b into mui:next Nov 21, 2022
@LukasTy LukasTy deleted the introduce-x-codemod-package branch November 21, 2022 13:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core Infrastructure work going on behind the scenes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants