Svelte: CSS minification broken in 3.13

[Conduitry]

Describe the bug
The simple CSS minification that the compiler once performed on component styles seems to have largely stopped working.

Logs
N/A

To Reproduce

<div>Hey</div>

<style>
    div {
        font-size: 200%;
        color: red;
    }
</style>

Expected behavior
The styles this produces should continue to be lightly minified - whitespace should be removed, semicolons right before closing braces should be removed, etc. This stopped working between 3.12.1 and 3.13.0.

Stacktraces
N/A

Information about your Svelte project:
3.15.0, but present since 3.13.0.

Severity
Fairly low, as this doesn't result in any bad behavior, just unnecessary bytes.

Additional context
None.

[Conduitry]

Wow this is way more insidious than I thought it was going to be. It's somehow related to the compiler getting into a bad state from previous components that it had compiled. As best as I can tell: If the very first thing you do with the compiler is to compile a component with component styles, they will be properly minified. But if you compile a component with no component styles, and then later compile a component that does have component styles, the second component's styles will not be minified.

[Conduitry]

More precisely: It looks like all that matters is whether the very first component that's compiled has component styles. If it does, all is well - it doesn't matter how many component without styles there are later down the line. If the first one does not, there are problems.

This issue is actually what was causing the weird test failures I noticed in #3945 - and those test failures made me notice that it's not just that a failure to minify that happens - it also causes media queries to be dropped. For example:

const svelte = require('./compiler');
svelte.compile('');
console.log(svelte.compile('<div></div><style>@media(min-width: 1px) { div { color: red; } }</style>').js.code);

This results in styles of @media(min-width: 1px){}, and the .svelte-xyz class isn't added to the div.

This actually does mean incorrect results, not just suboptimal results, so the priority is probably more than 'fairly low'.

[Conduitry]

It looks like this was broken by #3870. I'm seeing the issue in commit 4ffa6fc0314b8ab1c9dce69d5694ca11a39a1965 but not in de80ae72c3db8853109b39a841cd6562b65e45af.

[Conduitry]

It's related to code-red's handling of multi-line /* comments */, apparently. Of all things.

If I remove the /* ${banner} */ from the generated code, this works, even if I'd previously compiled a component with no styles. The single-line // ${this.comment} we're generating elsewhere seems to not be an issue.

[Conduitry]

If I install the latest version of code-red (which is 0.0.26), the issue persists. If I symlink the current master branch of code-red (which is also 0.0.26), the issue goes away. The versions of the various dependencies that code-red has in its repo seem to be the same as the ones that are in svelte's package-lock.json, so I'm currently guessing that this issue is masked by having two instances of some or code-red's dependencies in the final bundle.

[mrkishi]

Alright, this is definitely being caused by estree-walker's childKeys cache. If I export childKeys from the compiler and do this, the problem goes away:

const svelte = require('./compiler');
svelte.compile('');
for (const key in svelte.childKeys) delete svelte.childKeys[key];
console.log(svelte.compile('<div></div><style>@media(min-width: 1px) { div { color: red; } }</style>').js.code);

[mrkishi]

So, it turns out there's a node called Block with children in the styles' ast, but there's also Block without children for block comments from estree.

Seems like the short-term fix is just adding childKeys.Block = ['children'];, but that doesn't sound optimal.

[Conduitry]

The relevant confusion about the shape of the AST seems to be resolved by adding

childKeys.Block = ['children'];

when we're initing the walker.

[Rich-Harris]

heroic efforts 🙇