[styles] Allow CSS properties to be functions

[merceyz]

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

• Updated type definition to let CSS properties be functions that returns a value

Fixes #14814
Fixes #16007

[mui-pr-bot]

No bundle size changes comparing 2e78390...4b05497

Generated by 🚫 dangerJS against 4b05497

[eps1lon]

Can you add TS demos for https://next.material-ui.com/css-in-js/basics/#adapting-the-styled-components-api and https://next.material-ui.com/css-in-js/basics/#adapting-the-higher-order-component-api as well?

Looks like this change is required for these to work. This would mean we have a test for the change.

[merceyz]

Can you add TS demos for https://next.material-ui.com/css-in-js/basics/#adapting-the-styled-components-api and https://next.material-ui.com/css-in-js/basics/#adapting-the-higher-order-component-api as well?

Yes, though it's blocked by #15501 which is blocked by this PR. So the demos should probably be in their own PR

[eps1lon]

@merceyz Alright #15501 should be green. You can base this PR on #15501 and then we'll see if this worked.

This PR should also bring back docs/src/pages/css-in-js/basics/AdaptingHook.tsx

[merceyz]

@eps1lon Argos complaining now, everything else passed though so I'll wait for #15501 to be merged and then reset

[eps1lon]

I don't think it has any impact whether we use object or {} as the base type for Props. What object tries to convey is that a plain object should be passed which we can't type check with TypeScript so we just communicate the requirement.

Rest looks awesome.

[merceyz]

@eps1lon I noticed that core has it's own CSSProperties interface, tried to replace it with this one but couldn't. Can you take a look?

[eps1lon]

Right. I was wondering why this was working now (I thought I tried it before) but it only works because @material-ui/styles has very few test cases. As soon as you test this approach more thoroughly it breaks.

The problem is that there are just way to many overloads and for structural types it somehow breaks down because at some point the compiler doesn't know if it is themed based or props based.

I somewhat gave up on typing every possible pattern. We support props based styling on the class key level which should cover most of the use cases. If you can make this work for the current test suite in @material-ui/core I'm happy to merge. Right now it breaks too many common usage patterns.

[oliviertassinari]

Should we close the pull request, for now?

[mui-pr-bot]

No bundle size changes comparing f246dbd...912b97f

Generated by 🚫 dangerJS against 912b97f

[merceyz]

Right. I was wondering why this was working now (I thought I tried it before) but it only works because @material-ui/styles has very few test cases. As soon as you test this approach more thoroughly it breaks.

@eps1lon I didn't have time to look into this properly until now, but I think you're wrong here.
makeStyles from @material-ui/core is the exact same as the one from @material-ui/styles

As I didn't update createStyles in @material-ui/core, it only works with makeStyles

[eps1lon]

I didn't have time to look into this properly until now, but I think you're wrong here.
makeStyles from @material-ui/core is the exact same as the one from @material-ui/styles

About what? You can look at the repo and you can compare the amount of test in packages/material-ui/test/typescript/styles.spec vs packages/material-ui-styles/test/styles.spec and find that the core has way more test cases.

I'm not sure you're aware what is typed where currently i.e. core/styles/withStyles has a different definition compared to styles/withStyles which is why changing styles/withStyles doesn't cause breakage because it doesn't have the same tests as core/styles/withStyles.

Until we can consolidate both definitions we shouldn't make changes to the implementations because we don't have a clear picture what is tested.

[merceyz]

@eps1lon You're right, my bad. Working on it now.

[merceyz]

@eps1lon That should do it

[merceyz]

@eps1lon Requesting a review

[eps1lon]

I'd like to consolidate the types and tests for mui/core and mui/core/styles first to have an overview if something is already broken between core/styles and styles. Then we can move forward.

[eps1lon]

Very nice. Took the liberty and named some remaining unnamed generic arguments when we expect a certain type. T, U etc only make sense for truly generic types e.g Pick, Omit

[eps1lon]

@merceyz Could you take a look at 912b97f? Hope this improves backward compatibility.

[merceyz]

@eps1lon Since props is optional, that should keep it backwards compatible. Should we add a comment to switch it for V5?
Otherwise looks good

[eps1lon]

It's only a minor inconvenience/unelegant code. Nothing that warrants a breaking change in the future. Maybe that'll change. Let's leave this for now and see how it goes.