[react-jss] Root @media rules doesn't work with hook

[chapa]

Expected behavior: Generated classes of :

const styles = {
  '@media (min-width: 1024px)': {
    class1: {
      color: 'red'
    }
  },

  class2: {
    '@media (min-width: 1024px)': {
      color: 'red'
    }
  }
}

Should be like :

{
  class1: 'class1-0-0-1',
  class2: 'class2-0-0-2'
}

Describe the bug: When using createUseStyles(styles) (or the withStyles() HOC), generated classes are :

{
  '@media (min-width: 1024px)': undefined,
  class2: 'class2-0-2-2'
}

Note that it works well when using jss.createStyleSheet(styles), am I missing something ?

Codesandbox link: https://codesandbox.io/s/clever-firefly-w6dh8

Versions:

[zzuu666]

same issus happend to me.

[dyarfaradj]

@chapa @zzuu666 same issue, do you have any solutions?

[chapa]

@dyarfaradj for now I'm still in version 9 (I encountered this bug while trying to migrate to v10), but I'm thinking about switching to emotion instead (for this reason and others)

[kof]

Happy to accept a PR with a fix and a test

[maksym-boytsov]

I have the same issue, any updates?

[dyarfaradj]

No updates on this bug?

[chapa]

I digged a bit on this issue and I think have found where the bug comes from.

The function getSheetClasses returns an object based on getMeta(sheet).styles instead of sheet.classes, hence the difference between createUseStyles(styles)() and jss.createStyleSheet(styles).classes in the issue description.

Here's what I have in Chrome's debugger with the first @media rule (that wrap class1) of the example:

Is it normal that meta.styles doesn't reflect sheet.classes structure, since we use the former's keys and the latter's values ?

Remplacing for (const key in meta.styles) by for (const key in sheet.classes) seems to fix the bug, and all tests pass. But I'm wondering what is the point of the if (!meta) then.

@kof I can open a PR if you want, already have a test for it.

[kof]

I can see how getSheetClasses can be confusing, we need to add inline explaination and unit tests for this function as part of the fix.

  for (const key in meta.styles) {
    classes[key] = sheet.classes[key]

    if (key in dynamicRules) {
      classes[key] += ` ${sheet.classes[dynamicRules[key].key]}`
    }
  }

This essentially creates a classes object that combines classes from static and dynamic rules, which are technically already on sheet.classes, but the dynamic class names are not known to the consumer:

Lets say we have a sheet with this classes map, where "a" is a static class key and a{x} a dynamic one, which are used by different instances of the same component with different dynamic values.

sheet.classes = {a: 'a', a1: 'a1', a2: 'a2', a3: 'a3'}

We need to give to each instance a class name, that uses the right dynamic and static classes combination.

For example in an instance 1, we should receive:

sheet.classes = {a: 'a a1'}

When user accesses classes.a, they will get 2 classes, one static, one dynamic.

For instance 2, they would get

sheet.classes = {a: 'a a2'}

Feel free to incorporate this context into an inline comment if that makes sense to you.

[chapa]

Thank you for these explanations, I'll get on it soon.

[chapa]

Actually I'm not so sure to know what we want to do here.

Having:

const styles = {
  '@media (min-width: 1024px)': {
    class1: {
      display: 'block',
      color: ({color}) => color
    }
  }
}

We have in getSheetClasses:

sheet.classes = { class1: 'class1-0' }

getMeta(sheet) = {
  styles: {
    '@media (min-width: 1024px)': {
      class1: {
        color: ƒ(),
        display: "block"
      }
    }
  },
  dynamicStyles: {
    '@media (min-width: 1024px)': {
      class1: {
        color: ƒ()
      }
    }
  }
}

dynamicRules = {
  '@media (min-width: 1024px)': ConditionalRule {
    key: '@media (min-width: 1024px)-d0',
    rules: RuleList {
      classes: {
        class1: 'class1-0'
      },
      raw: {
        class1: {
          color: ƒ()
        }
      }
    }
  }
}

First of all, do you think those inputs are right ? (where is the dynamic class for color in sheet.classes ?) There is a difference in the inputs whether the @media is in class1 or class1 in @media:

// With:
const styles = {
  class1: {
    '@media (min-width: 1024px)': {
      display: 'block',
      color: ({color}) => color
    }
  }
}
// getSheetClasses gets:
sheet.classes = {
  class1: "class1-0",
  class1-d0: "class1-d0-1"
}
dynamicRules = {
  '@media (min-width: 1024px)-d1': ConditionalRule {…}
  class1: StyleRule {...}
}

// And with:
const styles = {
  '@media (min-width: 1024px)': {
    class1: {
      display: 'block',
      color: ({color}) => color
    }
  }
}
// getSheetClasses gets:
sheet.classes = {
  class1: "class1-0"
}
dynamicRules = {
  '@media (min-width: 1024px)': ConditionalRule {...}
}

Then I feel like this code doesn't handle the case where there is a ContainerRule in dynamicRules (here we have a ConditionalRule that implements ContainerRule) :

for (const key in meta.styles) {
  classes[key] = sheet.classes[key]

  if (key in dynamicRules) {
    classes[key] += ` ${sheet.classes[dynamicRules[key].key]}`
  }
}

