koenvzeijl / AspNetCore.SassCompiler

Sass Compiler Library for .NET Core 3.1/5.x/6.x/7.x without node, using dart-sass as a compiler
https://www.nuget.org/packages/AspNetCore.SassCompiler/
MIT License
215 stars 26 forks source link

BeforeTargets="Build" is not sufficient to include CSS in build output #170

Closed mikes-gh closed 8 months ago

mikes-gh commented 8 months ago

Describe the bug The current BeforeTargets="Build;ResolveScopedCssInputs;BundleMinify" is not sufficient to include the generated assets in a Blazor library.

I have to use a copy of the library and use BeforeTargets="BeforeBuild"in order for the generated css to be included in published output without building twice.

Expected behaviour Include the generated css in build output even if it doesn't exist (e.g. in a clone publish scenario such as a CI pipeline).

Desktop (please complete the following information):

sleeuwen commented 8 months ago

Hi @mikes-gh ,

I'm not using Blazor myself, but we do have a Blazor sample app in this repository which does correctly generate scoped css from a scss file with a single build. Do you know if there is any difference between a Blazor app and a Blazor library? It would be helpful if you could provide a repo/instructions to reproduce the problem so we can investigate it further.

Thanks

mikes-gh commented 8 months ago

@sleeuwen Thanks for the response. The CSS is generated in my scenario as well.
However, if you do a dotnet pack or dotnet publish from a clean build (no CSS previously generated) the CSS will not be included since BeforeTargets="Build" happens after MSBuild analyses the staticwebassets to include. This means the below files would be missing and the library broken.

image

I'll work on a repo hopefully with the sample projects in this lib.

sleeuwen commented 8 months ago

Hi @mikes-gh ,

I took a look at our BlazorWasmSample and our RazorClassLibrary sample projects, and did a git clean -dxf . && dotnet pack -c Release -o publish to run dotnet pack on a clean build without previously generated .css files like you mentioned. For me this did correctly generate the .css files and included it within the published output.

After that, I also took a look at the MudBlazor/MudBlazor repository to see if I could reproduce the issue on there, I compiled the project located at src/MudBlazor from the current dev commit (5717ee). I had to change 2 things

  1. Update the PackageReference from MudBlazor.SassCompiler to the latest version of AspNetCore.SassCompiler
     <ItemGroup>
    -    <PackageReference Include="MudBlazor.SassCompiler" Version="2.0.4">
    +    <PackageReference Include="AspNetCore.SassCompiler" Version="1.71.1">
            <PrivateAssets>All</PrivateAssets>
         </PackageReference>
  2. Clean the _NewCompiledCssFiles msbuild property which is reused between the MudBlazor.csproj and the AspNetCore.SassCompiler.build.targets file, because of this it incorrectly added the wwwroot/MudBlazor.min.css file twice to the nupkg as it was already in the itemgroup when the AspNetCore.SassCompiler.build.targets added it to the Content items, so I added a line to clean that property before assigning it the generated files (The <Content Include="@(_NewCompiledCssFiles)" /> in the MudBlazor.csproj now doesn't do anything anymore as it's already included in the Content itemgroup)
     <ItemGroup>
    +    <_NewCompiledCssFiles Remove="@(_NewCompiledCssFiles)" />
         <_NewCompiledCssFiles Include="wwwroot\MudBlazor.min.css" Exclude="@(Content)" />
         <_NewCompiledJsFiles Include="wwwroot\MudBlazor.min.js" Exclude="@(Content)" />
         <Content Include="@(_NewCompiledCssFiles)" />
     </ItemGroup>

After these two changes I could run git clean -dxf . && dotnet pack, which generated a .nupkg that included the staticwebassets/MudBlazor.min.css file.

So as far as I can see now it should work correctly, but maybe I'm doing something different than you are doing.

mikes-gh commented 8 months ago

