sveltejs / svelte

web development for the rest of us
https://svelte.dev
MIT License
80.09k stars 4.26k forks source link

Wrong CSS "Unused CSS selector" #14204

Open erwanvivien opened 2 weeks ago

erwanvivien commented 2 weeks ago

Describe the bug

I think we should relax the CSS verification for attributes

In my code I had to add data-active={false as true} to not trigger the first CSS selector Then I added the second CSS selector, but this one is not correctly picked up

Reproduction

<script lang="ts">
  //
</script>

<div class="floating-menu" data-active={false as true}>
  <div class="command-menu-tooltip"></div>
</div>

<style>
  /* This one is correctly picked up */
  .floating-menu[data-active='true'] > .command-menu-tooltip {
    background-color: red;
  }

  /* This one is not */
  .floating-menu[data-active='true'] {
    background-color: red;
  }
</style>

REPL

Logs

No response

System Info

System:
    OS: macOS 14.2.1
    CPU: (12) arm64 Apple M3 Pro
    Memory: 71.31 MB / 18.00 GB
    Shell: 5.9 - /bin/zsh
  Binaries:
    Node: 22.10.0 - ~/.nvm/versions/node/v22.10.0/bin/node
    npm: 10.9.0 - ~/.nvm/versions/node/v22.10.0/bin/npm
  Browsers:
    Chrome: 130.0.6723.93
    Safari: 17.2.1
  npmPackages:
    svelte: ^5.1.12 => 5.1.12

Severity

annoyance

Prinzhorn commented 2 weeks ago

Sorry I deleted my original comment. What exactly is broken here, there is no [data-active='true'] only [data-active='false']?

https://svelte.dev/playground/hello-world?version=5.1.12#H4sIAAAAAAAAA42QwW6DMBBEf2XlS1qpwJ0mSL3lH-IcHHtBVhcb2QtthPj32k4OjZRKPWFmZ-atdhVOjShacUQiD18-kIEXNJbRvIo30VvCKNrTKvg6ZV8Wkn5PfUxTHRckztpFRXyma-8YHacasY862ImBlBsOUnCUopMOoGlAun1zmyYl_Ri7gCYVY_L15BVbN1QjulkKMIpVpTTbBQ9rrygiqAgcZtxymB_C2o-jcqZkK_ae2E4Fm3xNMnaZfPumV-QrYdmpfqCefjN3hbk7Qwf1s3pYcwHARenPIfg5jbUnH1oIaN7zbMuw_zHWvKnkv7okb-V0973TvRm_WbTlGuftB3pYmw_hAQAA

This is not a boolean attribute so the value is stringified.

Leonidaz commented 2 weeks ago

If a variable is used for the data attribute then the css is good. Not sure how intentional this is but it makes sense to me as the css parser seems to be quite literal about the hard-coded values.

Use a variable instead

dummdidumm commented 2 weeks ago

I too do not fully understand what you're trying to show here. If the variable is false, then it's ok that it's marked as unused? Or is it about the weird discrepancy of one being marked as used and the other not?

Conduitry commented 2 weeks ago

I don't know what the false as true is supposed to do, but it does seem weird that the as true affects the scoping.

To me, the issue is that the first one isn't recognized as unused, not that the second one is.

erwanvivien commented 2 weeks ago

Thanks everyone for the comments

@Conduitry said

To me, the issue is that the first one isn't recognized as unused, not that the second one is.

Which is what I kinda want to point out, I would indeed skip the as true when checking the literal values (meaning trusting values instead of typing)

At the end we'd have two unused CSS selector

erwanvivien commented 2 weeks ago

I had this use case (which I changed later to using a state variable) but meanwhile I had this inner component using the old DOM API to add and remove data-attributes which meant I didn't care about the initial value

dummdidumm commented 1 week ago

I dug into this and it turns out that this goes back to our TS stripping algorithm not working correctly. While it removes the TS nodes, the parent and prev nodes become out of date - they point to the old nodes prior to stripping. This goes back to zimmerframe automatically creating new objects where needed so everything stays immutable. The most pragmatic solution IMO is to allow zimmerframe to mutate the AST instead, which solves this issue.

erwanvivien commented 1 week ago

I've added comments on the zimmerframe PR

My guts are telling me that mutating things is usually not the right way of doing things though