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

[GlobalStyles] styles type is probably too wide #25208

Open
eps1lon opened this issue Mar 5, 2021 · 3 comments
Open

[GlobalStyles] styles type is probably too wide #25208

eps1lon opened this issue Mar 5, 2021 · 3 comments
Assignees
Labels
component: GlobalStyles The React component. external dependency Blocked by external dependency, we can’t do anything about it package: styled-engine Specific to @mui/styled-engine typescript

Comments

@eps1lon
Copy link
Member

eps1lon commented Mar 5, 2021

Follow-up to #25191

Current Behavior 😯

<GlobalStyles styles={false} /> is accepted

Expected Behavior 🤔

It's unclear what this is supposed to do. https://next--material-ui.netlify.app/customization/how-to-customize/#5-global-css-override only documents an object.

The documented type in the API is probably too wide. Either we restrict it or we add tests for each overload.

@eps1lon eps1lon added the docs Improvements or additions to the documentation label Mar 5, 2021
@eps1lon eps1lon added the component: GlobalStyles The React component. label Mar 5, 2021
@mnajdova
Copy link
Member

mnajdova commented Apr 8, 2021

I've took a look on this. The typings comes from emotion's Global component's props - more specifically from the InterpolationPrimitive We could restrict them, but that would mean that we would diverge from the original typings. Happy to do that if we agree it's better.

@eps1lon
Copy link
Member Author

eps1lon commented Apr 9, 2021

We could restrict them, but that would mean that we would diverge from the original typings.

Are we even sure that is the correct type in this place? Where is this type used in emotion and are we using it the same way? If these are equal then I'm referring back to "It's unclear what this is supposed to do.". Which may be an upstream issue.

@oliviertassinari oliviertassinari added the package: styled-engine Specific to @mui/styled-engine label Apr 11, 2021
@oliviertassinari
Copy link
Member

oliviertassinari commented Apr 11, 2021

It looks like an upstream issue, we could report it back.

From a type perspective, would this improve them? It won't fix the issue but maybe make them more accurate, not sure

diff --git a/packages/material-ui-styled-engine/src/GlobalStyles/GlobalStyles.d.ts b/packages/material-ui-styled-engine/src/GlobalStyles/GlobalStyles.d.ts
index 70fc40831a..4458bd817c 100644
--- a/packages/material-ui-styled-engine/src/GlobalStyles/GlobalStyles.d.ts
+++ b/packages/material-ui-styled-engine/src/GlobalStyles/GlobalStyles.d.ts
@@ -1,9 +1,9 @@
 import * as React from 'react';
-import { Interpolation } from '@emotion/styled';
+import { GlobalProps } from '@emotion/react';

 export interface GlobalStylesProps {
   defaultTheme?: object;
-  styles: Interpolation<any>;
+  styles: GlobalProps['styles'];
 }

 export default function GlobalStyles(props: GlobalStylesProps): React.ReactElement;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: GlobalStyles The React component. external dependency Blocked by external dependency, we can’t do anything about it package: styled-engine Specific to @mui/styled-engine typescript
Projects
None yet
Development

No branches or pull requests

3 participants