Forward shouldForwardProp through withComponent #1668

animecyc · 2019-12-03T00:26:47Z

What:

Fixes a bug that prevented shouldForwardProp from forwarding to components created via withComponent; Fixes Ability to set default shouldForwardProp or expose to extended components #1394
Compacts createStyled's options argument

Why:

Ensures similar composition semantics as the as prop. This sort of begs the question of whether or not the withComponent functionality is needed given we have the as prop; Regardless, they should operate the same if the API exists.

How:

Coalesces shouldForwardProp together with options and nextOptions.

Checklist:

Documentation
Tests
Code complete
Changeset

changeset-bot · 2019-12-03T00:26:50Z

🦋 Changeset is good to go

Latest commit: 81a6877

We got this.

This PR includes changesets to release 6 packages

Name	Type
@emotion/styled	Major
@emotion/react	Major
@emotion/jest	Major
@emotion/primitives-core	Major
@emotion/native	Major
@emotion/primitives	Major

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

packages/styled-base/src/index.js

codecov · 2019-12-03T00:37:05Z

Codecov Report

Merging #1668 into next will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted Files	Coverage Δ
packages/styled/src/base.js	`100% <100%> (ø)`	⬆️
packages/jest-emotion/src/index.js	`100% <0%> (ø)`	⬆️
packages/jest-emotion/src/utils.js	`96.96% <0%> (+0.06%)`	⬆️

packages/styled/__tests__/styled.js

Andarist · 2019-12-03T22:15:59Z

Could you branch your change out from the next branch?

animecyc · 2019-12-04T20:59:17Z

Could you branch your change out from the next branch?

Sure, reconciling things now.

Andarist · 2019-12-05T21:18:11Z

Not sure exactly why, but the diff includes stuff that it shouldn't. Maybe it would be best to just close this PR and reopen it after branching away from next and cherry-picking your single commit?

codesandbox-ci · 2019-12-05T21:30:51Z

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 81a6877:

Sandbox	Source
Emotion	Configuration

animecyc · 2019-12-05T21:34:18Z

Not sure exactly why, but the diff includes stuff that it shouldn't. Maybe it would be best to just close this PR and reopen it after branching away from next and cherry-picking your single commit?

I thought I pushed, sorry -- its been reconciled now.

animecyc · 2019-12-05T21:44:09Z

packages/styled/src/base.js

+            ? propName =>
+                nextOptions.shouldForwardProp(propName) &&
+                defaultShouldForwardProp(propName)
+            : defaultShouldForwardProp


My understanding here is that we want to pipe the shouldForwardProp calls like a funnel (C -> B -> A) using either the supplied shouldForwardProp falling back to the composed defaultShouldForwardProp of the parent.

animecyc · 2019-12-05T21:46:30Z

packages/styled/__tests__/styled.js

+    expect(shouldForwardPropA).toHaveBeenNthCalledWith(1, 'as')
+    expect(shouldForwardPropA).toHaveBeenNthCalledWith(2, 'disabled')
+
+    expect(tree).toMatchSnapshot()


I figured it would be good to be a bit more explicit with this test and the calls made to shouldForwardProp on each withComponent

- Fixes a bug that prevented `shouldForwardProp` from forwarding to components created via `withComponent`; Fixes emotion-js#1394 - Compacts `createStyled`'s options argument

animecyc · 2019-12-08T01:08:35Z

packages/styled/src/base.js

+          nextTag = { ...nextTag }
+          nextTag.__emotion_real = nextTag
+          nextTag.__emotion_styles = nextTag.__emotion_styles.slice(1)
+        }


If we don't remove the prepended label for nextTag we end up with duplicate labels.

- Fixes duplicate labels when composing components. - Allows for correctly setting the label for the new composed component.

Andarist · 2019-12-18T21:03:59Z

@mitchellhamilton could you take this over? I'm afraid I won't have time to finish reviewing this right now.

emmatown · 2019-12-18T21:07:37Z

@mitchellhamilton could you take this over? I'm afraid I won't have time to finish reviewing this right now.

Absolutely, no problem!

emmatown · 2020-08-26T10:05:38Z

packages/styled/test/prop-filtering.test.js

  })()

  const ComposedDiv = styled(StyledDiv, {
-    shouldForwardProp: () => true
+    shouldForwardProp: prop => prop !== 'bar'


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.

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)

…mponent` (emotion-js#1668) * Forward shouldForwardProp through withComponent - Fixes a bug that prevented `shouldForwardProp` from forwarding to components created via `withComponent`; Fixes emotion-js#1394 - Compacts `createStyled`'s options argument * Fixes an issue with label duplication - Fixes duplicate labels when composing components. - Allows for correctly setting the label for the new composed component. * Refactor how withComponent handles shouldForwardProp * Fix flow errors Co-authored-by: Seth Benjamin <[email protected]> Co-authored-by: Mateusz Burzyński <[email protected]>

animecyc commented Dec 3, 2019

View reviewed changes

packages/styled-base/src/index.js Outdated Show resolved Hide resolved

Andarist changed the base branch from master to next December 3, 2019 22:07

Andarist reviewed Dec 3, 2019

View reviewed changes

packages/styled/__tests__/styled.js Outdated Show resolved Hide resolved

animecyc force-pushed the hotfix/1394 branch from 2f18d6b to 02c5010 Compare December 5, 2019 21:29

animecyc commented Dec 5, 2019

View reviewed changes

animecyc force-pushed the hotfix/1394 branch from 02c5010 to 8b25afa Compare December 5, 2019 21:45

animecyc commented Dec 5, 2019

View reviewed changes

Forward shouldForwardProp through withComponent

9f88750

- Fixes a bug that prevented `shouldForwardProp` from forwarding to components created via `withComponent`; Fixes emotion-js#1394 - Compacts `createStyled`'s options argument

animecyc force-pushed the hotfix/1394 branch from 8b25afa to 9f88750 Compare December 5, 2019 21:58

animecyc commented Dec 8, 2019

View reviewed changes

Fixes an issue with label duplication

dbdcb09

- Fixes duplicate labels when composing components. - Allows for correctly setting the label for the new composed component.

animecyc force-pushed the hotfix/1394 branch from 2508487 to dbdcb09 Compare December 8, 2019 01:10

emmatown self-requested a review December 18, 2019 21:19

Andarist added 3 commits August 22, 2020 14:08

Merge branch 'next' into hotfix/1394

6c24449

Refactor how withComponent handles shouldForwardProp

cacfbb9

Fix flow errors

81a6877

emmatown reviewed Aug 26, 2020

View reviewed changes

emmatown approved these changes Sep 3, 2020

View reviewed changes

Andarist merged commit 18c0d5f into emotion-js:next Sep 4, 2020

Andarist mentioned this pull request Sep 4, 2020

Ability to set default shouldForwardProp or expose to extended components #1394

Closed

github-actions bot mentioned this pull request Sep 4, 2020

Version Packages (next) #1982

Merged

github-actions bot mentioned this pull request Nov 10, 2020

Version Packages #2046

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Forward shouldForwardProp through withComponent #1668

Forward shouldForwardProp through withComponent #1668

animecyc commented Dec 3, 2019

changeset-bot bot commented Dec 3, 2019 •

edited

Loading

codecov bot commented Dec 3, 2019 •

edited

Loading

Andarist commented Dec 3, 2019

animecyc commented Dec 4, 2019

Andarist commented Dec 5, 2019

codesandbox-ci bot commented Dec 5, 2019 •

edited

Loading

animecyc commented Dec 5, 2019

animecyc Dec 5, 2019 •

edited

Loading

animecyc Dec 5, 2019

animecyc Dec 8, 2019

Andarist commented Dec 18, 2019

emmatown commented Dec 18, 2019

emmatown Aug 26, 2020

Andarist Aug 26, 2020 •

edited

Loading

Forward shouldForwardProp through withComponent #1668

Forward shouldForwardProp through withComponent #1668

Conversation

animecyc commented Dec 3, 2019

changeset-bot bot commented Dec 3, 2019 • edited Loading

🦋 Changeset is good to go

codecov bot commented Dec 3, 2019 • edited Loading

Codecov Report

Andarist commented Dec 3, 2019

animecyc commented Dec 4, 2019

Andarist commented Dec 5, 2019

codesandbox-ci bot commented Dec 5, 2019 • edited Loading

animecyc commented Dec 5, 2019

animecyc Dec 5, 2019 • edited Loading

Choose a reason for hiding this comment

animecyc Dec 5, 2019

Choose a reason for hiding this comment

animecyc Dec 8, 2019

Choose a reason for hiding this comment

Andarist commented Dec 18, 2019

emmatown commented Dec 18, 2019

emmatown Aug 26, 2020

Choose a reason for hiding this comment

Andarist Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

changeset-bot bot commented Dec 3, 2019 •

edited

Loading

codecov bot commented Dec 3, 2019 •

edited

Loading

codesandbox-ci bot commented Dec 5, 2019 •

edited

Loading

animecyc Dec 5, 2019 •

edited

Loading

Andarist Aug 26, 2020 •

edited

Loading