Upgrade to Stylis 3

[kitten]

No description provided.

[kitten]

@thysultan I'm seeing some weird { ; } artefacts. Most of them disappeared when I switched on the compress option, but not all of them.

[mxstbr-bot]

	Warnings
⚠️	There are library changes, but not tests. That's OK as long as you're refactoring existing code

Generated by 🚫 dangerJS

[kitten]

@thysultan some duplication in ssr.test:

    -/* sc-component-id:sc-global-2303210225 */
    -body{ background:papayawhip; }
    +/* sc-component-id: sc-global-2358157387 */
    +{ ; }
    +body{ background:papayawhip}
    +body{ background:papayawhip}
    +body{ background:papayawhip}
    +body{ background:papayawhip}

[thysultan]

I'm seeing some weird { ; } artefacts.

This is by design, V3 adds semicolons where omitted to support no-semicolons, so

h1 {
    color:red
} -> h1{
    color:red;
}

some duplication in ssr.test:

This looks like a bug, whats the full input?

[thysultan]

@philpl patched some of theses in 3.0.1, BTW the cascade option should be true(default), false turns of cascading.

Do you know what is passed to stylis in this case

body{ background:papayawhip}
body{ background:papayawhip}
body{ background:papayawhip}

Edit: found the bug

[thysultan]

Managed to patch it...

Where do these empty blocks .sc-a { } come from?

[mxstbr]

Where do these empty blocks .sc-a { } come from?

Those are on purpose, they are per-component rather than the others, which are per-instance-of-css-fragment 😊

[thysultan]

I was trying to track down where and how they are passed to stylis, since stylis removes empty blocks its a bit odd that they would appear, if they are getting passed through it.

[thysultan]

Most failed tests now are whitespace related...

Is there a way to have the tests break up on a failed test to mitigate having to view all the failed tests at once.

[kitten]

Hiya, I'll finish this PR this evening :) yea, you can use jest's file filter with the P key

[kitten]

@thysultan found another bug... I think:

-  .sc-a{ } .c{ color: blue; } .c > h1{ font-size: 4rem; } .sc-b{ } .d{ color: blue; } .d > h1{ font-size: 4rem; } .d{ color: red; }
+ .sc-a{ } .c{ color: blue; } .c > h1{ font-size: 4rem; } .sc-b{ } .d{ color: blue; color: red; } .d > h1{ font-size: 4rem; }

Source:

    const Parent = styled.div`color: blue;`
    const Child = Parent.extend`color: red;`

It seems to be combining the two color properties across rules here, but I'm unsure whether this changes the behaviour in terms of specificity, due to it being the same, but in a different order? (A bit confused)

Edit: btw that's the last outstanding question/regression/issue/dunno

Edit 2: Ok, correct me afterwards if I'm wrong—tired at this point—but my abysmal knowledge of CSS specificity tells me that this is indeed a correct merge

[geelen]

Hmm, is the red in the above expected or what v3 is putting out. Because I think it's more correct.

[geelen]

Depends on what exactly is in that test case. Could you post a link to the source?

[kitten]

@geelen it's merging the two rules that have the same selectors in the correct order. So (see Edit2) i think it's correct.

[kitten]

@geelen the updated test case is here: 894415e

Wanna review this? :)

[thysultan]

It's by design, it now behaves like it would in sass/postcss we where talking about this before here #286. I decided it's better to have parity with scss to avoid unexpected results, i.e nested @mediablock issues.

BTW, is there any reason for this stringifyRules.js#L23-L25

  const cssStr = (selector && prefix) ?
    `${prefix} ${selector} { ${flatCSS} }` : // here
    flatCSS

// and
.replace(/^\s*\/\/.*$/gm, '') // replace JS comments

[kitten]

@thysultan yea, these lines are for several cases that need to line up with the preprocessing:

On the normal side: https://github.com/styled-components/styled-components/blob/master/src/constructors/keyframes.js#L19

On the preprocessing side we then use it like this: https://github.com/styled-components/styled-components/blob/master/src/no-parser/stringifyRules.js#L13

It's all stuff that will be cleaned up with v3, since the entire preprocessing logic and the way we'll use stylis will change then :) (as discussed prior)

[geelen]

Alright this looks good to me

[brunolemos]

@philpl don't you get this error? Maybe it's because I'm using react-native-web:

ReferenceError: escade is not defined

thysultan/stylis#30

[kitten]

@brunolemos Oh, I didn't get that error in any test. Weird. Seems to be fixed now and we'll just need to update the yarn lockfile on release