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

[docs] withTheme is missing #27199

Closed
2 tasks done
strangedev opened this issue Jul 9, 2021 · 9 comments · Fixed by #27427
Closed
2 tasks done

[docs] withTheme is missing #27199

strangedev opened this issue Jul 9, 2021 · 9 comments · Fixed by #27427
Labels
docs Improvements or additions to the documentation ready to take Help wanted. Guidance available. There is a high chance the change will be accepted

Comments

@strangedev
Copy link

  • The issue is present in the latest release.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Current Behavior 😯

The v5 documentation shows the withTheme function which injects the Theme as a prop into a component. The function is not available in @material-ui/core/styles as per the documentation. @material-ui/styled-engine exports withTheme, but it seems to be a type only and not the actual function.

Expected Behavior 🤔

I should either be able to use the withTheme function, or the docs should be updated.

Steps to Reproduce 🕹

I've implemented a small example in a next.js application: https://github.com/strangedev/mui5-sc-bugs/blob/with-theme-missing/src/components/WithTheme.tsx

When trying to run with npm run dev, you can see that withTheme from @material-ui/styled-engine is not a function.
Try changing the import to any of the mentioned modules to show that the function is indeed missing.

Context 🔦

I'm migrating from v5 alpha with styled-components as an additional styling solution to v5 beta with styled-components as styled-engine for MUI.

Your Environment 🌎

`npx @material-ui/envinfo`
  System:
    OS: Linux 5.12 Arch Linux
  Binaries:
    Node: 14.16.0 - ~/.nvm/versions/node/v14.16.0/bin/node
    Yarn: 1.22.10 - /usr/bin/yarn
    npm: 6.14.11 - ~/.nvm/versions/node/v14.16.0/bin/npm
  Browsers:
    Chrome: Not Found
    Firefox: 89.0.2
  npmPackages:
    @material-ui/core: 5.0.0-beta.0 => 5.0.0-beta.0 
    @material-ui/private-theming:  5.0.0-beta.0 
    @material-ui/styled-engine-sc: 5.0.0-beta.0 => 5.0.0-beta.0 
    @material-ui/system:  5.0.0-beta.0 
    @material-ui/types:  6.0.1 
    @material-ui/unstyled:  5.0.0-alpha.39 
    @material-ui/utils:  5.0.0-beta.0 
    @types/react: 17.0.13 => 17.0.13 
    react: 17.0.2 => 17.0.2 
    react-dom: 17.0.2 => 17.0.2 
    styled-components: 5.3.0 => 5.3.0 
    typescript: 4.3.5 => 4.3.5 
@strangedev strangedev added the status: waiting for maintainer These issues haven't been looked at yet by a maintainer label Jul 9, 2021
@mnajdova
Copy link
Member

mnajdova commented Jul 9, 2021

Thanks for the report! The documentation should be updated. It should be

