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 issue with @apply rules not generating all the rules when the target is comma separated #3678

Closed

Conversation

27pchrisl
Copy link

@27pchrisl 27pchrisl commented Mar 3, 2021

Fixes #3360, Fixes #3154, Fixes #3306

TW has an issue generating rules during @apply when the target selector is part of a comma separated list of selectors (unless you're applying the first selector in a comma separated set). All three issues related to @apply have this root cause. Trying to manipulate a comma separated list and getting the right output with no duplicates causing specificity issues is tricky, however there's a shortcut which is to flatten the incoming CSS before starting.

So

h1, h2 {
  color: blue;
}

becomes

h1 {
  color: blue;
}

h2 {
  color: blue;
}

before we even start processing. The downside(?) is that the development mode CSS files become larger (about 25k larger). But the upside is that I believe all the @apply bugs are fixed. The duplicates are recombined during production builds by cssnano, so there's no production downside.

(Edit: example now uses slightly modified version of @RobinMalfait's comment #3360 (comment))

Input
.aspect-w-9,.aspect-w-16 {
  position: relative;
  padding-bottom: calc(var(--tw-aspect-h) / var(--tw-aspect-w) * 100%);
}

.aspect-w-9 > *,.aspect-w-16 > * {
  position: absolute;
  height: 100%;
  width: 100%;
  top: 0;
  right: 0;
  bottom: 0;
  left: 0;
}

.aspect-w-16 {
  --tw-aspect-w: 16;
} 

.aspect-h-9 {
  --tw-aspect-h: 9;
}

.sixteen-by-nine {
  @apply aspect-w-16 aspect-h-9;
}
Was
.aspect-w-9,.aspect-w-16 {
  position: relative;
  padding-bottom: calc(var(--tw-aspect-h) / var(--tw-aspect-w) * 100%);
}

.aspect-w-9,.aspect-w-16 > * {
  position: absolute;
  height: 100%;
  width: 100%;
  top: 0;
  right: 0;
  bottom: 0;
  left: 0;
}

.aspect-w-16 {
  --tw-aspect-w: 16;
}

.aspect-h-9 {
  --tw-aspect-h: 9;
}

.aspect-w-9,.aspect-w-16 {
  position: relative;
  padding-bottom: calc(var(--tw-aspect-h) / var(--tw-aspect-w) * 100%);
}

.aspect-w-9 > *,.aspect-w-16 > * {
  position: absolute;
  height: 100%;
  width: 100%;
  top: 0;
  right: 0;
  bottom: 0;
  left: 0;
}

.sixteen-by-nine {
  --tw-aspect-w: 16;
  --tw-aspect-h: 9;
}
Is now
.aspect-w-9,.aspect-w-16 {
  position: relative;
  padding-bottom: calc(var(--tw-aspect-h) / var(--tw-aspect-w) * 100%);
}

.aspect-w-9,.aspect-w-16 > * {
  position: absolute;
  height: 100%;
  width: 100%;
  top: 0;
  right: 0;
  bottom: 0;
  left: 0;
}

.aspect-w-16 {
  --tw-aspect-w: 16;
}

.aspect-h-9 {
  --tw-aspect-h: 9;
}

.sixteen-by-nine {
  position: relative;
  padding-bottom: calc(var(--tw-aspect-h) / var(--tw-aspect-w) * 100%);
}

.sixteen-by-nine > * {
  position: absolute;
  height: 100%;
  width: 100%;
  top: 0;
  right: 0;
  bottom: 0;
  left: 0;
}

.sixteen-by-nine {
  --tw-aspect-w: 16;
  --tw-aspect-h: 9;
}

@RobinMalfait I believe this is why you didn't see the issue when you created the test, but you did see it in the playground - you need the target to be in a list.

@codecov-io
Copy link

codecov-io commented Mar 4, 2021

Codecov Report

Merging #3678 (054723b) into master (ea3bd20) will decrease coverage by 0.02%.
The diff coverage is 91.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3678      +/-   ##
==========================================
- Coverage   93.34%   93.32%   -0.03%     
==========================================
  Files         178      178              
  Lines        1849     1858       +9     
  Branches      332      335       +3     
==========================================
+ Hits         1726     1734       +8     
- Misses        105      106       +1     
  Partials       18       18              
Impacted Files Coverage Δ
src/lib/substituteClassApplyAtRules.js 94.79% <91.66%> (-0.33%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea3bd20...054723b. Read the comment docs.

@alperyazgan
Copy link

As of today with version 2.2.4, it does not solve the problem defined in #3154

@27pchrisl
Copy link
Author

@adamwathan I rebased this on master - does this fix work for you?

@RobinMalfait
Copy link
Member

Hey! Thank you for your PR!
Much appreciated! 🙏

We fixed this in v3, thank you for your efforts!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants