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

Add warning when className prop is not used #2073

Merged
merged 5 commits into from Oct 8, 2018

Conversation

Projects
None yet
6 participants
@Fer0x
Contributor

Fer0x commented Oct 8, 2018

#349

I tried several options like using getters/Proxy but it doesn't work because of copying props inside React (https://github.com/facebook/react/blob/master/packages/react/src/ReactElement.js#L193)

Proposed code warns in such cases:

const Test = () => <div />;
const StyledTest = styled(Test)`
  background: white;
`;

<StyledTest />

and doesn't warn in such cases:

const Test2 = ({ className = '' }) => (
  <div>
    <div className={className} />
  </div>
);

const StyledTest2 = styled(Test2)`
  background: black;
`;

<StyledTest2 />

Tested with webpack 4: classNameUseCheckInjector.js will not present in bundle in production mode.

@probablyup

This is great! Can you add a test?

!node.querySelector(selector)
) {
console.warn(
"It looks like you've used styled() factor with React component, but prop `className` is not used"

This comment has been minimized.

@probablyup

probablyup Oct 8, 2018

Contributor

Nit: reword to:

`It looks like you've wrapped styled() around your React component (${getComponentName(target)}), but the className prop is not being passed down to a child. No styles will be rendered unless className is composed within your React component.`

This comment has been minimized.

@Fer0x

Fer0x Oct 8, 2018

Contributor

@probablyup Is getComponentName should work with React component instance?

@probablyup

This comment has been minimized.

Contributor

probablyup commented Oct 8, 2018

@probablyup

This comment has been minimized.

Contributor

probablyup commented Oct 8, 2018

@mxstbr

Ahhh I can't wait to land this warning, so many people have stumbled over this! 👏

@probablyup

This comment has been minimized.

Contributor

probablyup commented Oct 8, 2018

@Fer0x

  1. CI is failing, need to fix that
  2. Could you add at least one test?
  3. Could you add a changelog entry?
@Fer0x

This comment has been minimized.

Contributor

Fer0x commented Oct 8, 2018

@probablyup i will add all of this asap

@probablyup

This comment has been minimized.

Contributor

probablyup commented Oct 8, 2018

I'll finish this out so I can cut another beta. Definitely want this included!

@probablyup probablyup force-pushed the warning-not-used-classnames branch from 547571d to b989fa0 Oct 8, 2018

@probablyup

This comment has been minimized.

Contributor

probablyup commented Oct 8, 2018

Thanks @Fer0x!

@probablyup probablyup merged commit fb083e2 into develop Oct 8, 2018

2 checks passed

bundlesize ./dist/styled-components.min.js: 14.82KB < maxSize 16KB (gzip)(1.44KB smaller than master, good job!)
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@probablyup probablyup deleted the warning-not-used-classnames branch Oct 8, 2018

@StevenLangbroek

This comment has been minimized.

StevenLangbroek commented Oct 18, 2018

Is there a way to disable this warning and say "I know what I'm doing"? This is causing a lot of headaches for us.

@Fer0x

This comment has been minimized.

Contributor

Fer0x commented Oct 18, 2018

@StevenLangbroek please describe yours use case.

@StevenLangbroek

This comment has been minimized.

StevenLangbroek commented Oct 18, 2018

Sure, just an example, but we have an Image component that displays a <Fallback /> before the actual image is done loading. As soon as the fallback displays, we get this warning.

@StevenLangbroek

This comment has been minimized.

StevenLangbroek commented Oct 18, 2018

Another one:

(FlexWrapper and FlexInner are styled-components too)

const FlexCentered = props => (
  <FlexWrapper {...props}>
    <FlexInner>{props.children}</FlexInner>
  </FlexWrapper>
);

const TimesAndAddressWrapper = styled(FlexCentered)`
  > div {
    display: flex;
    flex-direction: column-reverse;

    @media ${MOBILE.OVER_1024} {
      flex-direction: row;
    }
  }
`;
@StevenLangbroek

This comment has been minimized.

StevenLangbroek commented Oct 18, 2018

There are a lot of hidden assumptions here that makes me think this needs a way of being disabled.

@StevenLangbroek

This comment has been minimized.

StevenLangbroek commented Oct 18, 2018

(reason I even bring it up is we're 4 teams working on the same codebase, people panic)

@Fer0x

This comment has been minimized.

Contributor

Fer0x commented Oct 18, 2018

For async render we could add some timeout in cdm which should fix your example with Image.

Second example: if <FlexWrapper /> is styled component then it should not trigger warnings (https://codesandbox.io/s/8nml16zjwl)

@StevenLangbroek

This comment has been minimized.

StevenLangbroek commented Oct 18, 2018

For async render we could add some timeout in cdm which should fix your example with Image.

This sounds brittle and like it might break arbitrarily.

I just gave some examples, and tbh, some might be actual issues (I'm going through them 1 by 1), but this is what my console looks like now:
image

@probablyup

This comment has been minimized.

Contributor

probablyup commented Oct 18, 2018

@StevenLangbroek

This comment has been minimized.

StevenLangbroek commented Oct 19, 2018

I think most of our cases are variations on this behavior:

const StyledUl = styled.ul``;

const List = props => (
  <StyledUl className={props.className} />
)

const StyledList = styled(List)``;
@Fer0x

This comment has been minimized.

Contributor

Fer0x commented Oct 19, 2018

@StevenLangbroek I don't see any reasons why this example should cause warnings https://codesandbox.io/s/5z51w8jw5k

@StevenLangbroek

This comment has been minimized.

StevenLangbroek commented Oct 19, 2018

Yeah I'm trying to figure out what's going on here, it might be related to the babel plugin (we're seeing a difference in dev / prod behavior, and between client / serverside, which we also have slightly different babel settings for).

@StevenLangbroek

This comment has been minimized.

StevenLangbroek commented Oct 19, 2018

I have no idea how to further debug what's going on here... the net effect of this is I have to revert the upgrade (it never made it out into production, so it's no big deal), I still obviously need to fix this situation. I'll try to come up with a minimal repro over the weekend.

@vitalybe

This comment has been minimized.

vitalybe commented Oct 20, 2018

I've had a similar issue with mbrevda/react-image library.

I've avoided by doing, instead of:

const ReactImageStyled = styled(ReactImage)`
  ${(props: {}) => css`
    max-height: 100%;
    max-width: 100%;
  `};

I am applying the styles from the parent, like so:

const ContainerDiv = styled.div`
  ${(props: {}) => css`
    img {
      max-height: 100%;
      max-width: 100%;
    }
  `};
`;
@Fer0x

This comment has been minimized.

Contributor

Fer0x commented Oct 20, 2018

@vitalybe Could you make sample project with reproducing of this warning? I can't reproduce it on codesandbox (https://codesandbox.io/s/w0n3wqnn17) so there can be issue related to environment like react-hot-loader or some babel plugins.

@vitalybe

This comment has been minimized.

vitalybe commented Oct 21, 2018

There you go. I've messed with it a bit until it worked, but it reproduces:

https://codesandbox.io/s/v3r61wk25

const classNames = elementClassName.split(' ');
// eslint-disable-next-line react/no-find-dom-node
const node: Element | null = (ReactDOM.findDOMNode(this): any);

This comment has been minimized.

@optunetobi

optunetobi Oct 25, 2018

Running React in Strictmode gives this warning:

Warning: findDOMNode is deprecated in StrictMode. findDOMNode was passed an instance of StyledComponent which is inside StrictMode. Instead, add a ref directly to the element you want to reference.

I am aware that it is only run in development-mode, but it clutters console output when trying to find errors.

@Fer0x Could we change this here somehow? (by not using findDOMNode)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment