Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
mikes-gh opened this issue Feb 22, 2024 · 15 comments · Fixed by #172
Closed

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

mikes-gh opened this issue Feb 22, 2024 · 15 comments · Fixed by #172

Comments

@mikes-gh
Copy link

mikes-gh commented Feb 22, 2024

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):

  • OS: macOS Linux and probably windows
  • Browser Any
  • Version Any
@sleeuwen
Copy link
Collaborator

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
Copy link
Author

@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.

@mikes-gh mikes-gh changed the title BeforeTargets="Build" is not sufficient to include css in build output BeforeTargets="Build" is not sufficient to include CSS in build output Feb 23, 2024
@sleeuwen
Copy link
Collaborator

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
Copy link
Author

@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
Copy link
Author

mikes-gh commented Feb 26, 2024

@sleeuwen
Copy link
Collaborator

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

@mikes-gh
Copy link
Author

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

@sleeuwen
Copy link
Collaborator

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
Copy link
Author

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

@sleeuwen
Copy link
Collaborator

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

@mikes-gh
Copy link
Author

mikes-gh commented Feb 26, 2024

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
Copy link
Author

mikes-gh commented Feb 29, 2024

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 MudBlazor/MudBlazor#8269

@sleeuwen
Copy link
Collaborator

sleeuwen commented Mar 1, 2024

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
Copy link
Author

mikes-gh commented Mar 3, 2024

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
Copy link
Author

mikes-gh commented Mar 4, 2024

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 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants