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 emotion codemod #27292

Merged
merged 15 commits into from Jul 24, 2021

Conversation

siriwatknp
Copy link
Member

@siriwatknp siriwatknp commented Jul 15, 2021

Review actual.js & expected.js

Preview Link
Codemod README

Migrate makeStyles and withStyles is a big part in migration to v5. This codemod might reduce some workload for developers. The approach works well with big component but not with small component that design to be reused. Developers can decide if they want to use or not via

npx @material-ui/codemod v5.0.0/jss-to-styled <folder>

Here is an example of transformation.

Before

import withStyles from '@material-ui/styles/withStyles';

const styles = (theme) => ({
  root: { },
  cta: { },
  footer: { },
})

function Page(props) {
  const { classes } = props;
  return (
    <div>
      <Button className={classes.cta}>Go</Button>
      <footer className={classes.footer}>
        ...
      </footer>
    </div>
  )
}

export default withStyles(styles)(Page);

After

import { styled } from '@material-ui/core/styles';

const PREFIX = 'Page'
const classes = {
  root: `${PREFIX}-root`,
  cta: `${PREFIX}-cta`,
  footer: `${PREFIX}-footer`,
}
const Root = styled('div')(theme => ({
  [`&.${classes.root}`]: {},
  [`& .${classes.cta}`]: {},
  [`& .${classes.footer}`]: {},
}))

function Page(props) {
  const { classes } = props;
  return (
    <Root>
     {/* the good part is there is no change inside Root */}
      <Button className={classes.cta}>Go</Button>
      <footer className={classes.footer}>
        ...
      </footer>
    </Root>
  )
}

export default (Page);

This approach is able to achieve with codemod because we need to styled only one component and let the CSS Specificity do the work.

✅ Tested on material-ui store, works ~70% of the whole project. However, some adjustments is needed.

@mui-pr-bot
Copy link

mui-pr-bot commented Jul 15, 2021

No bundle size changes (experimental)

Generated by 🚫 dangerJS against 59837e3

@siriwatknp siriwatknp added the package: codemod Specific to @mui/codemod label Jul 15, 2021
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.

Looks good for initial view. Would be great if we can document it in the codemods’ README file and list the constraints (for example, should there be one component per file, CSS specificity is increased, etc) I am excited, looks like this could automate big percent for the mogration! Good job @siriwatknp

@siriwatknp
Copy link
Member Author

Looks good for initial view. Would be great if we can document it in the codemods’ README file and list the constraints (for example, should there be one component per file, CSS specificity is increased, etc) I am excited, looks like this could automate big percent for the mogration! Good job @siriwatknp

Good point 👍 will add to README and migration docs.

Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Tested on material-ui store, works ~70% of the whole project. However, some adjustments is needed.

Niice. I plan to double-check the Gatsby integration on the store. I will open a new issue here if it works well, as we can likely improve our Gatsby example, and likely contradict: hupe1980/gatsby-plugin-material-ui#70 (comment).


I have renamed the title of this PR, as we consider it a sentence after the main label in brackets. I'm raising it because I have seen it multiple times :)

-[codemod] add jss to emotion codemod
+[codemod] Add jss to emotion codemod

@oliviertassinari oliviertassinari changed the title [codemod] add jss to emotion codemod [codemod] Add jss to emotion codemod Jul 15, 2021
@siriwatknp siriwatknp force-pushed the feat/codemod-migrate-makestyles branch from 46882d0 to 2cff5bb Compare July 15, 2021 13:21
@siriwatknp siriwatknp requested a review from mnajdova July 15, 2021 13:43
@eps1lon
Copy link
Member

eps1lon commented Jul 17, 2021

[`& .${classes.cta}`]: {},

We need to be careful here how we advertise this codemod. Because the higher specificity might not work in all cases.

Have you tried creating separate styled components instead?

@siriwatknp
Copy link
Member Author

[`& .${classes.cta}`]: {},

We need to be careful here how we advertise this codemod. Because the higher specificity might not work in all cases.

In the codemod README and migration docs, I added info & warning about the approach as much as I can. I think it is a tool that developer needs to trade-off but at least we have it.

Have you tried creating separate styled components instead?

Codemod is too hard to cover all the cases and I don't think it is worth the time. However, I built another tool that require inputs from developer instead.
https://siriwatk.dev/tool/jss-to-styled

@eps1lon
Copy link
Member

eps1lon commented Jul 19, 2021

The higher specificity is kind of scary to me.

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.

In my opinion, we should push forward with this codemod, offering it as an optional. We already list some of the constraints and cons against it. I don't think styled() utility as a codemod will scale, as usually when we render more classes, or conditional classes, the styles are being added in the styled() utility with new prop, then this prop needs to be included in the shouldForwardProp etc. Naming these props isn't trivial, as it depends on the context.

We can even state that this codemod is created in order to help removing the dependency on JSS in the project, and then developers can update the styles if they don't think the higher specificity will scale in their use-cases. The important thing is that, they won't be blocked with the upgrade.

@siriwatknp
Copy link
Member Author

siriwatknp commented Jul 24, 2021

@mnajdova FYI, I added the manual approach and link to the tool that might help developer.

@siriwatknp siriwatknp merged commit c3baf67 into mui:next Jul 24, 2021
@siriwatknp siriwatknp deleted the feat/codemod-migrate-makestyles branch July 24, 2021 07:08
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

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

Tested on material-ui store, works ~70% of the whole project. However, some adjustments is needed.

@siriwatknp Did you plan to open a PR on the store? Or maybe Marija if you are continuing to carry on this v5 migration effort?

packages/material-ui-codemod/README.md Show resolved Hide resolved
Replace JSS styling with `makeStyles` or `withStyles` to `styled` API.

```diff
import Typography from '@material-ui/core/Typography';
Copy link
Member

Choose a reason for hiding this comment

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

x the other cases

Suggested change
import Typography from '@material-ui/core/Typography';
import Typography from '@material-ui/core/Typography';

@siriwatknp
Copy link
Member Author

siriwatknp commented Jul 26, 2021

Did you plan to open a PR on the store

For the store, should we wait feedback from @danilo-leal ? In case he wants to redesign that part.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 26, 2021

In case he wants to redesign that part.

He might want to redesign it but we will have the bandwidth to handle it? This is another one. Here, it seems that we were more into using the store as a way to test the codemod, go to production with it and see where it fails short.

@mnajdova
Copy link
Member

I will open a PR on the store today/tomorrow

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 v5.x migration
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants