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

[styled] Convert implicit styleProps to explicit #26461

Merged
merged 4 commits into from Jun 1, 2021

Conversation

mnajdova
Copy link
Member

@mnajdova mnajdova commented May 26, 2021

BREAKING CHANGE

A wildcard styleProps is no longer being added by default by experimentalStyled(). You need to specify them by yourself, as any other props:

-const StyledComponent = styled('div')(({ styleProps }) => ({ ... });
+const StyledComponent = styled('div')<{ styleProps: { completed?: boolean } }>(({ styleProps }) => ({ ... });

Closes #26410

@mui-pr-bot
Copy link

mui-pr-bot commented May 26, 2021

Details of bundle changes (experimental)

Generated by 🚫 dangerJS against 4bbf83c

@mnajdova mnajdova requested a review from eps1lon May 26, 2021 13:53
Copy link
Member

@oliviertassinari oliviertassinari left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mnajdova How about adding new TypeScript tests based on the reproductions provided by Sebastian in #26410?

@mnajdova
Copy link
Member Author

@mnajdova How about adding new TypeScript tests based on the reproductions provided by Sebastian in #26410?

What exactly should we test. We removed these props from the native typings, so they behave as any other props. Testing whether they can be optional or not doesn't make sense to me. Can you point out to specific test case that makes sense?

@oliviertassinari
Copy link
Member

oliviertassinari commented May 28, 2021

I was thinking of something in this order, to complement the existing tests. I'm not sure if it really makes a difference.

diff --git a/packages/material-ui/src/styles/experimentalStyled.spec.tsx b/packages/material-ui/src/styles/experimentalStyled.spec.tsx
index 84fc2756f3..1ded15d117 100644
--- a/packages/material-ui/src/styles/experimentalStyled.spec.tsx
+++ b/packages/material-ui/src/styles/experimentalStyled.spec.tsx
@@ -1,5 +1,6 @@
 import * as React from 'react';
 import { experimentalStyled as styled } from '@material-ui/core/styles';

 const Box = styled('div')(({ theme }) => ({
   color: theme.palette.primary.main,
@@ -26,3 +27,21 @@ const StyledToolbar = styled('div')(({ theme }) => ({
 const StyledSpan = styled('span')(({ theme }) => ({
   ...theme.typography.body1,
 }));
+
+const WithProps = styled('span')<{ styleProps: { foo: string } }>(({ styleProps }) => {
+  return { color: styleProps.foo };
+});
+
+<WithProps styleProps={{ foo: 'bar' }} />;
+
+// @ts-expect-error styleProps doesn't exist
+const MissingOption = styled('span')(({ styleProps }) => {
+  return { height: 1 };
+});
+
+const MissingProperty = styled('span')<{ styleProps: { inner: boolean } }>(({ styleProps }) => {
+  return { height: 1 };
+});
+
+// @ts-expect-error missing inner property
+<MissingProperty styleProps={{}} />;

@mnajdova
Copy link
Member Author

I was thinking of something in this order, to complement the existing tests. I'm not sure if it really makes a difference.

I don't think it will bring much value, as it is nothing specific to the styled() util. The styleProps is now treated as any other arbitrary prop. The tests you've provided can be applied to any other prop, not sure it's worth adding it

@oliviertassinari
Copy link
Member

oliviertassinari commented May 28, 2021

@mnajdova Yeah, my point is that we don't have a test for this TypeScript first argument to add new props (it's not specific to stylesProps prop). Right now, we implicitly have this test with the demos of the docs. Which is maybe enough.

@github-actions github-actions bot added the PR: out-of-date The pull request has merge conflicts and can't be merged label May 31, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[styled] Implicit styleProps should be explicit
4 participants