Apply scoped styles to child components

[BernieCr]

Implements #573.
I also cleaned up that one if-condition a bit.

[giuseppeg]

👋 thanks for the PR. This change is going to be a breaking one as currently users don't expect to get a scoped class. I guess I will keep the PR open until we decide to cut a major release and evaluate if it makes sense to introduce this change

[giuseppeg]

let's wait for the next major release

[bySabi]

@bernhardc I create a minimal repo with your fork and I had no luck making it work: is working fine https://github.com/bySabi/styled-jsx-repo (using yarn resolutions)

[bySabi]

@bernhardc @giuseppeg @pushkine I would like to reactivate this feature by doing opt-in using a new syntax.

For example:

const Home = () => (
  <div>
    <h1 className="header">Login</h1>
    <Button className="login-button">Submit</Button>
    <Button>Cancel</Button>
    <style jsx nested>
      {`
        .header {
          font-size: 50px;
        }
        .login-button {
          color: red;
        }
      `}
    </style>
  </div>
)

maybe <style jsx local></style> or <style jsx custom></style>

It wouldn't break the current API, I think.

What could stop this improvement?

[bySabi]

I merged @bernhardc commits on current styled-jsx master: https://github.com/bySabi/styled-jsx

it would be necessary to add the code to make it opt-in

Please tell me if this is worth it or not ...

[BernieCr]

@bySabi I think making this feature opt-in would be a good solution since the next major release doesn't seem to be in sight.
My thought was to toggle this feature via a configuration option, maybe named scopedChildComponents.

Of course it also would be possible to toggle it on the level of the <style> tag, although I can't think of a use case where you wouldn't just want to enable this feature globally for your project if you intend to use it. In case we make use of the <style> tag I'd suggest deep as the name of the toggle, since all jsx styles are already kind of local, custom and nested. deep would also be in line with how things are named by Vue scoped CSS.

I also checked out your example repo. The reason why it didn't work is that the override of the styled-jsxdependency didn't affect next, which still uses the styled-jsx NPM package. I don't know how to override this properly, but if you copy node_modules/styled-jsx to node_modules/next/node_modules/styled-jsx, it builds correctly.

[bySabi]

Thanks for answering after so much time has passed since this PR.

Yes, deep was going to be my next suggestion after weighing custom, local or nested ... it's great that we have agreed on this.

And I also agree that the configuration option would be sufficient and much easier to implement.

I really like scopedChildComponents.

I don't know how to override this properly, but if you copy node_modules/styled-jsx to node_modules/next/node_modules/styled-jsx, it builds correctly.

For this you can use yarn resolutions or this package https://www.npmjs.com/package/npm-force-resolutions

This how is doned on: styled-jsx-repo: https://github.com/bySabi/styled-jsx-repo/blob/master/package.json#L15

I hope this PR can be merge soon.

Thanks for the hard work

[bySabi]

We could give you the final boost and add the option, scopedChildComponents

What do you think?

[BernieCr]

If you want to do it be my guest. Otherwise I think I could implement it next week.
Adding the config option will probably be trivial, but the tests also need to be updated, and some documentation will have to be added to the Readme.

@giuseppeg Would you be OK with this feature being opt-in via a compiler configuration toggle, therefore making this change non-breaking?

[bySabi]

I need this feature just right now. I will try to do my best, is a lot of work here, code, tests, docs, ...

[bySabi]

@bernhardc could be scopedChilds in place of scopedChildComponents ? is a little more short and looks a lot like current options: sourceMaps | vendorPrefixes

@giuseppeg we need a little of your time here please.

[bySabi]

Well, I have a working branch with the scopedChilds option.

Let's see if I can add some test ...

EDIT: added some test. I think it's done

Lets put some Docs ...

EDIT2: @bernhardc I need some help from you. I'm almost done, only the README is left.
Could you add some doc to the README.md of your branch?
I'm afraid my english is not very expressive

[giuseppeg]

Hey folks! Sorry for the late reply – I have been busy at my day job :)

Tim and I have some reservations on this feature because with this you'd potentially pass a prop to a component that doesn't wanted it or that can't accept it. For example if you have a declarative router component or if you have a component that accepts a className prop and sets it on their internal elements and in the parent component you use a generic selector:

function MyComponent() {  
  return (    
    <div>
      <SomeOtherComponent />
      <style jsx>{`
        div { margin: 300px }
      `}</style>
    </div>
  )
}

function SomeOtherComponent({ className }) {
  return <div className={className}>hello</div>
}

Here we are styling the div in MyComponent but this has the side effect of styling the div in SomeOtherComponent too! This bug can manifest even if you were able to turn the kind of changes that you want on a component level i.e. only add the className prop to components used in the current component.

The right way to style a component is to do it explicitly. To do this styled-jsx already provides a solution that is the resolve helper from styled-jsx/css:

import css from 'styled-jsx/css'

const { className, styles } = css.resolve`
  div {
    color: green
  }
`

function MyComponent() {    
  return (    
    <div>
      <SomeOtherComponent className={className} />
      <style jsx>{`
        div { margin: 300px }
      `}</style>
      <style jsx>{styles}</style>
    </div>
  )
}

function SomeOtherComponent({ className }) {
  return <div className={className}>hello</div>
}

Finally to mention what @bySabi wrote in the other PR:

After experimenting a bit with the fork, I have concluded that this PR is not very useful.
The selector works only with the element of the child component that has the className.
I thought, fool of me, that applying a selector on the child's container would allow me to select each subelement using nesting &.

This is indeed correct, we cannot scope child components from the parent. These are probably the only options you have at the moment https://github.com/zeit/styled-jsx#styling-third-parties--child-components-from-the-parent

I am going to close these PR for now as I think that the downsides make implementing this feature not worth it. Thank you a lot for working on these PRs instead of just shouting at us in an issue and expecting us to implement this. I feel very sorry that you invested time and energies pursuing this option and in the end I am not merging it, again thank you for your time and contribution! Maybe in the future we can discuss feature requests in an issue.

[BernieCr]

@giuseppeg Thanks for your reply! The issue you mentioned with styles for a div leaking into a child component could indeed be problematic, I hadn't thought of that before.
Nevertheless I think this PR still has a valid use case, and I believe most users of this library would expect to child components to receive scoped CSS as well if a className prop gets passed to them from the parent.

I originally came across this project while searching for a solution to recreate the great developer experience of Vue single file components in React. So I had a look how Vue handles this. As it turns out, it's exactly the same: the styling for our div in the example would indeed leak into child components as well, altough only into the root element of the child component.
The Vue docs explicitely state:

[...] The parent component's styles will not leak into child components. However, a child component's root node will be affected by both the parent's scoped CSS and the child's scoped CSS. This is by design so that the parent can style the child root element for layout purposes.

Since the vast majority of React components that accept a className prop would pass that class name to their root component, the behaviour of the changes in this PR would be exactly the same.
Plus we need to consider that, except for trivial components, it would likely be bad practice to do the styling via HTML elements instead class names. If you style using class names the problem doesn't really apply.

I understand your concerns, and if you're still cautious I'd suggest an explicit opt-in via the <style> tag, like @bySabi originally suggested. I'd propose <style deep>. The docs could then come with a warning that points out the caveat when styling directly HTML elements.

And regarding the issue that you'd pass a className prop to a component that doesn't actually accept it: I already took care of this in the PR. The jsx class name gets only added to components where the user already passes a className prop.

What do you think?

[giuseppeg]

As it turns out, it's exactly the same: the styling for our div in the example would indeed leak into child components as well, altough only into the root element of the child component

Folks can do this with css.resolve, the reason why I don't want to add this option is that while className is part of the dom element API (i.e. every element can get a class attribute) this is not the case for React components. Imagine if you use React Router <Route> and that get passed a className, or another component that takes props and spreads them automatically on other elements. This could lead to many unexpected scenarios.

However, a child component's root node will be affected by both the parent's scoped CSS and the child's scoped CSS.

Do you know how this is done? Do they use css like div > * or pass the className down?

Plus we need to consider that, except for trivial components, it would likely be bad practice to do the styling via HTML elements instead class names. If you style using class names the problem doesn't really apply.

If we support full css we cannot make this assumption.

deep

Using deep to pass the className to the component but then don't provide any guarantee that it will be set on the elements of the child component (because that depends on their implementation) could be very confusing.

Again I think that either passing it explicitly via css.resolve or using :global() is the best way to go and it is already implemented. Styling nested children with encapsulated CSS is a problem - this is a problem with ShadowDOM too etc and I'd rather wait to see if they come up with any interesting solution to this issue (right now they suggest to use css custom properties).

[nicolasmn]

I'm using this library to build a website currently and ran into this problem (thinking it was a bug).

I've read through the comments and think the css.resolve solution is far from acceptable for styling nested components.

In a real world application you compose components a lot. It is tedious to write every single selector that needs to be passed down in a different css.resolve function call and having to rename the className and style variables (which adds even more noise). Plus you loose the "real" classname by using that function.

The way it works currently makes me add a global attribute to nearly any styled jsx tag I write, which defends part of the purpose of this library.

An extra attribute for passing scoped styles would work, but changing the default to apply styles to child components would make more sense in my opinion.

[giuseppeg]

@nicolasmn unfortunately like in ShadowDOM this kind of encapsulation makes it very hard to style nested components. In the web standards world they suggest to use CSS custom properties to tweak values from a parent. With css.resolve and :global styled-jsx is making it a bit easier to tweak nested components but the problem is unsolvable in a robust way.

An extra attribute for passing scoped styles would work,

This is a very abstract statement, how would you pass scoped styles?

but changing the default to apply styles to child components would make more sense in my opinion.

We don't apply styles but best case we can pass the scoped className automatically to the component (basically what you'd do when using css.resolve). This could result in buggy behaviors as I described above.

[nicolasmn]

@giuseppeg thanks for the fast reply!

This is a very abstract statement, how would you pass scoped styles?

I meant an extra attribute on the <style jsx> tag like deep as discussed earlier in this issue.

We don't apply styles but best case we can pass the scoped className automatically to the component

Of course it's not the actual styles that would get passed but the scoped className, thanks for making that more clear.

This could result in buggy behaviors as I described above

I understand that passing a scoped className of "raw" html selectors would result in buggy behavior. However, these selectors are not the main use case anyways. Mostly I only want my "named" selectors passed down, like in the example below:

const HeaderMenuItem = ({ children }) => (
  <>
    <MenuItem className="HeaderMenuItem">
      <span>{children}</span>
    </MenuItem>

    <style jsx>{`
      /* This should be passed */
      .HeaderMenuItem { … }

      /* This should stay local */
      span { … }
    `}</style>
  </>
)

/* MenuItem implementation for reference */
const MenuItem = ({ className, ...props }) => (
  <>
    <div className={[ 'MenuItem', className ].join(,)} { ...props } />

    <style jsx>{`
      .MenuItem { … }
    `}</style>
  </>
)

[decimoseptimo]

Folks can do this with css.resolve, the reason why I don't want to add this option is that while className is part of the dom element API (i.e. every element can get a class attribute) this is not the case for React components. Imagine if you use React Router <Route> and that get passed a className, or another component that takes props and spreads them automatically on other elements. This could lead to many unexpected scenarios.

@giuseppeg Isn't css.resolve worse? it is yet another thing to research and remember, that spreads the code into multiple places. Perhaps we could use opt-in flags in the components themselves. Components that wouldn't otherwise inherit the jsx-XXX className.

#573 (comment)