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

isTag() additionally check for casing of the first char #2225

Merged
merged 7 commits into from Nov 28, 2018

Conversation

Projects
None yet
2 participants
@asvetliakov
Member

asvetliakov commented Nov 18, 2018

It's very common to write tests like this:

jest.mock("../SomeComponent", () => "MockedComponent");

it("test", () => {
  expect(testRenderer(<Component someParam="a" />)).toMatchSnapshot();
});

where Component may be:

import SomeComponent from "./SomeComponent";
export default styled(SomeComponent)` /* stuff */`;

In this case SomeComponent becomes string-typed internally similar to dom-like components and only name with passed props is being rendered is snapshots.

Without this fix, the styled-components thinks as SomeComponent is like dom-like component and applies props filtering to it, resulting in filtered out custom props (someParam in example above) from test snapshots.

@asvetliakov asvetliakov requested a review from probablyup Nov 18, 2018

Show resolved Hide resolved src/utils/isTag.js Outdated
@@ -4,5 +4,5 @@ import isTag from './isTag';
import type { Target } from '../types';

export default function generateDisplayName(target: Target): string {
return isTag(target) ? `styled.${target}` : `Styled(${getComponentName(target)})`;
return isTag(target) ? `styled.${getComponentName(target)}` : `Styled(${getComponentName(target)})`;

This comment has been minimized.

@probablyup

probablyup Nov 20, 2018

Contributor

why was this change added? it's not desired

This comment has been minimized.

@asvetliakov

asvetliakov Nov 20, 2018

Member

Flow can't coerce correctly to string type for some reason

This comment has been minimized.

@probablyup

probablyup Nov 20, 2018

Contributor

Ah ok, just put a // $FlowFixMe

This comment has been minimized.

@asvetliakov
export default function getComponentName(target: ComponentType<*>): string {
return target.displayName || target.name || 'Component';
export default function getComponentName(target: ComponentType<*> | string): string {
return typeof target === "string" ? target : target.displayName || target.name || 'Component';

This comment has been minimized.

@probablyup

probablyup Nov 20, 2018

Contributor

this can be reverted as well

This comment has been minimized.

@asvetliakov

asvetliakov Nov 20, 2018

Member

I don't think so, without it the generated display name will be Styled("Component") ("Component" string) with mocked components because string doesn't have either displayName || name

This comment has been minimized.

@probablyup

probablyup Nov 20, 2018

Contributor

It won't be because you're accounting for the uppercase string name in isTag. It'll end up as styled.YourMockedName

This comment has been minimized.

@asvetliakov

asvetliakov Nov 20, 2018

Member

No, it won't, because isTag will return false for such components (check for first lower case letter will fail)

@probablyup

This comment has been minimized.

Contributor

probablyup commented Nov 20, 2018

Finally, just need a test and a changelog entry :) thanks for your work!

asvetliakov and others added some commits Nov 20, 2018

@asvetliakov

This comment has been minimized.

Member

asvetliakov commented Nov 28, 2018

@probablyup probablyup merged commit 939d3d8 into styled-components:master Nov 28, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@asvetliakov asvetliakov deleted the asvetliakov:isTag-check branch Nov 29, 2018

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