Styled components v6 support

[jcfilben]

What does this PR do?

Where should the reviewer start?

What testing has been done on this PR?

How should this be manually tested?

Do Jest tests follow these best practices?

• screen is used for querying.
• The correct query is used. (Refer to this list of queries)
• asFragment() is used for snapshot testing.

Any background context you want to provide?

What are the relevant issues?

Screenshots (if appropriate)

Do the grommet docs need to be updated?

Should this PR be mentioned in the release notes?

Is this change backwards compatible or is it a breaking change?

[taysea]

I forget why we needed this -- is it documented anywhere why we have this? Would be helpful to keep note for future reference.

[jcfilben]

I believe we were having issues with the jest tests, the suggestion to add this line came from styled-components/jest-styled-components#144 (comment)

[taysea]

@jcfilben Can you remind me why in some cases we changed to "Box" vs others we kept as div?

would be good to document this decision for future reference.

[jcfilben]

There were some places that were using styled.div and as={Box} so I think we decided to just simplify to styled(Box)

[Tbhesswebber]

Is there some rationale for this being a direct dependency rather than a peer dependency? It is unintuitive (to me, at least) that a package used specifically for a peer dependency would be a direct dependency.

[taysea]

@jcfilben Do we need to add this directly? Or is it included with styled-components v6?

[Tbhesswebber]

It's not included, but it can be configured globally through styled components (and likely will in many cases). The alternative is to migrate to transient props, which a lot of people seem to be balking at

[jcfilben]

@emotion/is-prop-valid is a dependency of styled-components but since we need to import and use isPropValid within our own code I added it as a dependency on Grommet. Although it is a styled-components dependency, I didn't see a way to import isPropValid through styled-components

[Tbhesswebber]

v6 dropped the direct dependency, making it a peer dependency. They migrated to transient props instead

https://styled-components.com/docs/faqs#shouldforwardprop-is-no-longer-provided-by-default

[jcfilben]

We explored transient props but ultimately decided to use is-prop-valid to filter out styling props from being passed to the dom. Some more context on our explorations:
#6947
#6941
https://github.com/grommet/grommet/tree/styled-components-v6 - branch where we started converting to transient props

[taysea]

Thanks for jogging my memory Jessica!

[Tbhesswebber]

Supporting both seems like it might be risky with significant breaking changes specifically because of how v6 handles nesting as compared to v5. You can find that documentation here.

[taysea]

Hey @Tbhesswebber thanks for commenting. Could you elaborate on your concerns? Definitely want to hear them out as we're considering adding support for v6 as well.

With these changes, we are taking care of shouldForwardProp (mentioned in your link above) on all styled instances within grommet components (see styles.js under files changed). This will ensure proper styling if someone is using styled components v6.

We previously did an alpha release with the behavior and didn't see any issues. That release was v2.35.0-alpha.1 if it's helpful to have a released version to try. https://github.com/grommet/grommet/releases/tag/v2.35.0-alpha.1

[Tbhesswebber]

I updated the link above (I was hasty with copy pasta, it seems).

const Foo = styled.button`
  :hover {
    background: black;
  }
`;

The above snippet outputs different CSS in v5 and v6. In v5, there's an implicit & prepended to :hover, so it's applied directly to the element. In v6, it's prepended with & (note the space) to align with the nesting spec for CSS, so the hover state applies to children of the button rather than the button itself.

[jcfilben]

I went through and added & to the places in Grommet where the & was being implicitly added by styled components v5 so that we will get the same behavior in v5 and v6. (See the changes in StyledCheckBox.js for an example)

[Tbhesswebber]

Sorry for not being clear, my concern is moreso in the realm of consumers having this syntax in the .extends properties in their configuration or in styled(SomeGrommetComponent) declarations. Breaking changes that likely won't be documented in Grommet's changelog because they aren't actually changes to Grommet

[jcfilben]

I think we can add a note to the Grommet release where we add styled components v6 support, so that this doesn't catch anyone by surprise. We have a similar message on the alpha release we did for styled components v6 support: https://github.com/grommet/grommet/releases/tag/v2.35.0-alpha.1.
But ultimately since the breaking change is not caused by Grommet and is a result of the styled-components upgrade then I think the person upgrading would just have to refer to the styled-components upgrade documentation which calls out the breaking changes.
Since we will have support for both v5 and v6 of styled components it is up to the consumer if they want to upgrade to v6 or not.

[taysea]

@jcfilben any concerns around this attribute being removed? I'm going to look into it a bit more

[taysea]

I looked a little more, this is a change in the DOM although there's no break to functionality. This is in cases where the input is in a form and the input value is never updating in the DOM since it's managed by the form itself (this is the case regardless of styled-components version) so it moreso seems like it's just cleaning up an empty DOM attribute?

[jcfilben]

Agree that it looks like a cleanup. There are other snapshots in this PR that have the same change. It looks like styled components is just stricter and doesn't let an empty value pass into the DOM. I think this snapshot change is fine