A ContainerRule seems to be a composition of multiple Rule and the key of a ContainerRule is not what we want to use in this for loop. Do you think it would be relevant to have a method on BaseRule that would return "real" keys (keys of the final classes object) ? For example StyleRule would simply return this.key, ContainerRule would return an array of all "real" keys of child rules, GlobalRule would return an empty array, etc...

With that we could do something like:

for (const key in meta.styles) {
  if (key in sheet.classes) {
    // because key could be "@media..."
    classes[key] = sheet.classes[key]
  }

  if (key in dynamicRules) {
    dynamicRules[key].getKeys().forEach(k => {
      // here we're sure that k is a "real" key
      classes[k] += ` ${sheet.classes[k]}`
    })
  }
}

I don't a a global vision of the codebase so maybe I'm missing something.

[kof]

@chapa in the end, both syntaxes, media query inside a rule and a rule inside a media query need to work. I am not surprised if they don't, classes value should always have 2 class names, one from static, one from dynamic

[kof]

Do you think it would be relevant to have a method on BaseRule that would return "real" keys

Not sure I fully understand what you suggest, just know that BaseRule knows nothing about media queries or conditionals

[chapa]

@kof

@chapa in the end, both syntaxes, media query inside a rule and a rule inside a media query need to work. I am not surprised if they don't, classes value should always have 2 class names, one from static, one from dynamic

So you agree there is a problem before getSheetClasses gets called ? I'm pretty sure it came from addDynamicRules() in utils/sheet.js, and it's not impossible that other issues related to @media and dynamic values are caused by it.

Not sure I fully understand what you suggest, just know that BaseRule knows nothing about media queries or conditionals

Yes BaseRule knows nothing about media queries or conditionals, but here in getSheetClasses the key property of BaseRule is used as if every rules (including ConditionalRule) provides a single "class value", but it's wrong because rules inside a media query is represented by a ConditionalRule that have inner rules, so potentially multiple "class values".

For example, this:

const styles = {
  '@media (min-width: 1024px)': {
    class1: { color: 'blue' },
    class2: { color: 'red' }
  }
}

Is basically represented by:

ConditionalRule {
  key: '@media (min-width: 1024px)',
  rules: [
    StyleRule { key: 'class1' },
    StyleRule { key: 'class2' }
  ]
}

Where key is not at all a class value, class1 and class2 are.

Actually I may have use the wrong term by proposing this getKeys() method, what I meant is a getSomething() where "something" is how we call class1 and class2. Is "class value" the "official" term to name it in this library ? Maybe the method could be called getClassValues() then.

The point is to say that BaseRule knows that it represent something that could have class values, it could have one (e.g. StyleRule), many (e.g. ContainerRule) or even none (e.g. GlobalRule). So a getClassValues(): Array<string> method will allow to get the class value(s) without having to know which type of rule it is.

[kof]

So you agree there is a problem before getSheetClasses gets called ? I'm pretty sure it came from addDynamicRules() in utils/sheet.js, and it's not impossible that other issues related to @media and dynamic values are caused by it.

quite possible, yes

Where key is not at all a class value, class1 and class2 are.

style rules inside media queries require same named style rules on the top level to function, because otherwise we can't map them into a flat classes object

so essentially, the keys/class names inside media query don't matter, because there must always be a top level style rule with the same key/class name that we will be using, media query style rules, basically rely on those top level once.

[chapa]

Oh!

So actually there is no bug, from the beginning I made the assertion that this:

const styles = {
  '@media (min-width: 1024px)': {
    class1: {
      color: 'red'
    }
  },

  class2: {
    '@media (min-width: 1024px)': {
      color: 'red'
    }
  }
}

Should generate:

const classes = {
  class1: 'class1-0-0-1',
  class2: 'class2-0-0-2'
}

But it's wrong, this should instead be generated from:

const styles = {
  class1: {},
  '@media (min-width: 1024px)': {
    class1: {
      color: 'red'
    }
  },

  class2: {
    '@media (min-width: 1024px)': {
      color: 'red'
    }
  }
}

https://codesandbox.io/s/happy-ellis-f2k3i

I think it should at least be explicitly mentioned in the doc that there must always be a top level style rule with the same key that the ones inside the media query. A bit of shame that no one mentioned this before I spend hours trying to understand how things work and where is the problem...

However with the latter code, generated classes are:

const classes = {
  class1: 'class1-0-0-1',
  class2: 'class2-0-0-2',
  '@media (min-width: 1024px)': undefined
}

It's not that bad, but I feel like there still be something wrong somewhere.

[kof]

I think it should at least be explicitly mentioned in the doc that there must always be a top level style rule with the same key that the ones inside the media query.

I agree, lets mention it in the docs and even lets throw an error if this is not the case or alternatively make it so that it works when there is no top level rule, e.g. we could try register the css rule from media query in classes map, without adding that rule to the top level RuleList

I think this shouldn't be in the classes at all "'@media (min-width: 1024px)': undefined", I would say this issue should have a separate follow up issue for

1. making media css rule register a in classes or alternatively throwing (this way we don't actually need docs)
2. removing that media from classes

[chapa]

Sounds good :+1: