johan-v-r / LibSassBuilder

Sass builder for .NET projects
MIT License
99 stars 14 forks source link

Compile sass only if changed, and build if files changed #8

Closed JelleHissink closed 3 years ago

JelleHissink commented 3 years ago

I had the issue when using "dotnet watch run" that it did not detect .scss changes. I added support and also conditional support for skipping the sass targets if nothing changed.

Kind regards, Jelle

johan-v-r commented 3 years ago

Hi Jelle, thanks for the PR! There's a lot of good value in here, and have a few questions...

Watching files

Another option to watch the files (instead of DisableFastUpToDateCheck) is:

<ItemGroup>
    <!-- extends watching group - https://docs.microsoft.com/en-us/aspnet/core/tutorials/dotnet-watch?view=aspnetcore-5.0#customize-files-list-to-watch -->
    <Watch Include="**\*.scss" Exclude="**\*.css" />
</ItemGroup>

I've been using that config which works pretty well (except for the Exclude which is described here) Can you see if that works for you? And perhaps also add in the docs as another option..?

Skip files if nothing changed

I like the idea of the hash, but not a massive fan of the .cache file tbh... There's currently a check to see if the file contents has changed. Looks like that condition is still in your changeset as well.

Checking the contents probably isn't the best for perf, but honestly hasn't had a massive impact yet. Perhaps we can think of another way to check the hash rather than storing in file?

Specify excluded files/folders

This has been requested previously, so really nice feature! Again though, I'd prefer not having to deal with another custom .sass-list file. These temp files can be very imposing and annoying if users don't want the feature.

The other thing to keep in mind is the usage with CLI via the global tool. So I'd really like to parse the args better with something like the Command Line Parser. Then the MSBuild props/target send those through as default args..?

Also would be nice to see how a consumer could specify/override that in their .csproj

JelleHissink commented 3 years ago

Watching

I tried something with CustomCollectWatchItems and _LibSassBuilderCustomCollectWatchItems in the .props and .targets files. That is enough to get dotnet watch run satisfied. However VS still excluded the project from the build.

Hashing

I know... I don't like files either, I noticed unchanged files don't get overwritten, as is the hash file. I made sure to calculate the hash in memory over filenames/modification dates. And read back the old value, only writing back if it has detected a change. Is there currently a better mechanism to prevent execution?

Files /folder for passing arguments

I know... I don't like temporary files either, esp the .sass-list. I tried to pass the files as argument to the tool. But it the Exec task could not be persuaded to execute all in one execution. It fired up the tool per file. That hit was somewhat high. So for now I did this as a workaround. The tool is already capable of having either multiple directories, files or .sass-list files passed to it.

I updated the readme to for the project options.

Future

It might be nice to be able to specify a target directory (per-css-file-basis?). I did not need that just yet, but it could be specified on the project file as an additional property. Or a default property in the project file.

JelleHissink commented 3 years ago

I just doublechecked... the incremental compile in VS is not working for me when using:

<ItemGroup>
    <Watch Include="**\*.scss" Exclude="**\*.css" />
</ItemGroup>

At least on my VS 2019, latest preview it is not recompiling if I only edit a .scss file.

My method of testing:

Build started...
1>------ Build started: Project: Blazor.Controls, Configuration: Debug Any CPU ------
1>Evaluating Sass files
1>Sass hash New = 22d60e817da9b280f81ad41be4cba50e8b7eb30b
1>Sass hash Old = 6215a3943c2c99048a8b07123a910d3a70004d35
1>Sass changed  = true
1>Sass compile file-list: obj\Debug\net5.0\Blazor.Controls.csproj.LibSassBuilder.sass-list
1>Sass files compiled
1>Blazor.Controls -> C:\Work\frontend\src\Blazor.Controls\bin\Debug\net5.0\Blazor.Controls.dll
========== Build: 1 succeeded, 0 failed, 0 up-to-date, 0 skipped ==========
Build started...
========== Build: 0 succeeded, 0 failed, 1 up-to-date, 0 skipped ==========
johan-v-r commented 3 years ago

I see what you mean with the DisableFastUpToDateCheck setting 👍 Thanks for updating those docs!

Not sure about that hashing yet, need some more time to think/investigate. I'll rather stick with the current implementation for now if that does the same job.

I've done some work on the console parsing args in this PR - your code gave me the idea to simply pass the args through, meaning the MSBuild task and CLI tool will work the same. So a BIG thanks for that! ⭐


I think what's best here is to update this, or create a new PR, for only the "file watching docs". The hashing will have to be separate as that's a more intense task and requires quite a few testing scenarios as well.

JelleHissink commented 3 years ago

I already commented on the other PR. I like the project, and would like it to besides from being a valuable commandline tool to integrate better in the MSBuild environment. I think we can find a way for that. But for the hashing... I see the code can be cleanedup. But consider...

Your current solution

How could this ever be more efficient then skipping the whole step after reading 1 file in the msbuild task? I agree it does not take ages, but that single hash-file is not that expensive, and entirely in line with what msbuild is doing.

JelleHissink commented 3 years ago

I restructured the tasks a little... found a way to join all file names to a single list (so no need for .sass-list anymore). Files are quoted and seperated by spaces.

johan-v-r commented 3 years ago

I like it! You can add a --files option that would handle them as mentioned on that other PR...

Then we probably don't need properties for DefaultSassExcludes nor EnableDefaultSassItems..?

Would it be useful to have a property to disable this MSBuild behavior? I'm really not sure here...

JelleHissink commented 3 years ago

I'll try to build this upon your version of the tool, I am just unsure on how this default directory would look in the commandline nuget package. But I'll give it a shot, you can always refactor again...

EnableDefaultSassItems & DefaultSassExcludes

For the EnableDefaultSassItems, it just removes the default item, so you are able to do this completely on your own terms. It is effectively the same as msbuild does for content and compile items. It is in line with what the default project system provides: https://docs.microsoft.com/en-us/dotnet/core/project-sdk/msbuild-props#default-item-inclusion-properties I thought there should be a way to at least exclude node_modules. So I modelled it after how it has worked ever since asp.net 3

johan-v-r commented 3 years ago

Ah okay think I understand... Those properties will assist MSBuild in which files to include/watch and send through to LSB..?

That might be a bit confusing with the --exclude flag as well, but I see the benefit of it. They also seem somewhat mutually exclusive, but could just be explained with good docs.

My initial thought is to then use your properties by default. But if <LibSassBuilderArgs> properties are provided, it should override all others. Thoughts?

JelleHissink commented 3 years ago

I'll create a new PR based on the new tool command arguments