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

[typescript] components loose component prop with emotion or styled-component #15695

Closed
2 tasks done
Mario-Eis opened this issue May 14, 2019 · 15 comments
Closed
2 tasks done
Labels
package: styled-engine Specific to @mui/styled-engine typescript

Comments

@Mario-Eis
Copy link

Mario-Eis commented May 14, 2019

When using styled components >=4.1.0 or emotion >=10 there will be type errors if material ui components are styled.

  • This is not a v0.x issue.
  • I have searched the issues of this repository and believe that this is not a duplicate.

Expected Behavior 🤔

No build or type errors are expected when styling material ui components with styled components or emotion.

Current Behavior 😯

A lot of type errors like

TS2740: Type '{ children: string; }' is missing the following properties from type 'Pick<Pick<(Pick<PaperProps, "style" | "title" | "children" | "className" | "color" | "id" | "lang" | "role" | "tabIndex" | "elevation" | "aria-activedescendant" | "aria-atomic" | ... 245 more ... | "innerRef"> & RefAttributes<...>) | PropsWithChildren<...>, "ref
" | ... 257 more ... | "innerRef"> & Partial<...>, "ref...': ref, style, title, className, and 254 more.

or

TS2322: Type '{ children: Element; component: string; }' is not assignable to type 'IntrinsicAttributes & Pick<Pick<DefaultComponentProps<{ props: { dense?: boolean | undefined; disablePadding?: boolean | undefined; subheader?: ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<...>)> | n
ull) | (new (props: any) => Component<...>)> | undefine...'.
  Property 'component' does not exist on type 'IntrinsicAttributes & Pick<Pick<DefaultComponentProps<{ props: { dense?: boolean | undefined; disablePadding?: boolean | undefined; subheader?: ReactElement<any, string | ((props: any) => ReactElement<any, string | ... | (new (props: any) => Component<...>)> | null) | (new (props: any) => Component<...>)
> | undefine...'.

Steps to Reproduce 🕹

  1. Checkout https://github.com/Mario-Eis/materialui-styled-test and npm install
  2. Run npm script 'serve-dev' --> localhost:3000
  3. Build errors are readable in the console.

I don't know for sure, but I think there could be two different issues in the demo.
The first is a simple styled Paper component. When downgrading to styled components <=4.0.3, this issue will be fixed.
The second one is a styled List component. Styled List components will work, as long as there are no prop attributes set. If e.g. component="nav" is added, there will be an error. Downgrading to styled-components 4.0.3 won't work here. But downgrading to material ui 3.9.2, will.

Both cases are shown in the demo.

Context 🔦

I want to style material ui components using styled-components library.

Your Environment 🌎

Tech Version
Material-UI 4.0.0
React 16.8.17
TypeScript 3.4.5
@oliviertassinari oliviertassinari added the priority: important This change can make a difference label May 15, 2019
@Mario-Eis Mario-Eis changed the title [typescript] Material UI v4-beta components are incompatible to emotion and styled-components [typescript] Material UI v4.0.0 components are incompatible to emotion and styled-components May 24, 2019
@eps1lon eps1lon removed the priority: important This change can make a difference label Jun 2, 2019
@eps1lon
Copy link
Member

eps1lon commented Jun 2, 2019

This is a limitation of typescript. See #15759 (comment) for the recommended solution i.e. casting the wrapped component back to the original type. I understand this is not ideal but there are other limitations in typescript that block other approaches. For these simple applications where props available from the HOC are of no interest type casting seems good enough. For more complex examples follow #15827 which should have a proposal by the end of this week.

@eps1lon eps1lon changed the title [typescript] Material UI v4.0.0 components are incompatible to emotion and styled-components [typescript] Material UI v4.0.0 components loose component prop with emotion or styled-component Jun 2, 2019
@eps1lon eps1lon added external dependency Blocked by external dependency, we can’t do anything about it out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) labels Jun 2, 2019
@Mario-Eis
Copy link
Author

Mario-Eis commented Jun 3, 2019

Casting back to the original type would be possible. But its a hard change. We have a large code base. And adding casts to each and every component could break the upgrade for us.

I also tried to implement the type cast to the demo under https://github.com/Mario-Eis/materialui-styled-test.

The good news: The issue shown in the demo with the Paper Component, seems to be fixed.

Unfortunately

const StyledList = styled(List)`
    background-color: blue;
    text-align: center;
` as List;

results in

TS2749: 'List' refers to a value, but is being used as a type here.

I pushed the workaround to the demo repo.

List is now defined as a variable.

declare const List: OverridableComponent<{
  props: {
    dense?: boolean;
    disablePadding?: boolean;
    subheader?: React.ReactElement;
  };
  defaultComponent: 'ul';
  classKey: ListClassKey;
}>;

The workaround doesn't seem to work any more.

@eps1lon
Copy link
Member

eps1lon commented Jun 3, 2019

const StyledList = styled(List)`
    background-color: blue;
    text-align: center;
` as typeof List;

Seems codemodable. Though you have to be aware that any type information about props handled by styled is lost.

@Mario-Eis
Copy link
Author

Thanks! I updated the demo repo. It compiles without errors now.
Only the IDE (IntelliJ) shows a warning that there is a missing required attribute "component". But I think that will be another issue. Will investigate that.

@IvanGrimes
Copy link

@Mario-Eis
Hello.
This should help:

const StyledList = styled<ComponentType<ListProps>>(List)`
    background-color: blue;
    text-align: center;
`;

@oliviertassinari oliviertassinari changed the title [typescript] Material UI v4.0.0 components loose component prop with emotion or styled-component [typescript] components loose component prop with emotion or styled-component Dec 1, 2019
@oliviertassinari
Copy link
Member

Closing as it was labeled as a limitation of TypeScript and has a workaround.

@bengry
Copy link

bengry commented Dec 2, 2019

@oliviertassinari why shouldn't the workaround be part of MUI itself though?

@michaelbayday
Copy link

interested in this :(

@oliviertassinari
Copy link
Member

why shouldn't the workaround be part of MUI itself though?

@bengry If you have proposals on how it could be implemented, please let us know.

@eps1lon eps1lon added package: styled-engine Specific to @mui/styled-engine and removed out of scope The problem looks valid but we won't fix it (maybe we will revisit it in the future) external dependency Blocked by external dependency, we can’t do anything about it labels Sep 18, 2021
@enrique-ramirez
Copy link
Contributor

@oliviertassinari @eps1lon I've been doing this to prevent this issue:

import { Typography } from '@mui/material'
import { createStyled } from '@mui/system'

// Some theme props here
const defaultTheme = {}

const styled = createStyled({ defaultTheme });

const StyledBodyText = styled(Typography)`
  margin-bottom: 12px;
` as typeof Typography;

This works well if I don't want to extend the typography props. However, let's say I want to create a Label component, which is just a typography component with some background color depending on a prop:

import { Typography } from '@mui/material'
import { createStyled } from '@mui/system'

// Some theme props here
const defaultTheme = {}

const styled = createStyled({ defaultTheme });

enum LabelType {
  clothes = 'clothes',
  electronics = 'electronics',
}

interface LabelProps extends TypographyProps {
  type?: LabelType;
}

const styled(Typography)<LabelProps>(({ theme, type = LabelType.Primary }) => {
  // Here I use the `type` prop to generate different background colors. 
  // In the end, I just return an object of styles, but some may be conditional.
  // The point is: I need the `type` prop to generate specific styles.
  return {...MyStyles}
});

This works, but I completely lose the component prop. If I try to do the typecasting like this:

const styled(Typography)<LabelProps>(({ theme, type = LabelType.Primary }) => {
  // Here I use the `type` prop to generate different background colors. 
  // In the end, I just return an object of styles, but some may be conditional.
  // The point is: I need the `type` prop to generate specific styles.
  return {...MyStyles}
}) as typeof Typography;

I get a No overload matches this call. error when attempting to use the type prop.

What's the best way to deal with this scenario? Sorry, it's probably real simple but I've struggled with this for a while and am on the verge of just using any...

@aldrichdev
Copy link

const StyledList = styled(List)`
    background-color: blue;
    text-align: center;
` as typeof List;

Seems codemodable. Though you have to be aware that any type information about props handled by styled is lost.

That solution doesn't work for me. I still get a warning when trying to add the component prop: Type 'string' is not assignable to type 'ElementType<any>'.

@aldrichdev
Copy link

aldrichdev commented Nov 24, 2021

const StyledList = styled(List)`
    background-color: blue;
    text-align: center;
` as typeof List;

Seems codemodable. Though you have to be aware that any type information about props handled by styled is lost.

That solution doesn't work for me. I still get a warning when trying to add the component prop: Type 'string' is not assignable to type 'ElementType<any>'.

Think I figured this out on my own. I think it wasn't assignable because I was saying any old string value could be used for that prop value, but it specifically needs to be a string that contains the name of an HTML element. Like 'h4' would be okay but TypeScript didn't know if I would pass something that's not an element name, like 'paul'. So I changed my prop type to this, and it works:

typographyComponent?: 'h1' | 'h2' | 'h3' | 'h4' | 'h5' | 'h6' | 'label' | 'span'

@jonbnewman
Copy link

jonbnewman commented Jan 27, 2022

@enrique-ramirez were you able to identify a solution to this issue? This is a major problem for me as I need to pass props into styled components while using an alternate component value.

@javier-zabala-lenio
Copy link

@enrique-ramirez Did you find a solution? I'm currently facing the same issue.

@zirkelc
Copy link

zirkelc commented Feb 1, 2022

@enrique-ramirez Did you find a solution? I'm currently facing the same issue.

These two solutions worked for me:

const FireNav = styled(List)<{ component?: React.ElementType }>({ });
const StyledList = styled(List)`
    background-color: blue;
    text-align: center;
` as typeof List;

thenick775 pushed a commit to thenick775/gbajs3 that referenced this issue Jan 4, 2024
- on 6.1.6 seeing issues similar to:
   - styled-components/styled-components#4062
   -  mui/material-ui#15695

- downgrades styled-components
thenick775 pushed a commit to thenick775/gbajs3 that referenced this issue Jan 28, 2024
- on 6.1.6 seeing issues similar to:
   - styled-components/styled-components#4062
   -  mui/material-ui#15695

- downgrades styled-components
thenick775 added a commit to thenick775/gbajs3 that referenced this issue Feb 5, 2024
* feat: initial vitest setup

* feat: starting on shared component tests

* feat: add testing library linting

* feat: add jest dom linting

* feat: shared components tests

* feat: tests for nav component/leaf

* refactor: exclude wasm dir from coverage

* fix: styled components dropping types

- on 6.1.6 seeing issues similar to:
   - styled-components/styled-components#4062
   -  mui/material-ui#15695

- downgrades styled-components

* fix: update styled components, proper rnd prop passing

- sourced from: https://stackoverflow.com/a/76623553

- had issues with function parameter prop inference with >6.1.1

* refactor: exclude test dir from coverage

- not necessary

* feat: modal body and footer tests

* feat: include test files when using eslint

* feat: wrap all context access, more tests, mocking strategy

- not sure if im sold on how I'm mocking so far, but this is food for thought

- seems like this may be the best way for me to interact with the global context in tests

* feat: exclude files from coverage

- maybe use include if this list grows any more

* feat: tests for context access wrapper

* feat: virtual controls form tests

- tentative impl for tests using media queries

- package updates

- snapshot updates due to styled components update

* refactor: fix spy/mock reset, virtual controls render initlal values from storage if provided

* feat: keybindings form spec

* feat: download save spec

- fixes download mime type

- remove dynamic anchor

- adds tests

* feat: local local rom spec

* feat: testing package updates

* refactor: define env vars in .d.ts

* feat: navigation menu spec, add msw

- adds navigation menu tests

- adds msw

- adds auth provider to render with context

* feat: bump msw

* feat: load rom/save specs

* feat: upload/rom save to server specs

* feat: login form spec

* feat: file system modal spec

* feat: modal container spec

- modal container spec

- bumps testing deps

* feat: pwa prompt spec

* chore: periodic package updates

* refactor: keybindings form validation assertions

* refactor: aria label case

* feat: save states spec

* chore: dedupe packages

* fix: jsdom latency

- see: jsdom/jsdom#3659

* feat: controls modal spec

* refactor: use testing selector

* feat: cheats spec

- todo: tentatively refactor all tour tests
  - may want to test for # of steps

* feat: upload file modal specs

* chore: testing package updates

* chore: update test deps

* feat: screen spec

- additional readme update

* chore: testing package updates

* refactor: screen initial bounds test

* refactor: screen validate gripper handles

* refactor: screen rename tests

* refactor: remove explicit timeout

* feat: control panel spec, accessible buttons

- control panel spec

- accessible buttons

- remove unnecessary props

- fix aria labels

---------

Co-authored-by: Nick VanCise
timheilman referenced this issue in timheilman/cypress-realworld-app Mar 7, 2024
timheilman referenced this issue in timheilman/cypress-realworld-app Mar 7, 2024
markerikson referenced this issue in replayio-public/cypress-realworld-app Jun 25, 2024
markerikson referenced this issue in replayio-public/cypress-realworld-app Jun 25, 2024
jennifer-shehane referenced this issue in cypress-io/cypress-realworld-app Aug 27, 2024
* build: yarn add @mui/material @mui/styles ; yarn add @mui/lab ; yarn add @mui/icons-material

* build: yarn add @emotion/react @emotion/styled

* build: npx @mui/codemod@latest v5.0.0/preset-safe src

* build: empty commit, for: npx @mui/codemod@latest v5.0.0/variant-prop src

* build: npx @mui/codemod@latest v5.0.0/link-underline-hover src

* build: npx prettier --write src backend cypress scripts

* build: yarn remove @material-ui/core @material-ui/icons @material-ui/lab

* build: rm patches/@material-ui+core+4.12.4.patch

* build: update renovate.json to use @mui not @material-ui

* build: update ThemeProvider import, per

https://mui.com/material-ui/migration/v5-style-changes/\#update-themeprovider-import

* build: update EventType for Tabs onChange event, per

https://mui.com/material-ui/migration/v5-component-changes/\#update-event-type-typescript-5

* fix: Two tabs problems:

First, the color changes indicated at https://mui.com/material-ui/migration/v5-component-changes/#update-default-indicatorcolor-and-textcolor-prop-values

Second, remove the the trailing "px" a number rather than string when passing values to the react-virtualized List component.  See
https://mui.com/material-ui/migration/v5-style-changes/\#%E2%9C%85-remove-px-suffix

* fix: transaction amount was falling off right edge

* fix: indirection of onChange prop of Slider via ownerState

this was causing the slider-based cypress tests to fail.

* fix: remove zIndex from AppBar to fix drawer issue

appBar was still over the date picker in mobile screen size, failing one of the cypress:run:mobile tests.  See also
https://mui.com/material-ui/migration/v5-component-changes/\#fix-z-index-issues

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/containers

followed by lint fixes and manual review

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/BankAccountForm.tsx

linting and review

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/CommentForm.tsx

review, lint

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/MainLayout.tsx

review, remove class for root and use ampersand instead, adding a flexGrow:1 to get styling as before

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/NavBar.tsx,

review, appBarShift was incorrectly considered a child of root; fixed that with removal of space between & and ., lint

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/NavDrawer.tsx

review, lint, no issues

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/NotificationListItem.tsx

review, lint, no issues

* build:  npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/SignInForm.tsx

lint, cast StyledContainer to typeof Container per https://github.com/mui/material-ui/issues/15695\#ref-commit-7fd1a29

* build:  npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/SignUpForm.tsx

lint, cast StyledContainer to typeof Container per https://github.com/mui/material-ui/issues/15695\#ref-commit-7fd1a29

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/SkeletonList.tsx

lint, review, no issues

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/TransactionAmount.tsx

lint, remove space between & and . because classes are conditionally applied

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/TransactionCreateStepOne.tsx

lint and review, no issues

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/TransactionCreateStepTwo.tsx

lint, review, no issues

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/TransactionCreateStepThree.tsx

lint, review, no issues

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/TransactionDateRangeFilter.tsx

lint, review, no issues

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/TransactionDetail.tsx

lint, review, no issues

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/TransactionItem.tsx

lint, review, problems with SmallAvatar, fixed manually

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/TransactionList.tsx

lint, review, no issues

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/TransactionListAmountRangeFilter.tsx

lint, review, did not work really at all, fixed by using sx prop rather than classes at all

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/TransactionListFilters.tsx

lint, review, no issues

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/TransactionTitle.tsx

lint, review, no issues

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components/UserSettingsForm.tsx

lint, review, form did not want to play nice with classes, fixed with sx for submit button and styled wrapping div for form

* build: npx @mui/codemod@latest v5.0.0/jss-to-styled src/components

had apparently accidentally failed-to-checkin TransactionInfiniteList and userSearchForm, but both were reviewed and had no issues

* build: yarn remove @mui/styles and some

minor style fixups to be rid of the dependency causing the peer dependency warning

* fix: oops did not mean to modify database.json

* build: remove @ts-expect-error annotations no longer used after yarn.lock

merge resolution

* test: issue 1278 re-enabling test that had failed on firefox,

which is not failing after the upgrade to MUI v5.  See
#1278

* style: npm run prettier

* fix: unset flex-direction which is newly set to row-reverse for

AvatarGroup in mui5.

* fix: undo parent commit fix because avatars were backward and

no longer overlapping.  Instead eat up space to right of avatars.

* fix: get body font size back down to 14, establish knobs for

modifying input styles

* test: apply force:true to clicks failing due to issue #29776

---------

Co-authored-by: AtofStryker <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
package: styled-engine Specific to @mui/styled-engine typescript
Projects
None yet
Development

No branches or pull requests