Rework CompilationResult #315
Conversation
Making the CompilationResult object aware of the Compiler to check for cache freshness is weird, as the Compiler is the one with knowledge about the logic. The CompilationResult should be a value object.
a5f8410 said that this was making it consistent with the way dart-sass builds the list. But this was wrong, due to a bad analysis of the dart-sass behavior. Code generating a CSS import rule des not consider the URL of that import as an included file for the sass rendering.
|
I think I'm a bit lost. The idea of the CompilationResult was (for me) exactly to group and expose all informations needed to manage cache and/or cache invalidation logic to avoid in the future any new problem about consumers having issue with the cache invalidation : if the default (improved) logic was not fitting their need it was possible to manage it your own way. That said, I can fully see your logic of trying to lower as much as possible the public and exposed side of the lib, to allow a easier evolution in the future. But here I have the feeling we're almost back to the start, the CompilationResult beeing just a container for some additional data to the plain css, and not giving any more information than before (ie the fact that the result is a cached result or not, and the list of resolvedImports which is the keypoint if you want to make your own check on invalidation mecanism) I have to say I'm not available enough to spend sufficient time and dig on this subject to have a fully clear view. |
src/CompilationResult.php
Outdated
| public function getIncludedFiles() | ||
| { | ||
| return array_column($this->importedFiles, 'filePath'); | ||
| return $this->getIncludedFiles(); |
There was a problem hiding this comment.
Mistake here I think. Should be $this->includedFiles;
|
@Cerdic using the resolved import actually relies on being able to use #270 claimed to solve 2 issues at once:
why should you care whether the Compiler was able to reuse its cache or had to do the compilation again ? You still get the same result in both cases. I don't see a need for that.
That was precisely the request in #222: being a container against the extra additional data that was accessible through a stateful API of the Compiler before (which is an awful way to access it as the usability of
AFAICT, your implementation still does not allow any hooking inside the cache invalidation. You only get your hand on the CompilationResult object after the Compiler is done with it, and so caching is already done by that time. If the goal is to allow disabling the built-in compiler cache to handle caching in the caller while still having a proper cache invalidation, I can suggest an alternative API. We could have |
Tracking the data during the compilation is now done again internally in the Compiler. this gives more flexibility into refactoring the logic if necessary. It also avoids storing the CompilationResult after the end of the compilation for the getParsedFiles deprecated getter, which would keep a reference to the generated CSS and sourcemap as well, potentially increasing memory usage. Data used only by our caching layer for freshness checks is now kept outside the CompilationResult object, to avoid exposing it in the public API.
stof
force-pushed
the
rework_compilation_result
branch
from
f7ef795 to
72cb387
Compare
|
Feel free to go, as I said my mind is not able to get the big picture right now and this is not helping to challenge you |
|
@Cerdic the fact that you want to be able to check for cache invalidation in the embedder (see #316) is actually a good challenge for that suggested API. But do you need the embedded to be able to use the data of |
|
It depends of the level of consistency you need and if you can deal with some edge case. I was dealing with the previous invalidation implementation relying only on the If you want a reliable invalidation, the acces to the I think the implementation of the invalidation check in scssphp is now reliable enough to avoid the need of this api for now and we could postpone this work as there is already a lot to do besides that - except if we decide to get rid of the cache mecanism in the lib. |
|
My biggest issue with CompilationResult is that it saves the source map (if not embedded) into the filesystem. What if I wanted to upload the file into CDN instead of saving it locally? Or use FlySystem to save both the CSS and map? The class should only do what it is supposed to. |
|
@mahagr saving the sourcemap in the filesystem is indeed the point that still bothers me. But as long as we keep returning it from @Cerdic for now, I think I will merge this PR without the external access to the cache invalidation logic. Exposing that publicly later (if requested) is easy to do in a backward compatible way (while removing it from the public API is not) |
|
@stof let's go for that |
|
@stof Can you at least provide the alternate method to return CSS and map separately without saving anything to a file? There must be some way to do that without breaking things. :) |
3 participants
This reworks the CompilationResult result object introduced in #270. This has to be merged before the next release (otherwise CompilationResult will be covered by BC and prevent the changes)
Scssphp\Scssphp\CompilationResultis now an immutable value object instead of a mutable oneScssphp\Scssphp\CompilationResultnow longer exposes some internal API publiclyCachedResultobject wrapping the CompilationResult)parsedFileswhich has a deprecated getter)parsedFilesas there is the CSS and sourcemap in there)getParsedFilesis not exposed in CompilationResult, in favor ofgetIncludedFiles, which returns a list of file paths. When looking at various open-source packages using scssphp, any usage of$compiler->getParsedFiles()I found were actually usages ofarray_keys($compiler->getParsedFiles()/cc @Cerdic as you implement the initial version.