[core] Remove `makeStyles` dependency on `@material-ui/core/styles` #1627

mnajdova · 2021-05-11T14:45:54Z

Changes done in the PR:

Where there is no use of the theme in makeStyles I just changed the import from @material-ui/core/styles to @material-ui/styles.

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

Where there is theme usage, I've added the defaultTheme arg in the makeStyles options to be the result of createMuiTheme(). If there is another theme in the context, it will be picked up anyway by the makeStyles() util.

-import { makeStyles } from '@material-ui/core/styles';
+import { makeStyles } from '@material-ui/styles';
+import { createMuiTheme } from '@material-ui/core/styles';

+const defaultTheme = createMuiTheme();
 const useStyles = makeStyles((theme) => ({
   backgroundColor: theme.palette.primary.main,
-}));
+}), { defaultTheme });

…pdates

oliviertassinari · 2021-05-11T15:16:43Z

This one is going to be interesting. Data grid supports v4 and v5, which add complexity to this effort. The best option I can think of is to have a implicit transitive dependency on @material-ui/styles and ask v5 users to install the package manually. Why? Because @material-ui/styles is a singleton, a duplication of package in the bundle would mean losing the theme which will be a mess to debug and handle as support requests.

oliviertassinari

Good luck with this one, it will require to make tradeoffs.

oliviertassinari · 2021-05-11T15:18:46Z

docs/src/pages/components/data-grid/components/CustomPaginationGrid.tsx

@@ -1,5 +1,5 @@
 import * as React from 'react';
-import { makeStyles } from '@material-ui/core/styles';
+import { makeStyles } from '@material-ui/styles';


This is the docs for v4, this change is borderline but it can fly. Alternatives are probably worse.

packages/grid/_modules_/grid/components/GridPagination.tsx

mnajdova · 2021-05-11T15:36:30Z

This one is going to be interesting. Data grid supports v4 and v5, which add complexity to this effort. The best option I can think of is to have a implicit transitive dependency on @material-ui/styles and ask v5 users to install the package manually. Why? Because @material-ui/styles is a singleton, a duplication of package in the bundle would mean losing the theme which will be a mess to debug and handle as support requests.

Don't have a better proposal either. Seems like the best compromised to make it work with both versions... I also think, that v5 users will need to install @material-ui/styles anyway it to as it is used in the components too 😕

@oliviertassinari should we then list @material-ui/styles in peerDependencies too?

oliviertassinari · 2021-05-11T16:39:34Z

@oliviertassinari should we then list @material-ui/styles in peerDependencies too?

~~Yeah, sounds good~~ not sure, it would harm the DX for v4 users.

oliviertassinari · 2021-05-11T16:41:02Z

Also adding @DanailH as the last one to work on v4/v5 support.

We would need to open the changes in codesandbox and test that v5 works too.

docs/src/pages/components/data-grid/columns/RenderExpandCellGrid.tsx

packages/grid/x-grid-data-generator/src/renderer/renderPnl.tsx

DanailH · 2021-05-12T11:44:34Z

@mnajdova let me know how I can help with this effort. If I understood the problem correctly it looks like we would need a wrapper like the muiStyleAlpha util.

mnajdova · 2021-05-12T11:48:15Z

@mnajdova let me know how I can help with this effort. If I understood the problem correctly it looks like we would need a wrapper like the muiStyleAlpha util.

Just pushed the change :D Let me know if it looks correct. I didn't change the docs/ and packages/storybook files, is that correct?

DanailH · 2021-05-12T12:07:07Z

@mnajdova let me know how I can help with this effort. If I understood the problem correctly it looks like we would need a wrapper like the muiStyleAlpha util.

Just pushed the change :D Let me know if it looks correct. I didn't change the docs/ and packages/storybook files, is that correct?

Yes I don't think we need to change those since they are loaded under v4.

mnajdova · 2021-05-13T08:16:54Z

@dtassone @DanailH this should be ready for review. Also, see discussion for the dependencies change we will need to make (#1627 (comment)), to be honest not sure what will be the best way going forward. Not sure if it is critical at this moment, but would be good to have a plan on it.

dtassone · 2021-05-13T10:53:22Z

not sure I understand why there are 2 styles packages now

import { createMuiTheme, Theme } from '@material-ui/core/styles';
import { makeStyles } from '@material-ui/styles';

I would have expected.

import { makeStyles, createMuiTheme, Theme } from '@material-ui/styles';

mnajdova · 2021-05-13T11:11:12Z

not sure I understand why there are 2 styles packages now

import { createMuiTheme, Theme } from '@material-ui/core/styles';
import { makeStyles } from '@material-ui/styles';

I would have expected.

import { makeStyles, createMuiTheme, Theme } from '@material-ui/styles';

The @material-ui/styles are the legacy utilities that use JSS. We need to remove them from @material-ui/core. The @material-ui/core@v5.* will no longer have dependency on @material-ui/styles

On the other hand, all theme-related utils, like createTheme need to be part of the core this is why we have this separation.

dtassone · 2021-05-13T12:59:29Z

It seems that it is halfway done.

So if @material-ui/styles will go, I suggest we add a makeStyles in material-ui/core/styles in v5 that will have a different underlying implementation but the same signature as in V4. That way we are compatible with v4, v5. Just a silent change under the hood. Would that work?

import { makeStyles, createMuiTheme, Theme } from '@material-ui/core/styles';

mnajdova · 2021-05-13T13:40:56Z

It seems that it is halfway done.

So if @material-ui/styles will go, I suggest we add a makeStyles in material-ui/core/styles in v5 that will have a different underlying implementation but the same signature as in V4. That way we are compatible with v4, v5. Just a silent change under the hood. Would that work?

import { makeStyles, createMuiTheme, Theme } from '@material-ui/core/styles';

We won’t support this API in v5 (at least not as utils from core using emotion), we plan to support the styled() and sx api for customization.

dtassone · 2021-05-13T13:55:55Z

It seems that it is halfway done.
So if @material-ui/styles will go, I suggest we add a makeStyles in material-ui/core/styles in v5 that will have a different underlying implementation but the same signature as in V4. That way we are compatible with v4, v5. Just a silent change under the hood. Would that work?
import { makeStyles, createMuiTheme, Theme } from '@material-ui/core/styles';

We won’t support this API in v5 (at least not as utils from core using emotion), we plan to support the styled() and sx api for customization.

We could add it, for compatibility reason, and maybe mark it as deprecated in the first minor or something.
So the grid and other components can be compatible with v4 and v5 without any dependency issue.

DanailH · 2021-05-13T14:51:21Z

If I'm understanding this correctly the DataGrid component is an expectation to the case where a component needs to be v4 and v5 compatible at the same time, right?

Also, I remember when we were adding the v5 support it was always meant to be temporary and once v5 is released we would migrate the DataGrid fully as well.

If that is the case then we can have it as it is proposed for now and remove the makeStyles once we are fully migrated.

mnajdova · 2021-05-13T15:12:28Z

We could add it, for compatibility reason, and maybe mark it as deprecated in the first minor or something.
So the grid and other components can be compatible with v4 and v5 without any dependency issue.

I should check whether and how this will be possible, I am not sure if it is trivial change. Even if we add it I would mark it as deprecated, as I don’t think the core should in the long run support this. cc @oliviertassinari @eps1lon for opinion or ideas

mnajdova · 2021-05-13T15:13:42Z

Also, I remember when we were adding the v5 support it was always meant to be temporary and once v5 is released we would migrate the DataGrid fully as well.

I didn’t know this, I thought the Grid should always support both versions.

oliviertassinari · 2021-05-13T15:30:49Z

for opinion or ideas

Looking at the thread it seems that we need to precise the direction. It seems that there is confusion around this point: the intent is to progressively remove makeStyles and withStyles. The changes pushed here are meant to meet halfway between the need to drop @material-ui/styles and not to break the data grid for v4 users until v5 is in a beta state. From a timing perspective, I believe that the intended steps were so far:

@material-ui/data-grid depends on @material-ui/styles over @material-ui/core for the styles.
@material-ui/core v5 removes all dependencies on JSS, JSS is isolated on @material-ui/styles
@material-ui/core v5-beta is released
@material-ui/data-grid drops support for v4, depends on v5-beta.0.
@material-ui/data-grd rewrite the styles to depend on the styled() API with em/sc

Do we want to commit to it?

Regarding making it easy for third-party libraries to transition from v4 to v5, or even supporting them both for a couple of months. It seems that we have 3 options:

rely on @material-ui/styles/makeStyles
rely on @material-ui/core/styled (not sure if it works, but it's likely)
introduce @material-ui/core/makeStyles back in v5 but powered by emotion, not JSS (sc support not possible)

dtassone · 2021-05-13T16:28:36Z

introduce @material-ui/core/makeStyles back in v5 but powered by emotion, not JSS (sc support not possible)
I would mark it as deprecated

it seems like an option 🤔 .

v4 users don't see any issues.
when they migrate to v5 and they still use this version of the grid that support both then they will see the deprecation warning? (maybe we could silent it in the grid and leave it for users...)
v5 goes live, we refactor the grid and replace makeStyles with styled, and we stop supporting v4 versions?

Another option would be to add the styled api in v4, and use styled in the grid, and silently move to v5.

The styled would be the jss implementation, and map to makeStyle
This avoids having the old api in v5, and deprecation warning.

mnajdova · 2021-05-14T13:00:20Z

I will look into adding makeStyles powered by emotion. Definitely deprecated, or even marked as private_?

mnajdova · 2021-05-14T13:01:04Z

rely on @material-ui/styles/makeStyles

@oliviertassinari seems like this is not possible because of the JSS singleton? Or am I missing something?

mnajdova · 2021-05-14T16:47:02Z

From the first look, supporting makeStyles with emotion is not that straight forward, especially for the use-cases we have, like overriding styles implemented by styled(), supporting SSR etc.

I did some exploration today - https://codesandbox.io/s/makestyles-with-emotion-zfeyx?file=/src/App.tsx (the solution is inspired by emotion-js/emotion#1853 (comment), only the API is adjusted to our use-cases). The first problem I have is that I cannot override styles defined by the styled() API which is the first use-cases we will have (it may be possible by using ClassName's cx prop not sure it would fly well, as we would need to use it in all core components, and require using provider at top) :( I haven't even started looking into SSR...

Taking into account that this is only needed for like very short period of time (transition), I am not sure how smart it is to invest too much time into making it all work when anyway don't plan to support it in the long run.

So maybe we should look more Option 1 - rely on @material-ui/styles/makeStyles.

Another option would be to add the styled api in v4, and use styled in the grid, and silently move to v5.

The styled would be the jss implementation, and map to makeStyle
This avoids having the old api in v5, and deprecation warning.

styled() and makeStyles() have different APIs, not sure how this would work. Maybe we can first mirgate all codebase (material-ui-x) to use styled() instead of makeStyles() and then we would use the experimentalStyled() in v5?

oliviertassinari · 2021-05-14T18:33:18Z

@oliviertassinari seems like this is not possible because of the JSS singleton? Or am I missing something?

@mnajdova I believe that it's option 1, what you have initially pushed in this PR. For the data grid, it seems good enough,

Thanks for exploring option 3. For my perspective, the biggest value of makeStyles powered by emotion would be for members of the community.

Did we consider option 2?

mnajdova · 2021-05-14T18:43:16Z

Did we consider option 2?

Haven’t tried it yet. Looks like it could be easier then to migrate to the current experimentalStyled().

I didn’t go in this direction, as when we will get to withStyles usages we will then again get in this situation, and replacing it won’t be that simple (what we have currently in core for migrating to emotion). That’s why in the end I think it’s easiest to go with Option 1.

mnajdova · 2021-05-17T11:57:55Z

@dtassone @DanailH would be great if I can get a review, I need to resolve merge conflicts each time there is a commit on master. Would suggest reviewing with hidden white spaces changes - https://github.com/mui-org/material-ui-x/pull/1627/files?diff=split&w=1

oliviertassinari · 2021-05-17T16:11:42Z

It works with core v5 👍 https://codesandbox.io/s/material-demo-forked-jl74j?file=/demo.js

mnajdova · 2021-05-18T08:43:32Z

Resolving last conflicts and merging. Thanks everyone on the input :)

mnajdova added 2 commits May 11, 2021 16:43

init

5938bdd

Merge remote-tracking branch 'upstream/master' into feat/makeStyles-u…

15816c4

…pdates

oliviertassinari reviewed May 11, 2021

View reviewed changes

more changes

b363e95

mnajdova added 2 commits May 11, 2021 17:38

import

6814c38

Theme update

3113a87

mnajdova marked this pull request as ready for review May 11, 2021 16:02

mnajdova requested a review from dtassone May 11, 2021 16:04

oliviertassinari requested a review from DanailH May 11, 2021 16:40

oliviertassinari added the v5.x label May 11, 2021

oliviertassinari reviewed May 11, 2021

View reviewed changes

docs/src/pages/components/data-grid/columns/RenderExpandCellGrid.tsx Outdated Show resolved Hide resolved

packages/grid/x-grid-data-generator/src/renderer/renderPnl.tsx Outdated Show resolved Hide resolved

move createStyles too

7d8024f

createTheme correct method based on version

0200537

typo

7081fe5

oliviertassinari mentioned this pull request May 12, 2021

[docs] Improve the data grid components page #1382

Merged

9 tasks

mnajdova added 2 commits May 13, 2021 08:41

Merge branch 'upstream-master' into feat/makeStyles-updates

780c2b2

prettier

0bb6d6f

mnajdova added 2 commits May 17, 2021 12:50

Merge branch 'master' into feat/makeStyles-updates

6070aff

more changes

168c705

dtassone approved these changes May 17, 2021

View reviewed changes

mnajdova added 2 commits May 18, 2021 10:46

Merge branch 'master' into feat/makeStyles-updates

2b8f2a8

merge conflicts

25732d9

DanailH approved these changes May 18, 2021

View reviewed changes

merge conflicts

df11fa8

mnajdova merged commit 40ebf9b into mui:master May 18, 2021

mnajdova mentioned this pull request May 18, 2021

[core] Remove withStyles dependency on @material-ui/core/styles #1690

Merged

oliviertassinari mentioned this pull request May 24, 2021

[DataGrid] Update to latest version breaks #1761

Closed

2 tasks

oliviertassinari mentioned this pull request Jun 4, 2021

Fix support for next version of @material-ui/core iamhosseindhv/notistack#387

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[core] Remove `makeStyles` dependency on `@material-ui/core/styles` #1627

[core] Remove `makeStyles` dependency on `@material-ui/core/styles` #1627

mnajdova commented May 11, 2021 •

edited

Loading

oliviertassinari commented May 11, 2021

oliviertassinari left a comment

oliviertassinari May 11, 2021 •

edited

Loading

mnajdova commented May 11, 2021 •

edited

Loading

oliviertassinari commented May 11, 2021 •

edited

Loading

oliviertassinari commented May 11, 2021

DanailH commented May 12, 2021

mnajdova commented May 12, 2021

DanailH commented May 12, 2021

mnajdova commented May 13, 2021

dtassone commented May 13, 2021

mnajdova commented May 13, 2021

dtassone commented May 13, 2021 •

edited

Loading

mnajdova commented May 13, 2021

dtassone commented May 13, 2021

DanailH commented May 13, 2021

mnajdova commented May 13, 2021

mnajdova commented May 13, 2021

oliviertassinari commented May 13, 2021 •

edited

Loading

dtassone commented May 13, 2021 •

edited

Loading

mnajdova commented May 14, 2021

mnajdova commented May 14, 2021

mnajdova commented May 14, 2021

oliviertassinari commented May 14, 2021

mnajdova commented May 14, 2021

mnajdova commented May 17, 2021

oliviertassinari commented May 17, 2021 •

edited

Loading

mnajdova commented May 18, 2021

[core] Remove makeStyles dependency on @material-ui/core/styles #1627

[core] Remove makeStyles dependency on @material-ui/core/styles #1627

Conversation

mnajdova commented May 11, 2021 • edited Loading

oliviertassinari commented May 11, 2021

oliviertassinari left a comment

Choose a reason for hiding this comment

oliviertassinari May 11, 2021 • edited Loading

Choose a reason for hiding this comment

mnajdova commented May 11, 2021 • edited Loading

oliviertassinari commented May 11, 2021 • edited Loading

oliviertassinari commented May 11, 2021

DanailH commented May 12, 2021

mnajdova commented May 12, 2021

DanailH commented May 12, 2021

mnajdova commented May 13, 2021

dtassone commented May 13, 2021

mnajdova commented May 13, 2021

dtassone commented May 13, 2021 • edited Loading

mnajdova commented May 13, 2021

dtassone commented May 13, 2021

DanailH commented May 13, 2021

mnajdova commented May 13, 2021

mnajdova commented May 13, 2021

oliviertassinari commented May 13, 2021 • edited Loading

dtassone commented May 13, 2021 • edited Loading

mnajdova commented May 14, 2021

mnajdova commented May 14, 2021

mnajdova commented May 14, 2021

oliviertassinari commented May 14, 2021

mnajdova commented May 14, 2021

mnajdova commented May 17, 2021

oliviertassinari commented May 17, 2021 • edited Loading

mnajdova commented May 18, 2021

[core] Remove `makeStyles` dependency on `@material-ui/core/styles` #1627

[core] Remove `makeStyles` dependency on `@material-ui/core/styles` #1627

mnajdova commented May 11, 2021 •

edited

Loading

oliviertassinari May 11, 2021 •

edited

Loading

mnajdova commented May 11, 2021 •

edited

Loading

oliviertassinari commented May 11, 2021 •

edited

Loading

dtassone commented May 13, 2021 •

edited

Loading

oliviertassinari commented May 13, 2021 •

edited

Loading

dtassone commented May 13, 2021 •

edited

Loading

oliviertassinari commented May 17, 2021 •

edited

Loading