index 073583e080..ab1d0efb56 100644
--- a/docs/src/pages/styles/advanced/advanced.md
+++ b/docs/src/pages/styles/advanced/advanced.md
@@ -51,7 +51,7 @@ function DeepChild() {
 For use in class or function components:

 ```jsx
-import { withTheme } from '@material-ui/core/styles';
+import { withTheme } from '@material-ui/styles';

 function DeepChildRaw(props) {
   return <span>{`spacing ${props.theme.spacing}`}</span>;

Note that any documentation inside https://next.material-ui.com/styles/api is related to the legacy JSS styling package @material-ui/styles. We plan to deprecate this package in v5.

Regarding the withTheme utility, we discussed this in #26051 and decided not to introduce this for the new styling engine, as it is pretty trivial and if needed developers can implement it on their side.

@mnajdova mnajdova added docs Improvements or additions to the documentation OCD21 and removed status: waiting for maintainer These issues haven't been looked at yet by a maintainer labels Jul 9, 2021
@strangedev
Copy link
Author

Even if this is trivial, it would be nice not to have to add the same code to every project that uses MUI. Unfortunately, the withTheme function provided by styled-components is not compatible with MUI's styled, so we can't use that.

Actually, I found implementing this in TypeScript to be surprisingly difficult. Trying to remove the theme property from a component and adding it back in results in an incompatible type:

import React, { FunctionComponent, ReactElement } from 'react';
import { Theme, useTheme } from '@material-ui/core/styles';

type WithoutTheme<TProps> = Omit<TProps, 'theme'>;

const withTheme = function<TProps extends { theme: Theme }> (Component: FunctionComponent<TProps>): FunctionComponent<WithoutTheme<TProps>> {
  return (props: WithoutTheme<TProps>): ReactElement => {
    const theme = useTheme();

    return (
      <Component theme={ theme } { ...props } />
    );
  };
};

export {
  withTheme
};

This results in the following error:

Type '{ theme: Theme; } & WithoutTheme<TProps>' is not assignable to type 'IntrinsicAttributes & TProps & { children?: ReactNode; }'.
  Type '{ theme: Theme; } & WithoutTheme<TProps>' is not assignable to type 'TProps'.
    '{ theme: Theme; } & WithoutTheme<TProps>' is assignable to the constraint of type 'TProps', but 'TProps' could be instantiated with a different subtype of constraint '{ theme: Theme; }'.ts(2322)

The solution is to use any, which is a hacky solution at best:

import { useTheme } from '@material-ui/core/styles';
import React, { FunctionComponent, ReactElement } from 'react';

type WithoutTheme<TProps> = Omit<TProps, 'theme'>;

const withTheme = function<TProps extends { theme: Theme }> (Component: FunctionComponent<TProps>): FunctionComponent<WithoutTheme<TProps>> {
  return (props: any): ReactElement => {
    const theme = useTheme();

    return (
      <Component theme={ theme } { ...props } />
    );
  };
};

export {
  withTheme
};

Adding this function back in would save a lot of people a lot of headaches. The state of styled-components support in MUI5 does not feel very cohesive at the moment and not having familiar helpers like this one adds to that.

@strangedev
Copy link
Author

Maybe I don't understand the use case behind using styled-components with MUI. As it is at the moment, I can't see how it is useful.

On one hand, we can't use styled from styled-components directly, as this messes with the load order of MUI styles and custom styles. Maybe I'm missing something here and it's just an issue of incomplete docs, but in my experience so far it seems like it's not deterministic in what order styles are loaded.

On the other hand, when we use styled provided by MUI, the API is sufficiently different from styled-components so that we can't use our existing code with it. If we have to change our code to work with the MUI/emotion API, we might as well fully migrate to emotion and drop styled-components.

I suspect that using styled-components alongside MUI is not really how MUI should be used, seeing how it has a CSS API and sx. So maybe it isn't something that should be done. I just got the impression that v5 would facilitate this use case and after many days of working on the migration, we haven't really got to any usable state.

@mnajdova
Copy link
Member

mnajdova commented Jul 9, 2021

Maybe I don't understand the use case behind using styled-components with MUI. As it is at the moment, I can't see how it is useful.

On one hand, we can't use styled from styled-components directly, as this messes with the load order of MUI styles and custom styles. Maybe I'm missing something here and it's just an issue of incomplete docs, but in my experience so far it seems like it's not deterministic in what order styles are loaded.

You can use styled() from styled-components directly at any moment. If you are using styled-components as a style engine, everything will be working as expected. If you are using emotion as a style engine, then yes, you need to configure the injection order as described in https://next.material-ui.com/guides/interoperability/#css-injection-order

On the other hand, when we use styled provided by MUI, the API is sufficiently different from styled-components so that we can't use our existing code with it. If we have to change our code to work with the MUI/emotion API, we might as well fully migrate to emotion and drop styled-components.

Can you explain what do you mean by "sufficiently different"? Again, you can still use styled-components's styled() API, no need to migrate to MUI's if you don't need any of the extra features it offers. See https://next.material-ui.com/system/styled/

I suspect that using styled-components alongside MUI is not really how MUI should be used, seeing how it has a CSS API and sx. So maybe it isn't something that should be done. I just got the impression that v5 would facilitate this use case and after many days of working on the migration, we haven't really got to any usable state.

You can use styled-components as a styling engine (without emotion) and still have the sx prop. What do you mean by css API?

I just got the impression that v5 would facilitate this use case and after many days of working on the migration, we haven't really got to any usable state.

Can you share some pain points? We are working on a codemod for the v5 migration. Any feedback is more than welcomed.


@strangedev would be great if you can open a dedicated issue about the pain points so that we can discussed then in a more structured manner.

@strangedev
Copy link
Author

Thanks for your answer @mnajdova, the advice is very much appreciated 😊

It seems we were not fully aware of how to integrate v5.

Might be that the behavior we're experiencing is related to the aliasing with webpack not working as expected in Next.js. I will test it again with a smaller codebase and get back to you in a separate issue if we're still experiencing problems.

@oliviertassinari
Copy link
Member

oliviertassinari commented Jul 9, 2021

aliasing with webpack not working as expected in Next.js

@strangedev On the same topic: #27087.

Do you have a specific reason for using withTheme?

@strangedev
Copy link
Author

Do you have a specific reason for using withTheme?

None that couldn't ultimately be replaced with useTheme. We're working with a larger codebase at the moment so our first approach was not touching too many places.

@siriwatknp
Copy link
Member

@oliviertassinari what do you think if we bring withTheme in core/styles back? because I think

  • withTheme is for class component
  • useTheme is for fn component

@oliviertassinari oliviertassinari added the ready to take Help wanted. Guidance available. There is a high chance the change will be accepted label Jul 16, 2021
@oliviertassinari
Copy link
Member

@siriwatknp I think that we can wait for pushbacks from the community. So far, this issue seems to be about the wrong link in the documentation. If we can make it harder for developers to use class components, we might even argue that it's healthy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs Improvements or additions to the documentation ready to take Help wanted. Guidance available. There is a high chance the change will be accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants