Using important: true leads to duplicate rules in output

[willdean]

This issue supersedes #9203 which was a bit of a false start. V3.1.18 Tailwind CLI Node v14.18.1 Chrome Window 10 (No Docker or WSL) Command line : npx tailwindcss --output twout.css --watch -i twbase.css

Reproduction URL: https://github.com/willdean/TwRepro1

Describe your issue

When the important option is set to true, modifications to the source file cause TW to emit duplicate rules into the output for each update.

Repro steps:

1. Start the CLI, using the command line above (or tw.bat on Windows)
2. Observe that the output file twout.css contains just two rules (after the boilerplate):

   .ml-2 {
   margin-left: 0.5rem !important
   }
   .ml-4 {
   margin-left: 1rem !important
   }

3. Modify the index.html file so that the ml-4 selector on the second div becomes ml-6 and save the file
4. Verify that the CLI regenerates the output file twout.css

The output file now contains

.ml-2 {
  margin-left: 0.5rem !important
}
.ml-4 {
  margin-left: 1rem !important
}
.ml-2 {
  margin-left: 0.5rem !important
}
.ml-6 {
  margin-left: 1.5rem !important
}

i.e. the .ml-2 rule has been duplicated.

This will happen each time a modification to the source file is made - there will be an additional .ml-2 rule generated.

Some commentary:

I have spent some time trying to understand this, and I think there may be two separate problems, though I'm not a JS developer by trade, and I don't really understand TW's architecture, so please forgive me if I'm completely wrong here.

Possible Issue 1 - Each time a file is modified, TW adds duplicates of all the rules related to that specific file to context.stylesheetCache. This is nothing to do with whether the important configuration option is set or not. However, this does not normally cause duplication in the output, because the code which later converts the list of rules into css code elides all the duplicates. This may however be a part of the slow-down/leak problem in #7449.

Possible Issue 2- The code which converts the rules list into CSS (is this Postcss?) is apparently able to elide duplicate rules, and that normally deals with the duplicates which build-up in the cache. However, this elision features partly breaks (not completely - some rules get repeated 'n' times, whereas some only see one duplicate) when the important option is set.

If issue 1 is dealt with then issue 2 possibly wouldn't matter. In #9203 I posted some very crude work-around code which stops the duplicates in the cache.

Edit: This issue doesn't seem to repro in https://play.tailwindcss.com/

[willdean]

In case this raises questions about PostCSS:

>npm -g list postcss
C:\Users\will\AppData\Roaming\npm
`-- tailwindcss@3.1.8
  +-- postcss-import@14.1.0
  | `-- postcss@8.4.16 deduped
  +-- postcss-js@4.0.0
  | `-- postcss@8.4.16 deduped
  +-- postcss-load-config@3.1.4
  | `-- postcss@8.4.16 deduped
  +-- postcss-nested@5.0.6
  | `-- postcss@8.4.16 deduped
  `-- postcss@8.4.16

[thecrypticace]

I don't think this is likely to be related to PostCSS. We do some internal cache clearing stuff on Tailwind Play. Since there's only a single content input source we have it set up to act essentially as if it were a fresh build.

[willdean]

Thanks Jordan - I don't properly comprehend the flow of:

(List-Of-Rules-with-lots-of-duplicates) => [A miracle happens here] => (CSS file with fewer duplicates)

and thought that the miracle might be PostCSS.

Anyway, the duplicates have been annoying us for months and months, so it was nice to get to some level of understanding.

[thecrypticace]

Yeah I'm not sure why the duplicates some time disappear and sometimes don't but I can absolutely reproduce it in the CLI so I'm taking a look. I've also been able to create a script that makes the rule cache grow significantly which reproduces the slowdown issue. That's a small but significantly helpful data point as well!

[willdean]

The point at which the duplicate is created in the cache is returnValue.utilities.add(rule); in buildStylesheet. I wondered if there had originally been the intent that, being made from a Set object, the cache would inherently not hold duplicates. Of course, from the PoV of a Set, the duplicates are not the same, so that doesn't work.

[adamwathan]

I wouldn’t be surprised if it did used to be the same rule instance but in fixing another bug we’ve introduced a regression (probably by cloning the rule somewhere) that is causing duplicates to appear. Candidates (like ml-4) should be pulling their corresponding rule from a cache if they have been generated already, so should have been the same rule instance originally I think.

[thecrypticace]

Alright that duplicate issue is taken care of — as well as the memory leak with the ruleCache. It is append-only be design but we shouldn't be just appending things to it without re-using them. That's definitely a bug. Whoops. Thanks for the repro and analysis. That's super helpful!

Could you give the insiders build a test and let us know what you see?

[willdean]

LGTM.