Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[fix]: support @layer #7514

Merged
merged 2 commits into from Jul 4, 2022
Merged

[fix]: support @layer #7514

merged 2 commits into from Jul 4, 2022

Conversation

kindoflew
Copy link
Contributor

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • Prefix your PR title with [feat], [fix], [chore], or [docs].
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Tests

  • Run the tests with npm test and lint the project with npm run lint

This should resolve #7504. I added a check for this.node.name === 'layer' inside the apply method of AtRule.

Let me know if there's anything else I should do. Thanks!

@baseballyama
Copy link
Member

If there is useless css inside @layer, should we purge it?

<div>hello</div>

<style>
	@layer base, special;
	@layer special {
		div {
			color: rebeccapurple;
		}
+              p {
+                      color: red;
+              }
	}
	@layer base {
		div {
			color: green;
		}
	}
</style>

@kindoflew
Copy link
Contributor Author

kindoflew commented May 6, 2022

interesting! i wasn't even aware that svelte purges unused CSS! 😂 i guess scoped styles make it easier for me to notice CSS i'm not using anymore.

my first thought is I don't know enough yet about @layer's use cases to know if there would be reasons to keep rules that the compiler might think are unused.

my second thought is that, since @layer is part of the natural hierarchy of the cascade, it should be treated like any other CSS -- so if svelte is purging other "normal" rules, we should purge here too.

@baseballyama
Copy link
Member

AFAIK @layer is just one of how to specify specificity.
So I thought we should do same way as normal CSS (It means we should purge it.)

@geoffrich Do you have opinion/thought?

@geoffrich
Copy link
Member

Yeah, I would expect unused CSS inside @layer to be purged, similar to how we purge unused styles inside @media. I was hoping we'd get that for free -- is it not happening?

Thanks for the quick PR @kindoflew!

<script>
	let name = 'world';
</script>

<h1>Hello {name}!</h1>
<style>
/* Warning: Unused CSS selector "h2" (8:2) */
	@media (min-width: 300) {
		h2 {
			color: red;
		}
	}
</style>

@kindoflew
Copy link
Contributor Author

kindoflew commented May 6, 2022

@geoffrich As far as I can tell, it is happening for free!

I just added an unused CSS rule to my test example locally and ran it and it still passed. Should I push that as well, just to cover the case?

@geoffrich
Copy link
Member

Perfect! I don't think we need to include it in the test case. Looking at the other @rule tests, they don't seem to be testing style removal specifically.

@baseballyama
Copy link
Member

Oh is it! Awesome!
Thank you for your fast PR and correspondence @kindoflew !

@baseballyama baseballyama merged commit 7e1691c into sveltejs:master Jul 4, 2022
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The contents of @layer in <style> are removed
4 participants