DOM event modifiers prevent styles from being generated
taylorzane opened this issue · 13 comments
I tried to make the title as succinct as possible, but this is a really odd bug.
If you have a component that has DOM event modifiers (on:click|once) and are using a preprocessor for styles, the styles will not be generated/output.
More interestingly, this appears to be the case even when you have two separate components (that do not import each other) where one has no modifiers and the other one does. No styles will be generated for this bundle.
Another oddity is if you run two separate rollup builds in one config (export default [/* Array of configs */]) all of the builds will not contain styles.
Check out a repro here
Instructions:
$ yarn install
$ rollup -c build/rollup.config.js
# Observe that *both* outputs do not contain any styles
$ rollup -c build/rollup.no-mod.config.js && rollup -c build/rollup.with-mod.config.js
# Observe that the no-mod output has styles and with-mod output does not.
I was unable to reproduce this when not using svelte-preprocess-sass.
Something I did notice is if I set svelte3/lint-template to false (in the repro repo it's set to true) the bug doesn't occur.
After completing phase one of debugging: this is still really weird.
This issue doesn't occur consistently for me. There's some sort of race condition going on here, but I could get this to happen not too infrequently. It does seem to only happen with svelte3/lint-template enabled. Through some console.logs I could see that the preprocessed component going into the compiler did have the (compiled SASS) styles in it and the compiled component did not contain the styles.
The AST walker that the lint-template option uses is the exact same instance as the compiler uses when walking the AST. The AST walker (estree-walker) caches the valid keys on each node type. (I imagine this is for performance reasons, but I don't immediately see how it would make much of a difference.)
My best/only theory about what's happening here is that while walking the AST in this plugin (which mutates the tree as it goes in a hacky but convenient way to avoid traversing bits it doesn't want to traverse), estree-walker gets (and caches) some wrong ideas about the shape of various AST nodes, so that when Svelte does go to compile them the walker skips certain child nodes that it thinks don't exist. And certain things just don't show up in the compiled output.
I have no idea yet what to do about this.
Is this something that we should take up with estree-walker since it seems like the issue is stemming from a caching problem on that side?
I had been hoping that I could introduce some artificial delays into the plugin chain to get this error to always occur, but I wasn't able to quickly get that to happen. I'll have to make due with the issue occurring half-ish of the time on my machine.
I did try disabling the caching in estree-walker, and the build here in this repro has now succeeded about 20 times in a row, so I'm pretty sure this is working.
I did bring up this issue with @Rich-Harris about the caching and what he had to say was "it was for performance reasons — I remember it having some impact, though I can't remember how large a one [...] mutating the AST is probably always going to have... interesting results. i guess it depends what you're doing with it though? we should test that perf thing, it'd be nice if caching was unnecessary"
The simplest answer is to remove the caching in estree-walker (and release a new version of Svelte), but it's yet to be seen whether this is an acceptable answer. Another possibility is to have estree-walker reset the cache one per tree you ask it to walk. We're still trying to figure out where and how we want to address this.
I do recall that it seemed like I was deleting keys from the AST for a good reason. I was leery about doing that while walking, but it really did seem like the easiest way to prevent certain things from being walked.
Had an idea of how to make this work in a semi-sane way without requiring any changes to Svelte or estree-walker.
Can you give this a go with eslint-plugin-svelte3 1.2.1 1.2.2 and see whether this is resolved?
Phenomenal, I'll test first with the repro repository and the again with my primary repo and see how this goes.
Using version 1.2.2 still fails when following the original repro steps.
yarn.lock
eslint-plugin-svelte3@^1.2.2:
version "1.2.2"
resolved "https://registry.yarnpkg.com/eslint-plugin-svelte3/-/eslint-plugin-svelte3-1.2.2.tgz#eaf573a32cfcfdcee6a8ee97d5c203956f236dbe"
integrity sha512-PS9Kid/Lg9L+RnxJ3nUEBdTJ0F/RAWaAt1ufiEZ50wVdhiNeY+MHfs8rUZWqMG2iPz+K3Oq5Bv0eTza8QNj9ZA==
Darn, okay. Well I'm still glad I made the changes that this issue has spurred me to so far, but I will take another look at this when I can.
Dug some more into this weirdness and it continues to be weird.
Some of the AST nodes that estree-walker is walking do not have a type field. The walker does still try to cache their child keys though (under the lookup undefined), and it uses the same cache for all of the nodes with undefined type. I suppose ideally all of the nodes that Svelte's parser generates would have types, but in the meantime it still seems prudent for estree-walker to not try to cache those.
The root of the CSS AST does not have a type. Ordinarily this bug is hidden because apparently the compiler doesn't walk any other type-less nodes first, and so the CSS root's children get cached as the undefined keys. And while estree-walker is walking an AST for a component with even modifiers, it actually walks down into the strings once/preventDefault/etc., and these obviously have no type, and so if that happens first, it messes up the style generation.
Ideally, every node the Svelte parser generates would have a type. And it also might be nice if estree-walker didn't walk down into actual strings and stopped at nodes. But the most practical solution here is to probably make estree-walker not cache child keys when looking at a node with no type.
The estree-walker fixes have been released, and I just bumped the dependency in sveltejs/svelte@673a3c9 so this should be all set whenever the next version of Svelte is released.
A version of Svelte with this fix has been finally released as 3.5.0. Can you give that a try please?
Yep, I'll give this a shot this morning.
It appears to be working! This is awesome, thank you very much.