search

mui / material-ui

Material UI: Comprehensive React component library that implements Google's Material Design. Free forever.
https://mui.com/material-ui/
MIT License
93.86k stars 32.26k forks source link

RFC: Material-UI v4 breaking changes #13663

Closed eps1lon closed 5 years ago

eps1lon commented 5 years ago

⚠️ No more breaking changes planned.

Tracking issue for all the breaking changes that are planned or proposed for Material-UI v4.

v4 preview: https://next--material-ui.netlify.com/

Unchecked checkboxes mean planned only. If those changes should be discussed then there either exists an existing issue or a separate one should be opened. Checked means a PR is either open or merged into next. Changes live in the next branch. Beta releases are not planned yet.

Releases

4.0.0-alpha.0

4.0.0-alpha.1

4.0.0-alpha.2

4.0.0-alpha.3

4.0.0-alpha.4

4.0.0-alpha.5

4.0.0-alpha.6

4.0.0-alpha.7

4.0.0-alpha.8

4.0.0-alpha.9

4.0.0-beta.0

🏁

Rejected

This includes a proposal to consider the master branch as 3.x and next as 4.0.

We can either default to next which means we would need to backport changes where necessary or stay at master which would require a port of each PR to next. Either way merges between next and master should not be squashed since each commit in master is already squashed. Further squashing reduces the usability of git.

Someone with admin rights to the repo should protect the next branch against force pushes.

cc @mui-org/core-contributors

rosskevin commented 5 years ago

Always forward, never back. So when 4.x is ready for release, branch master as 3.x and next should become master. Makes 3.x maintainable, puts focus on current, and makes room for 5.x on next.

oliviertassinari commented 5 years ago

@eps1lon Even before v1, I have started collecting a list of potential breaking changes for v4. I have 28 items so far. How do you want me to render this list? I can list them here maybe with a description and motivation?

KaRkY commented 5 years ago

What I would suggest for v4 is unifying callbacks. Table pagination onPageChange(event, value) while onChangeRowsPerPage(event). I would suggest that all callbacks thar return a value are callback(event, value).

eps1lon commented 5 years ago

@oliviertassinari Just add them to the original post and a small explanation if necessary. If you think it needs discussion I would suggest another issue. Github issues are not equipped to handle multiple discussions.

oliviertassinari commented 5 years ago

@eps1lon Done.

TrySound commented 5 years ago

functional -> function components

TrySound commented 5 years ago

Can Grid be eliminated in favour of system?

fzaninotto commented 5 years ago

How are we supposed to :+1: / :-1: on particular items? Also, please prefix this discussion with [RFC] if you're really open for discussion.

eps1lon commented 5 years ago

@fzaninotto Either on the linked issues or open a separate one. A productive discussion about multiple items is really hard within a single issue. This is just a tracking issue.

jedwards1211 commented 5 years ago

theme.palette -> theme.colors

Have any devs reported being confused by theme.palette? If this change would actually help enough devs I think it would be worth it, but it would require wide-ranging changes to a lot of people's code that uses theme.palette, so I think it's only worth changing if very many devs have expressed confusion about this. Making a codemod isn't as trivial as some of the other changes because of the different ways one can access theme.palette (destructuring or not, etc.)

jedwards1211 commented 5 years ago

Icon/SvgIcon change the fontSize value property default -> normal.

I think "normal" actually has less concrete meaning than "default"?

jedwards1211 commented 5 years ago

<Grid spacing={1|2|3} />

If someone for whatever reason is forced by a UX team to use a spacing that isn't a multiple of 8px, both the current behavior and this proposal would make that impossible for them. Maybe there should be a separate spacingPx property?

eps1lon commented 5 years ago

If someone for whatever reason is forced by a UX team to use a spacing that isn't a multiple of 8px,

By default we want to provide a good implementation and DX for material design which describes that elements are aligned to a grid. See https://material.io/design/layout/spacing-methods.html#spacing. Moving from absolute spacing to increments helps themeability of the spacing.

both the current behavior and this proposal would make that impossible for them. 

- <Grid spacing={9} />
+ <Grid spacing={1.125} />

It's not impossible but in material design there is a base spacing that every element aligns to. The new approach makes this obvious where the old one left room for interpretation.

willbamford commented 5 years ago

I'd be in favour of the change of theme.palette to theme.colors. I don't think that palette is particularly confusing but colors captures the semantics better. Sorry for the noise in this thread - would have 👍 if separate issue.

VincentLanglet commented 5 years ago

@jedwards1211 What about exposing both theme.palette and theme.colors. palette can be deprecated in v4 and removed in v5. I would be easier for developer to make the change.

oliviertassinari commented 5 years ago

We have just changed the default branch from master to next. We will keep publishing important bug fixes from the master branch (v3.9.x). Any ongoing work will be on the next branch, starting today.

The major version history:

@VincentLanglet 👋🍔

fzaninotto commented 5 years ago

I'm +1 with all these. Just a few remarks:

jedwards1211 commented 5 years ago

@VincentLanglet deprecation is fine, but it doesn't reduce the amount of work it takes to make the change, it just allows devs to put it off. @oliviertassinari changing palette -> colors will require a lot of work for devs for no benefit other than keeping up-to-date with Material UI. (I mean I there is a quick and dirty solution: Object.defineProperty(theme, 'palette', {get() { return this.colors }}) but ugh...). There are many breaking changes that have no upfront benefit but are perfectly acceptable because they enable new features in the future; however, this doesn't enable any new features. I think you should conduct a twitter poll on it.

jedwards1211 commented 5 years ago

@eps1lon ah, that's a good point. I saw that the Grid propTypes currently warn if the spacing isn't a multiple of 8 <= 72 or something, so you're suggesting that the new spacing prop would be allowed to be any number without restriction?

VincentLanglet commented 5 years ago

changing palette -> colors will require a lot of work for devs

A lot ? It's juste a search-replace for me. 10 minutes maximum with a correct IDE. Am I wrong ? @jedwards1211

jedwards1211 commented 5 years ago

@VincentLanglet that's true, now that it's so easy to write jscodemod scripts for things like the Typography variant changes I guess I'm spoiled into thinking anything that wouldn't be a trivial codemod script will be a PITA. The fortunate thing is that palette isn't super likely to be used anywhere apart from theme code. If it were the other way around (colors -> palette) there would be a lot of irrelevant matches.

eps1lon commented 5 years ago

Proposed removal of typings for @material-ui/core/es/* in #14392.

eps1lon commented 5 years ago

RFC opened for ref behavior in v4: #14415 . I suggest that anybody who attached refs to our components to check it out.

igorbt commented 5 years ago

@material-ui/styles package seems to have already a pretty generic theming support. However, still TypeScript definitions are not generic: 

declare module '@material-ui/styles/ThemeProvider' {
  import { Theme } from '@material-ui/core';
ianschmitz commented 5 years ago

A few breaking change considerations that have tripped up our team a few times:

fzaninotto commented 5 years ago

I'd love an UPGRADE file at the root of the repository listing all the breaking changes in v4, and documenting the upgrade path for existing codebases. The changelog on GitHub is mixing bug fixes and breaking changes, and it's not organized by component, so it's hard to use it to update a codebase.

Note that I really missed that for v1 upgrade, too ;)

eps1lon commented 5 years ago

@fzaninotto Already covered in #14741. Please note that not every breaking change will be listed there immediately since some are part of a bigger picture. https://next.material-ui.com/guides/migration-v3/

fzaninotto commented 5 years ago

Sorry, I had missed that one. Thanks!

Falieson commented 5 years ago

Something changed in the tables alpha.1 to alpha.7 , I now have to add height (not lineHeight) to all rows. Update - I looked at the migration guide PR and see that the table height change is noted there.

oliviertassinari commented 5 years ago

@Falieson Thank you for the problem report. Were you overriding the styles prior to the change of behavior? Any idea what changed?

Falieson commented 5 years ago

@oliviertassinari My table is derived from: https://material-ui.com/demos/tables/#sorting-amp-selecting . I just componentized and hooked it . I might have a fixed height on my tables which is causing the spread?

On a new note - would it be a lot of effort to include an upgrade script that will look for all instances of blocks that contain the properties moved from /core/ to /styles/ and update the import?

import {
  CardContent,
  createStyles,
  Theme,
  Typography,
  withStyles,
  WithStyles,
} from '@material-ui/core'
import {
  CardContent,
  Theme,
  Typography,
} from '@material-ui/core'
import {
  createStyles,
  withStyles,
  WithStyles,
} from '@material-ui/styles'

I tried a simple search/replace which gave me a big chunk of the work, but also created some mistakes. In my 6 week old app, I've had to update 120+ files, and its taken me over 5 hours.

oliviertassinari commented 5 years ago

My table is derived from: https://material-ui.com/demos/tables/#sorting-amp-selecting

@Falieson I have taken the demo, and replaced v3 for v4, no issues: https://codesandbox.io/s/j3vjoyp7oy. I don't know what's going on. We need a reproduction.

On a new note - would it be a lot of effort to include an upgrade script that will look for all instances of blocks that contain the properties moved from /core/ to /styles/ and update the import?

I'm sorry, that's not needed. What lead you to this path? We don't promote it.

Falieson commented 5 years ago

@oliviertassinari Tables: When I have a second I'll make a repro, try to figure out what the issue is that I'm coming across. Imports: not sure what you mean by not promoted? createStyles, withStyles, WithStyles were moved from the /core package to the /styles package. imports fail without updating it. When I write my applications I have a module folder for each of the react libraries I majorly use. Inside my material-ui library I've created abstractions and "Simple React" permutations of all the ways I use Material-UI. I'm not leading the charge here, I've found most mature React teams in SF are building abstraction libraries around third party libs. Here's my MUI folder: image

oliviertassinari commented 5 years ago

@Falieson It's definitely not a bad idea. My point is that it won't change much at the end of the day. All the component demos are using @material-ui/core/styles. If you can avoid the changes, keep your code as is :).

Falieson commented 5 years ago

@oliviertassinari I didn't realize the properties were removed from the toplevel /core/ export but still available at /core/styles. Even still I would have had to refactor and split the imports like below

import {
  CardContent,
  Theme,
  Typography,
} from '@material-ui/core'
import {
  createStyles,
  withStyles,
  WithStyles,
} from '@material-ui/core/styles'
jedwards1211 commented 5 years ago

@Falieson FWIW if you need to rename a bunch of import paths, you can use jscodeshift-transport

pachuka commented 5 years ago

@eps1lon - Something I ran into today when upgrading from 3.9.3 -> 4.0.0-alpha.8. If you are explicitly using JssProvider/Jss to work around bundle splitting/class name generator issues, you need to make sure you upgrade your jss(9.8.7 -> 10.0.0-alpha.16) and react-jss(8.6.1 -> 10.0.0-alpha.16) packages to next versions as well otherwise I kept getting root is undefined errors on classes properties for all components.

Might want to add this to the migration-v3 doc.

oliviertassinari commented 5 years ago

@pachuka We do no longer depend on react-jss. You can remove this dependency. You are correct regarding jss v9 :+1:. We should document it. We might also want to throw an explicit error. Do you have a reproduction for this problem? Do you want to add a quick note about it in the upgrade guide :) ?

pachuka commented 5 years ago

@oliviertassinari, haha, sure I can create a codesandbox reproduction + PR note tomorrow.

TidyIQ commented 5 years ago

Is there an issue with ThemeProvider at this stage? It was working fine previously but now I'm having issues.

I've created a custom theme and wrapped my apps in a ThemeProvider yet all components and makeStyles(theme => { ... })are still using the default theme values.

For example:

// theme.tsx
const theme = createMuiTheme({
  palette: {
    primary: cyan
  }
});
export default theme;
// _app.tsx
class MyApp extends App {
  componentDidMount() {
    // Remove the server-side injected CSS
    const jssStyles = document.querySelector("#jss-server-side");
    if (jssStyles && jssStyles.parentNode) {
      jssStyles.parentNode.removeChild(jssStyles);
    }
  }
  render() {
    const { Component, pageProps } = this.props;
    console.log(theme) // THIS RETURNS THE CORRECT NEW CUSTOM THEME VALUES
    return (
      <Container>
        <Provider>
          <ThemeProvider theme={theme}>
            <CssBaseline />
            <Component {...pageProps} />
          </ThemeProvider>
        </Provider>
      </Container>
    );
  }
}
export default MyApp;
// index.tsx
....
<Typography color="primary">
  Test
</Typography>
....

The result of this is the typography color is set to the default purple[500] instead of cyan[500] as specified in createMuiTheme.

The same issue occurs when using:

const useStyles= makeStyles(theme => {
  changeColor: {
    color: theme.palette.primary.main
  }
}

Is this a known issue?

oliviertassinari commented 5 years ago

@TidyIQ Interesting. It's not the first time I hear about the problem. But I have never been able to reproduce it. What dependency versions are you using?

TidyIQ commented 5 years ago

@oliviertassinari 

 "dependencies": {
    "@material-ui/core": "^4.0.0-alpha.8",
    "@material-ui/icons": "^4.0.0-alpha.8",
    "@material-ui/styles": "^4.0.0-alpha.8",
    "@zeit/next-typescript": "^1.1.1",
    "next": "^8.1.0",
    "react": "^16.8.6",
    "react-dom": "^16.8.6"
  },
  "devDependencies": {
    "@types/next": "^8.0.3",
    "@types/react": "^16.8.13",
    "@types/react-dom": "^16.8.4",
    "@types/styled-jsx": "^2.2.8",
    "typescript": "^3.4.3"
  }
oliviertassinari commented 5 years ago

@TidyIQ 😨 do you have a github repository I could have a look at?

TidyIQ commented 5 years ago

Sure. It's a private repo but I'll make it public for a couple of hours so you can take a look.

https://github.com/TidyIQ/tidyiq_website

edit: Also just FYI, I followed the example repo here to set it up: https://github.com/mui-org/material-ui/tree/next/examples/nextjs-next-with-typescript

oliviertassinari commented 5 years ago

@TidyIQ It works great with a fresh yarn or npm install (rm package-lock.json). However, It's broken with your package-lock.json. I will see if I can find why and even better, add an explicit warning. Thank you!

TidyIQ commented 5 years ago

Yeah you're right. I deleted all my node_modules and package_lock.json then reinstalled and now it's working fine. Strange... Well at least it's working now! Thanks for your help.

oliviertassinari commented 5 years ago

@TidyIQ You have "@material-ui/styles" installed twice in package-lock.json: #15264.

oliviertassinari commented 5 years ago

Thank you, everybody, for the feedback! We are marching toward stable v4. We will replicate the same approach for v5.

ee0pdt commented 5 years ago

@oliviertassinari The change that @Falieson refers to above (createStyles no longer exposed on /core) is a breaking change, but it is not documented anywhere. Worse, the typescript defs still show it being available in /core so VSCode doesn't warn me, but then the compiler fails. Should I raise an issue?

oliviertassinari commented 5 years ago

@ee0pdt See #15532.