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 jss to tss-react codemod #31802

Merged
merged 48 commits into from
May 11, 2022
Merged

Conversation

ryancogswell
Copy link
Contributor

Initial naive implementation. Many more test cases left to add.

Initial naive implementation. Many more test cases left to add.
@mui-bot
Copy link

mui-bot commented Mar 14, 2022

No bundle size changes

Generated by 🚫 dangerJS against 264794f

@danilo-leal danilo-leal added the package: codemod Specific to @mui/codemod label Mar 14, 2022
@ryancogswell
Copy link
Contributor Author

ryancogswell commented Mar 28, 2022

Here is the list of scenarios I intend to support before taking this out of draft mode:

  • Transform imports for withStyles and makeStyles from @material-ui/core/styles, @material-ui/core, and @mui/styles
  • Remove imports and usages of createStyles
  • Transform usages of makeStyles with an object argument
  • Transform usages of makeStyles with an arrow function argument with a single theme parameter
  • Transform usages of makeStyles with an arrow function argument with no parameters
  • Transform usages of makeStyles with an arrow function argument with no parameters with nested selectors
  • Transform usages of makeStyles with an object argument with nested selectors
  • Transform usages of makeStyles with an arrow function argument with a single theme parameter with nested selectors
  • Transform usages of withStyles with an object argument defined inline
  • Transform usages of withStyles with an object argument. Object assigned to a top-level variable. With and without nested selectors.
  • Transform usages of withStyles with an arrow function argument. Arrow function assigned to a top-level variable. Arrow function contains a code block with an explicit return of an object expression containing nested selectors.

@ryancogswell ryancogswell marked this pull request as ready for review March 30, 2022 00:38
@ryancogswell ryancogswell requested a review from mnajdova March 30, 2022 01:36
@ryancogswell
Copy link
Contributor Author

ryancogswell commented Mar 30, 2022

@mnajdova I haven't verified this against my own code base yet so I might have follow-up tweaks once I do, but it now covers all of the scenarios that I am aware of in my code base. The main features it doesn't handle are params to useStyles (we completely avoided dynamic styles in JSS in our code base) and providing a stylesheet name.

@mnajdova
Copy link
Member

mnajdova commented Apr 6, 2022

@mnajdova I haven't verified this against my own code base yet so I might have follow-up tweaks once I do, but it now covers all of the scenarios that I am aware of in my code base.

That's great! Should we wait for your verification in your project? I think it's healthy if we have tested it on at least one project.

The main features it doesn't handle are params to useStyles (we completely avoided dynamic styles in JSS in our code base) and providing a stylesheet name.

Alright, I may spend some time this quarter on it, or we could try to find some help from the community of tss-react cc @garronej


All in all great effort @ryancogswell

@ryancogswell

This comment was marked as outdated.

@ryancogswell
Copy link
Contributor Author

ryancogswell commented Apr 19, 2022

@mnajdova I believe this is now ready to merge.

@github-actions github-actions bot removed the PR: out-of-date The pull request has merge conflicts and can't be merged label Apr 19, 2022
@ryancogswell
Copy link
Contributor Author

@mnajdova Please let me know if there’s anything I can do to help move the review of this forward.

@ryancogswell
Copy link
Contributor Author

ryancogswell commented May 2, 2022

@garronej There are 9 pairs of test files in the mui-codemod/src/v5.0.0/jss-to-tss-react.test directory. Could you please help with the review by looking through each pair (each actual* file has a corresponding expected* file showing what the codemod should produce) of test files to verify that the expected results match what you would expect/recommend.

You can also help by reviewing my changes to the documentation. In particular, I removed your #### Overriding styles - classes prop section. Instead, the comprehensive example includes an example of how the codemod would transform use of the classes prop and includes a link to the tss-react documentation about it.

@garronej
Copy link
Contributor

garronej commented May 2, 2022

@ryancogswell Yes yes, sorry, I have a lot on my plate but I will definitively have a look ASAP.

@ryancogswell
Copy link
Contributor Author

@garronej Most of the examples are pretty straightforward, and I've already verified that they all work (at least once all the MUI imports are also transformed by other codemods), so I think it will be pretty quick to look through.

Copy link
Contributor

@garronej garronej left a comment

Choose a reason for hiding this comment

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

You did an impressive job congratulation 👏🏻🎉
I just added some comments about what, I think, are best practice but feel free to ignore.

@ryancogswell
Copy link
Contributor Author

@garronej Thanks for reviewing things! In looking over the migration guide I noticed a few broken links to your GitHub readme that I just fixed to go to the equivalent docs.tss-react.dev URL, so that will give people a resource for best practices in using tss-react. For now, I think I'll leave my "comprehensive example" as it is -- with the focus being to show what the codemod will do in a more complicated scenario rather than trying to duplicate the best-practices guidance that is in your documentation.

@garronej
Copy link
Contributor

garronej commented May 3, 2022

Fair enough

@ryancogswell
Copy link
Contributor Author

@garronej And thanks for creating tss-react! Our company's application is migrated to v5 in our testing environment with the intention of moving v5 to production this weekend. We wouldn't be able to make this transition nearly as quickly or confidently if we had needed to move the styles (700 usages of makeStyles) to a different implementation approach and it's nice to be able to make a clean break from the JSS implementation so that our code can be StrictMode compatible going forward.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 6, 2022
@garronej
Copy link
Contributor

garronej commented May 7, 2022

Hi @ryancogswell,
Just so you know there is now an ESLint plugin available for detecting unused classes:
https://docs.tss-react.dev/detecting-unused-classes
I want to document it but I'll wait for this PR to be merged first to avoir conflict.

Best regards.

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

I am going to do a review on this tomorrow, looks promising @ryancogswell :)

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

I couldn't wait till tomorrow :D Just small details added, looks great overall.

const expected = read('./jss-to-tss-react.test/expected-from-material-ui-core-styles.js');
expect(actual).to.equal(expected, 'The transformed version should be correct');
});
it('should be idempotent', () => {
Copy link
Member

Choose a reason for hiding this comment

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

Nice, if we add more features, people would be able to just run the new codemod version again 👍

:::error
**WARNING**: You should drop [`clsx`](https://www.npmjs.com/package/clsx) in favor of [`cx`](https://emotion.sh/docs/@emotion/css#cx).
The key advantage of `cx` is that it detects emotion generated class names ensuring styles are overwritten in the correct order.
The default precedence of styles from multiple CSS classes is different between JSS and tss-react and some manual re-ordering of `cx` parameters
Copy link
Member

Choose a reason for hiding this comment

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

I've pushed a commit here for applying some changes from master. Would be great if you can double check if this is the changed content on the migration guide (LGTM btw)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did a merge of those migration guide changes yesterday, but I should have installed the merge tool that I use at work onto my home computer. I used my IDE's merge resolution instead and looks like it didn't behave the way I thought and lost a lot of the changes that I thought I was merging in. Thanks for fixing it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Some of my previous migration guide changes were lost during your fix of my bad merge. I've reapplied those changes now.

npx @mui/codemod v5.0.0/jss-to-tss-react <path>
```

You can find more details about migrating from JSS to tss-react in [the migration guide](https://mui.com/guides/migration-v4/#2-use-tss-react).
Copy link
Member

Choose a reason for hiding this comment

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

Would be great if we can list here things that are not supported, for example the "TODOs" added in expected-todo-comments.js.

This will make sure we are transparent and will help future contributors to take features one by one and add them as part of the codemod.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've added documentation about the scenarios that produce TODO comments.

Copy link
Member

@mnajdova mnajdova left a comment

Choose a reason for hiding this comment

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

Great job @ryancogswell! Can't wait to share this with the community :)

@mnajdova mnajdova merged commit 2c4f912 into mui:master May 11, 2022
@garronej
Copy link
Contributor

Thank you very much @ryancogswell, you did a great job!

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.

5 participants