@sleeuwen thankyou for spending time on this. I spent several days on this and could never get it working. I was using an older version of AspNetCore.SassCompiler although AFAICS only changes recently are to do with source target. I'll confirm you steps this end and report back. I'll also need to check dotnet publish of our site as that is single target and it may be a timing thing that MudBlazor.min.css gets included in the pack as MudBlazor is built 3 times. net6/7/8.

mikes-gh commented 8 months ago

@sleeuwen Did you change this line? https://github.com/MudBlazor/MudBlazor/blob/5717ee8139bf63e6ef15f20ebefca11d07937086/src/MudBlazor/MudBlazor.csproj#L92

sleeuwen commented 8 months ago

@mikes-gh yes, in the item group for that target is where I added the line from the second diff

mikes-gh commented 8 months ago

Just to clarify you altered that line to remove DependsOnTargets="Compile Sass;?

sleeuwen commented 8 months ago

No, I left the DependsOnTargets as it is, I only changed it like this

  <Target Name="IncludeGeneratedStaticFiles" DependsOnTargets="Compile Sass;Compile JS" BeforeTargets="BeforeBuild">
    <Error Condition="!Exists('$(MSBuildProjectDirectory)/wwwroot/MudBlazor.min.css')" Text="Missing MudBlazor.min.css in wwwroot" />
    <Error Condition="!Exists('$(MSBuildProjectDirectory)/wwwroot/MudBlazor.min.js')" Text="Missing MudBlazor.min.js in wwwroot" />
    <ItemGroup>
      <!--Include without duplication-->
+     <_NewCompiledCssFiles Remove="@(_NewCompiledCssFiles)" />
      <_NewCompiledCssFiles Include="wwwroot\MudBlazor.min.css" Exclude="@(Content)" />
      <_NewCompiledJsFiles Include="wwwroot\MudBlazor.min.js" Exclude="@(Content)" />
      <Content Include="@(_NewCompiledCssFiles)" />
      <Content Include="@(_NewCompiledJsFiles)" />
    </ItemGroup>
  </Target>
mikes-gh commented 8 months ago

OK thats maybe why it works. SInce DependsOnTargets="Compile Sass;Compile JS" BeforeTargets="BeforeBuild"> may overide the current targets configuration in AspNetCore.SassCompiler

sleeuwen commented 8 months ago

Ahh I didn't notice the BeforeBuild there, I'll see if I can try some more things later this week

mikes-gh commented 8 months ago

Thanks for your time on this. I'll have a look too. You have stumbled on a good workaround though. I still think BeforeTargets="Build" is not enough. However, changing it to BeforeBuild means your GitHub repo will break, since using BeforeBuild requires a precompiled library to work and won't work with project references.,

mikes-gh commented 8 months ago

After spending a few hours on this I am beginning to think this is an incremental build issue. I can get it all to work if I clone the repo and use dotnet commands to pack or publish and the required files are included. This is using the package as is without any BeforeTargets overides.

See https://github.com/MudBlazor/MudBlazor/pull/8269

sleeuwen commented 8 months ago

Hi @mikes-gh, I may have found a solution, it seems that when packing your project it resolves the @(Content) items before the sass is compiled to css, in a target named ResolveProjectStaticWebAssets. I've added that target to the BeforeTargets for the "Compile Sass" target and when I run it locally I do now get the css published in the staticwebassets folder within the nuget. I've published a fix for this and it should be on nuget shortly as version 1.71.1.1. Can you see if that version works for you as well? if not please reopen this issue and I can look into it further.

mikes-gh commented 8 months ago

Thanks @sleeuwen. I have deployed the fix to MudBlazor Repo. I'll need to keep an eye on things but so far this looks like a fix. Thanks for all your time on this

mikes-gh commented 8 months ago

Just reporting back. Did a release via our CI pipeline and everything is working as expected. image

No need to mess around with the @(Content) variables either so project file is significantly simpler. Thanks a lot 🙏