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

Forward shouldForwardProp through withComponent #1668

Merged
merged 5 commits into from
Sep 4, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .changeset/rotten-onions-hide.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@emotion/styled': minor
---

Custom `shouldForwardProp` is being preserved now when using `.withComponent`. Also, when passing an additional `shouldForwardProp` in `.withComponent`'s options (like this: `SomeComponent.withComponent('span', { shouldForwardProp })`) it's being composed with the potentially existing `shouldForwardProp`.
1 change: 1 addition & 0 deletions packages/styled/__tests__/styled.js
Original file line number Diff line number Diff line change
Expand Up @@ -640,6 +640,7 @@ describe('styled', () => {
const tree = renderer.create(<OneMoreComponent />).toJSON()
expect(tree).toMatchSnapshot()
})

test('theming', () => {
const Div = styled.div`
color: ${props => props.theme.primary};
Expand Down
32 changes: 12 additions & 20 deletions packages/styled/src/base.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ import * as React from 'react'
import type { ElementType } from 'react'
import {
getDefaultShouldForwardProp,
composeShouldForwardProps,
type StyledOptions,
type CreateStyled,
type PrivateStyledComponent
Expand All @@ -26,27 +27,18 @@ let createStyled: CreateStyled = (tag: any, options?: StyledOptions) => {
)
}
}
const isReal = tag.__emotion_real === tag
const baseTag = (isReal && tag.__emotion_base) || tag

let identifierName
let shouldForwardProp
let targetClassName
if (options !== undefined) {
identifierName = options.label
targetClassName = options.target
shouldForwardProp =
tag.__emotion_forwardProp && options.shouldForwardProp
? propName =>
tag.__emotion_forwardProp(propName) &&
// $FlowFixMe
options.shouldForwardProp(propName)
: options.shouldForwardProp
}
const isReal = tag.__emotion_real === tag
const baseTag = (isReal && tag.__emotion_base) || tag

if (typeof shouldForwardProp !== 'function' && isReal) {
shouldForwardProp = tag.__emotion_forwardProp
}
let defaultShouldForwardProp =
const shouldForwardProp = composeShouldForwardProps(tag, options, isReal)
const defaultShouldForwardProp =
shouldForwardProp || getDefaultShouldForwardProp(baseTag)
const shouldUseAs = !defaultShouldForwardProp('as')

Expand Down Expand Up @@ -196,12 +188,12 @@ let createStyled: CreateStyled = (tag: any, options?: StyledOptions) => {
nextTag: ElementType,
nextOptions?: StyledOptions
) => {
return createStyled(
nextTag,
nextOptions !== undefined
? { ...(options || {}), ...nextOptions }
: options
)(...styles)
return createStyled(nextTag, {
...options,
// $FlowFixMe
...nextOptions,
shouldForwardProp: composeShouldForwardProps(Styled, nextOptions, true)
})(...styles)
}

return Styled
Expand Down
23 changes: 23 additions & 0 deletions packages/styled/src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,29 @@ export const getDefaultShouldForwardProp = (tag: ElementType) =>
? testOmitPropsOnStringTag
: testOmitPropsOnComponent

export const composeShouldForwardProps = (
tag: PrivateStyledComponent<any>,
options: StyledOptions | void,
isReal: boolean
) => {
let shouldForwardProp
if (options) {
const optionsShouldForwardProp = options.shouldForwardProp
shouldForwardProp =
tag.__emotion_forwardProp && optionsShouldForwardProp
? (propName: string) =>
tag.__emotion_forwardProp(propName) &&
optionsShouldForwardProp(propName)
: optionsShouldForwardProp
}

if (typeof shouldForwardProp !== 'function' && isReal) {
shouldForwardProp = tag.__emotion_forwardProp
}

return shouldForwardProp
}

export type CreateStyledComponent = <Props>(
...args: Interpolations
) => StyledComponent<Props>
Expand Down
65 changes: 65 additions & 0 deletions packages/styled/test/__snapshots__/prop-filtering.test.js.snap
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`composes shouldForwardProp on composed styled components 1`] = `
<div
className="emotion-0 emotion-1"
xyz={true}
/>
`;

exports[`custom shouldForwardProp works 1`] = `
.emotion-0,
.emotion-0 * {
Expand Down Expand Up @@ -124,3 +131,61 @@ Array [
/>,
]
`;

exports[`withComponent inherits explicit shouldForwardProp 1`] = `
.emotion-0 {
color: hotpink;
}

<span
className="emotion-0 emotion-1"
foo={true}
/>
`;

exports[`withComponent inherits explicit shouldForwardProp from flattened component 1`] = `
.emotion-0 {
color: hotpink;
background-color: blue;
}

<span
className="emotion-0 emotion-1"
foo={true}
/>
`;

exports[`withComponent should accept shouldForwardProp 1`] = `
.emotion-0 {
color: hotpink;
}

<span
className="emotion-0 emotion-1"
xyz={true}
/>
`;

exports[`withComponent should compose shouldForwardProp 1`] = `
.emotion-0 {
color: hotpink;
}

<span
className="emotion-0 emotion-1"
qwe={true}
xyz={true}
/>
`;

exports[`withComponent should compose shouldForwardProp with a flattened component 1`] = `
.emotion-0 {
color: hotpink;
}

<span
className="emotion-0 emotion-1"
qwe={true}
xyz={true}
/>
`;
74 changes: 69 additions & 5 deletions packages/styled/test/prop-filtering.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,17 +6,16 @@ import styled from '@emotion/styled'

test('composes shouldForwardProp on composed styled components', () => {
const StyledDiv = styled('div', {
shouldForwardProp: prop => prop === 'forwardMe'
shouldForwardProp: prop => prop !== 'foo'
})()

const ComposedDiv = styled(StyledDiv, {
shouldForwardProp: () => true
shouldForwardProp: prop => prop !== 'bar'
Copy link
Member

Choose a reason for hiding this comment

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

Why these changes? Was this test failing with the changes to styled here? If the original test here was failing, I think that's a problem.

Copy link
Member

@Andarist Andarist Aug 26, 2020

Choose a reason for hiding this comment

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

I don't think the previous version of this test was foo - it was supposed to test shouldForwardProp composition but () => true doesn't make any difference in regards to the final behavior.

This test would pass with it or without this shouldForwardProp as the composition uses && so if we fold this shouldForwardProp1(prop) && shouldForwardProp2(prop) to shouldForwardProp1(prop) && true then all we really have is shouldForwardProp1(prop)

})()

const tree = renderer.create(<ComposedDiv filterMe forwardMe />).toJSON()
const tree = renderer.create(<ComposedDiv foo bar xyz />).toJSON()

expect(tree.props.filterMe).toBeUndefined()
expect(tree.props.forwardMe).toBeDefined()
expect(tree).toMatchSnapshot()
})

test('custom shouldForwardProp works', () => {
Expand Down Expand Up @@ -470,3 +469,68 @@ test('prop filtering on composed styled components that are string tags', () =>

expect(tree).toMatchSnapshot()
})

test('withComponent inherits explicit shouldForwardProp', () => {
const SomeComponent = styled('div', {
shouldForwardProp: prop => prop === 'foo'
})`
color: hotpink;
`
const AnotherComponent = SomeComponent.withComponent('span')
const tree = renderer.create(<AnotherComponent foo bar />).toJSON()
expect(tree).toMatchSnapshot()
})

test('withComponent inherits explicit shouldForwardProp from flattened component', () => {
const SomeComponent = styled('div', {
shouldForwardProp: prop => prop === 'foo'
})`
color: hotpink;
`
const AnotherComponent = styled(SomeComponent)`
background-color: blue;
`
const YetAnotherComponent = AnotherComponent.withComponent('span')
const tree = renderer.create(<YetAnotherComponent foo bar />).toJSON()
expect(tree).toMatchSnapshot()
})

test('withComponent should accept shouldForwardProp', () => {
const SomeComponent = styled('div')`
color: hotpink;
`
const AnotherComponent = SomeComponent.withComponent('span', {
shouldForwardProp: prop => prop === 'xyz'
})
const tree = renderer.create(<AnotherComponent qwe xyz />).toJSON()
expect(tree).toMatchSnapshot()
})

test('withComponent should compose shouldForwardProp', () => {
const SomeComponent = styled('div', {
shouldForwardProp: prop => prop !== 'foo'
})`
color: hotpink;
`
const AnotherComponent = SomeComponent.withComponent('span', {
shouldForwardProp: prop => prop !== 'bar'
})
const tree = renderer.create(<AnotherComponent foo bar qwe xyz />).toJSON()
expect(tree).toMatchSnapshot()
})

test('withComponent should compose shouldForwardProp with a flattened component', () => {
const SomeComponent = styled('div', {
shouldForwardProp: prop => prop !== 'foo'
})`
color: hotpink;
`
const AnotherComponent = styled(SomeComponent)`
background-color: blue;
`
const YetAnotherComponent = SomeComponent.withComponent('span', {
shouldForwardProp: prop => prop !== 'bar'
})
const tree = renderer.create(<YetAnotherComponent foo bar qwe xyz />).toJSON()
expect(tree).toMatchSnapshot()
})