Media Query String Interpolation Bug

[zachhardesty7]

Just discovered what appears to be a bug with this library that causes media queries to be improperly parsed by placing the class name in the improper location. Unfortunately no error is produced so.

To reproduce

const mediaQuery = '@media only screen and (min-width: 426px) and (max-width: 767px)'

const Component = styled.div`
  background-color: red;
  ${mediaQuery} {
    background-color: blue;
  }
`

Without interpolating the string, it works fine, but the above produces a rule like this:

/* sc-component-id: sc-14zearg-1 */
.jmWrIX.jmWrIX{background-color: red;}
.jmWrIX.jmWrIX @media only screen and (min-width: 426px) and (max-width: 767px){background-color: blue;}

while it should produce a rule like this:

/* sc-component-id: sc-14zearg-1 */
.jmWrIX.jmWrIX{background-color: red;}
@media only screen and (min-width: 426px) and (max-width: 767px){.jmWrIX.jmWrIX{background-color: blue;}}

[nvanselow]

Thanks for the heads up. We will need to investigate and fix. I don't think we have any tests for media queries.

[zachhardesty7]

Looked at it a bit more and I'm not sure exactly how one would fix this. The syntax of the broken media query looks identical to using a element selector (ex: a test of yours below) but they're handled essentially opposite.

import styled from 'styled-components';

const Child = styled.span;

export default styled.div`
  position: relative;
  ${Child} {
    ${props => props.childStyles};
  }
  ${Child} + &, & + ${Child} {
    margin-right: ${props => props.spaceBetween};
  }
`;

Possible solutions?

• You can't evaluate expressions in Babel from my understanding, so that's out.
• You could just cancel the transform when a CSS rule starts with an expression which technically should work but isn't a whole lot better.
• The only other thing I can think of would be parsing the rest of the file and checking how the variable is declared but that seems difficult and maybe not possible?
• There is something that works currently, but it's a bit clunky. It needs a letter after the @ in order to work.

const styled = { div() {} };

const mediaQuery = 'only screen and (min-width: 426px)';

const MyStyledComponent = styled.div`
  background-color: red;
  @media ${mediaQuery} {
    background-color: blue;
  }
`;

export default MyStyledComponent;

I guess since it's not a common use-case (but is mentioned at least once by their team somewhere) it doesn't need to be fixed with much promptness. Documenting that this breaks it would be good for now. I'd be happy to drop a line in the README in my PR but I'm not sure what the standard procedure is for pushing failing tests. Let me know if I can help any other way!

[nvanselow]

Thanks for all the investigation. I think a note in the Readme would be a great idea for now. Let's disable the tests with a link to this issue and then check them in. That way, they are there for follow up investigation and hopefully a fix.

I think you are correct in your possible solutions that we'd have to check the contents and see if we can differentiate between media queries and other things.

[georanma]

Hi everyone,

Just wanted to mention that we are seeing this same kind of issue when considering the plugin for use in a styled-components with styled-systems set up. I have a project that we'd like to namespace, as it will be an app loaded into other sites. We want to do our best to prevent other site styles from over-riding out existing code. Unfortunately, @zachhardesty7 's suggestion does not appear valid for our use case, since the additional characters ahead of the styled-system identifier would break it.

[zachhardesty7]

@georanma Hmm, seems like you're right unfortunately. If you're just using it for a smaller sized app, rolling your own solution for styled-system is essentially what I did to tailor it to how I wanted to use it. I put custom arguments inside the getColor() call in my implementation. Simple proof of concept below...

const theme = {
  colors: {
    black: '#000e1a',
    white: '#fff',
    blue: '#007ce0',
    navy: '#004175'
  }
}

const getColors = () => ({ color, bg }) => ({
  color: theme[color] || color,
  backgroundColor: theme[bg] || bg
})

const StyledComponent = styled.div`
  ${getColors()}
`

const Component = () => (
  <StyledComponent color='black' bg='green'>test text to view</StyledComponent>
)

Since the part that seems to make this work is just the parenthesis after the function, you should be able to wrap their functions and call them. The performance impact should be minimal.

import { color } from 'styled-system'

const getColor = () => color

// Add styled-system functions to your component
const Box = styled.div`
  ${getColor()}
`

[georanma]

@zachhardesty7 Ill have to check this idea out a bit further. I threw your idea into one of my most used styled-components and the below didnt destroy anything.

const getFlexDirection = () => flexDirection;

const FlexRow = styled.div`
  display: flex;
  margin: ${props => (props.margin ? props.margin : '10px 0 10px 0')};
  ${width};
  ${height};
  ${alignItems};
  ${justifyContent};
  ${getFlexDirection()};
  ${space};
`;

It will be a bit tedious to move to it, but less so maybe than rolling our own. We do still need to work out a couple of dependency issues first, but this is the biggest hurdle we have in my eyes right now.

[georanma]

@zachhardesty7 after we finally got the plugin integrated with our base properly, we don't seem to need to make adjustments for media queries. Didn't get in to deep yesterday, but I'll post back Monday about any issues. May be a direction for your issue.

[georanma]

@zachhardesty7 it does look to be mostly working. I have experienced some bugs where depending on the placement of the styled systems template interpolation. So far, the main one I saw was with a themeGet call inside of a nested selector. When I moved the other styled system templates up the styles list, everything started working again. Im guessing it working may be an unintended side effect, but Id have to investigate after the project Im on is done.

[nvanselow]

Thanks for the investigations @georanma!