[theme] Make theme.palette.augmentColor() pure

[ryancogswell]

• I have followed (at least) the PR section of the contributing guide.

This change avoids mutating the color argument to augmentColor. Includes the corresponding typescript change to indicate return type for augmentColor and test coverage of the typescript and code changes.

Closes #12390

Breaking change

The theme.palette.augmentColor() method do no longer perform a side effect on its input color.
In order to use it correctly, you have to use the output of this function.

-const background = { main: color };
-theme.palette.augmentColor(background);
+const background = theme.palette.augmentColor({ main: color });

console.log({ background });

[ryancogswell]

It looks like there are a couple places in the docs that use augmentColor relying on the mutation. I'll fix this shortly.

[oliviertassinari]

@ryancogswell Sure, I'm adding some new warnings in the meantime.

[oliviertassinari]

Done. We need to fix the /style/color documentation page.

At some point, we should really document our palette/color helpers! #13039, #10789.
By the way, we are considering renaming theme.palette -> theme.colors. What do you think about this change?

[ryancogswell]

I guess I slightly prefer theme.colors. The names of the properties make more direct sense that way (e.g. primary color, secondary color) whereas with theme.palette your brain has to make an additional leap to make sense of it ("primary what? a palette is made up of colors, so I guess it's a primary color), but it still makes sense with theme.palette. If I were making the decision, I don't think I would prefer theme.colors enough to change it. We started using material-ui very recently (a month ago), so at this point it wouldn't be hard to adjust our code, but it seems like a fairly big change for those who have already used material-ui heavily. It would mostly be a straightforward global replace of theme.palette, but it gets more problematic if someone has code that doesn't consistently use the variable name theme, though .palette may still be a sufficient search string in most code bases.

[oliviertassinari]

@ryancogswell Thanks for the feedback.

[eps1lon]

I would define:

• palette as a collection of colors ordered by their looks e.g. palette.grey['400'] or palette.cyan['A300']
• colors as a collection of colors ordered by their semantics e.g. colors.error or colors.primary.light.

But the spec clearly says the primary and secondary are part of the palette: https://material.io/design/color/the-color-system.html#tools-for-picking-colors

Honestly I think this is just not worth it. Palette is simply a synonym for a "collection of colors" (see https://en.oxforddictionaries.com/definition/palette 1.3 (in computer graphics)) so all this change does is add migration workload. I suspect this is motivated by DX since it's easier to type (palette vs. pallete vs. pallette) but this has a workaround: just™ use a statically typed language 😛 Color might have the same drawback (color vs colour).

[oliviertassinari]

@eps1lon Thank you for the feedback too. My concern is primarily with matching what people are already used to. Looking at a wide range of UI libraries, I rarely see the term "palette" being used. Most of the time, they are using "colors" (US). From this observation, I conclude that we can ease the adoption of the library by using the colors wording. colors is 1 letter shorter too but I doubt it will make any difference.
@mbrookes What do you think? It's something I was proposed with v4. But It seems to be controversial. This change won't really help people already using Material-UI.

It's kind of the same tradeoff we are facing at work with SEO, we have millions of pages indexed under a nonideal pathname. Does it worth it to change the URL introducing redirects and losing ranking during the transition?

[eps1lon]

@oliviertassinari What bug does this change address? Or do you consider the mutation a bug?

[oliviertassinari]

@eps1lon I'm removing the bug label.

[ryancogswell]

I suspect the bug label was related to my comment on the issue this closed: #12390 (comment)
I didn't log it as a separate issue, but I would consider the current side-effects that createMuiTheme can have prior to this change to be a bug.

[mbrookes]

@mbrookes What do you think? It's something I was proposed with v4. But It seems to be controversial. This change won't really help people already using Material-UI.

Sorry, missed this first time around. I don't have a strong opinion on it, but as breaking changes go, it would be a relatively minor one for existing users to have accommodate. Is it enough benefit to be worth it? I don't know.

[nthgol]

Hi all,
What version does this change get folded in? I was just about to report the mutation as a bug - it actually does create problems, such as if a user wants to customize their theme. Using a default (incomplete) theme object to initialize the app mutates it and makes it unusable if user wants to override just a few properties.

[oliviertassinari]

@nthgol It's planned for v4.

[nthgol]

@oliviertassinari using _.cloneDeep() on all arguments to createMuiTheme for now

[mbrookes]

Let's merge this for now, and we can consider the palette / color question separately for v4.