kaystrobach / TYPO3.dyncss_less

Dyncss Less Adapter
http://forge.typo3.org/projects/extension-dyncss
5 stars 14 forks source link

Compiling new CSS file for each page #23

Closed mabolek closed 5 years ago

mabolek commented 5 years ago

KayStrobach\DyncssLess\Parser\LessParser->_checkIfCompileNeeded always returns true. This leads to the CSS file being compiled each time a page is generated.

In cases where TYPO3 concatenates CSS files, the CSS file's constantly changed timestamp will make TYPO3 generate a new file name hash each time. The number of files will grow uncontrolled, affecting available disk space on the server. Additionally, each page will have its own uniquely named CSS file, leading to slow page loads.

This can be fixed by making KayStrobach\DyncssLess\Parser\LessParser->_checkIfCompileNeeded return false.

A pull request is on its way.

kaystrobach commented 5 years ago

Thank you for your contribution.

@mabolek are you using this extension together with dyncss?

if so: https://github.com/kaystrobach/TYPO3.dyncss/blob/master/Classes/Parser/AbstractParser.php#L295

if ((file_exists($outputFilename)) && (!ApplicationContext::isDevelopmentModeActive() && (!$this->config['enableDebugMode']))) {
            return $outputFilename;
        }

should return the already compiled file and _checkCompile is only called if the env changes https://github.com/kaystrobach/TYPO3.dyncss/blob/master/Classes/Parser/AbstractParser.php#L299

//write intermediate file, if the source has been changed, the rest is done by the cache management
        if (@filemtime($outputFilename) < @filemtime($inputFilename) || $this->_checkIfCompileNeeded($inputFilename)) {

The function was explicitely made to ensure, that the parsing adapter can also implement an own caching algorithm like.

So i think, it's more important to understand, why the files which are generated do not exist on your system. This should lead us to the correct entry point to fix that problem centrally.

mabolek commented 5 years ago

Hi @kaystrobach!

Thanks for the detailed feedback. It helped me figure out that this was indeed a false positive. The issue must be elsewhere. I'm closing this issue.

kaystrobach commented 5 years ago

good to hear, if you find the root cause i would be glad to understand it and maybe add it to the documentation.

mabolek commented 5 years ago

I originally drew the wrong conclusion because my development environment was running in Development context and it also generated new files. It may be good to not automatically bypass the caching if running in Development context.

The real cause, however, was because we're running live sites on a continuous deployment platform. composer install will reinstall all the packages, including the one containing the LESS code to be compiled. This updates the modification dates of the LESS files, and dyncss will think the files have changed and generate new files on every redeploy. That's quickly a lot of new files on some multisite installations.

In the end, we chose to run rm -f web/typo3temp/assets/compressed/* on every new deployment.

It's possible basing the changed file check on a hash would be better for these kinds of situations.

kaystrobach commented 5 years ago

Having to recompile all files makes no sense imho. Maybe we should really check the content instead ...

Let me think about that.

kaystrobach commented 5 years ago

We also discussed to store the css file via fal, this would make it easier to store files in s3 etc.