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

Do not emit empty rules/at-rules #16121

Merged
merged 9 commits into from
Jan 31, 2025
Merged

Conversation

RobinMalfait
Copy link
Member

@RobinMalfait RobinMalfait commented Jan 31, 2025

This PR is an optimization where it will not emit empty rules and at-rules. I noticed this while working on #16120 where we emitted:

:root, :host {
}

There are some exceptions for "empty" at-rules, such as:

@charset "UTF-8";
@layer foo, bar, baz;
@custom-media --modern (color), (hover);
@namespace "http://www.w3.org/1999/xhtml";

These don't have a body, but they still have a purpose and therefore they will be emitted.

However, if you look at this:

/* Empty rule */
.foo {
}

/* Empty rule, with nesting */
.foo {
  .bar {
  }
  .baz {
  }
}

/* Empty rule, with special case '&' rules */
.foo {
  & {
    &:hover {
    }
    &:focus {
    }
  }
}

/* Empty at-rule */
@media (min-width: 768px) {
}

/* Empty at-rule with nesting*/
@media (min-width: 768px) {
  .foo {
  }

  @media (min-width: 1024px) {
    .bar {
    }
  }
}

None of these will be emitted.

@RobinMalfait RobinMalfait force-pushed the fix/do-not-emit-empty-rules branch 2 times, most recently from 8b9caa0 to fe51b4a Compare January 31, 2025 14:30
@RobinMalfait RobinMalfait marked this pull request as ready for review January 31, 2025 14:31
@RobinMalfait RobinMalfait requested a review from a team as a code owner January 31, 2025 14:31
RobinMalfait and others added 8 commits January 31, 2025 16:00
The first occurrence is for `& {}`, which is an edge case already
The second occurrence is for normal rules that are empty
There are some exceptions to this rule. Some at-rules can be body-less:
```css
@charset "UTF-8";
@layer foo, bar, baz;
@Custom-Media --modern (color), (hover);
```
@RobinMalfait RobinMalfait force-pushed the fix/do-not-emit-empty-rules branch from c8952e3 to 92c2d1c Compare January 31, 2025 15:02
copy.name === '@layer' ||
copy.name === '@charset' ||
copy.name === '@custom-media' ||
copy.name === '@namespace'
Copy link
Member

@philipp-spiess philipp-spiess Jan 31, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this just be all at-rules? 🤔 Nevermind me here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@media (foo) {}

Should be removed imo

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah i noticed this too, you just responded before i could update my comment heh

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hehe, I think in theory we could invert the condition and don't touch everything but the @media (but then we need to check for @suspports as well.

In a perfect world @layer foo {} is also removed, (because you should use @layer foo; but we don't make that distinction.

@RobinMalfait RobinMalfait merged commit 7f1d097 into main Jan 31, 2025
5 checks passed
@RobinMalfait RobinMalfait deleted the fix/do-not-emit-empty-rules branch January 31, 2025 16:56
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.

3 participants