-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[Blazor] CSS isolation follow-ups #25565
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
[Blazor] CSS isolation follow-ups #25565
Conversation
SummaryThis change includes several improvements to CSS isolation that we have gathered from validation and usage feedback. We have switched from producing a single scoped CSS bundle file for the entire application with all the scoped css files from the current project, referenced projects and package projects to producing one bundle per referenced project/package and to include those bundles into an "application" bundle throught CSS We have cleaned up the bundle names to make them more unique by including the project name on them and we have also cleaned up the bundle extensions. We have decided to put the individual bundles generated for the project scoped css assets into the static web assets base path of the project, so that when developers reference assets from their scoped css files (like using the CSS url function) the path they use matches what they have inside their library wwwroot folder. We have decided to put the application bundle on the root path of the application provided that the developer has not overriden the default StaticWebAssetsBasePath. This is so that the bundle location is consistent across templates, and can be found at For cases where the default StaticWebAssetBasePath has been overriden, the value is respected and the bundle is placed at Packaged razor class libraries with scoped css files now package a "project" bundle instead of the individual files. MotivationThe current way we bundled assets was problematic since it didn't allow package/library authors to deterministically reference assets within their scoped css files. In addition to that, there were a couple of bugs regarding the integration with Visual Studio that we discovered and that ammount to a suboptimal experience (users having to perform rebuilds to get their scoped CSS files/bundles updated) that we want to fix before RTM. Goals
Non-goals
Scenarios
RisksMSBuild changes are harder than normal code changes, but we have mitigated the risk with a substantial amount of tests that validate the majority of scenarios. We had to relax some constraints in Static Web Assets in general, but the default behavior still satisfies those constraints and bypassing those constraints requires using advanced extensibility. The constraints that we relaxed are the original limitation that all assets for a package had to share the same ContentRoot and BasePath (which in practice prevented any two assets url path from conflicting at development and publish time) Now individual assets have the freedom to define their own base paths individually. This is not a problem because we still use a constraint that we included while developing Blazor WebAssembly that validates that no two assets will conflict. We perform this check before we pack the assets on a package and before we generate a manifest for development, so users still get feedback at compile time if their application has a problem. Interaction with other parts of the frameworkThis feature integrates with static web assets, we had to modify a bit the way we package assets to support having assets on different base paths to keep consistency with what applications can do. We had to change the place in MSBuild where we generate the development manifest because it was causing unintended targets to run. The change is actually good since it was likely a latent bug in static web assets that we didn't know about. Detailed designDrawbacksIt requires more requests to load all the styles (1 additional request per referenced project/package) but the alternative is to merge everything into a single bundle, which makes it really hard to reference assets from the scoped styles. Considered alternativesSingle file bundle, which was the implementation we started with until we determined it wasn't good enough. Open questionsNone so far |
src/Components/WebAssembly/Sdk/integrationtests/WasmPublishIntegrationTest.cs
Outdated
Show resolved
Hide resolved
src/ProjectTemplates/Web.ProjectTemplates/content/BlazorServerWeb-CSharp/Pages/_Host.cshtml
Show resolved
Hide resolved
|
As part of doing this, would it be reasonable to update the RCL project template to make use of scoped styles now it's actually possible? If you don't want this PR to include that change it's fine, but we should definitely do it before RTM, which in practice means in a matter of days, so it might be lowest friction to include it here. Bonus points would be adding JS isolation to the RCL template. I'm very happy to do that myself if you prefer. Just let me know. |
| var relativePath = NormalizePath(bundle.GetMetadata("RelativePath")); | ||
| var importPath = NormalizePath(Path.Combine(prefix, bundleBasePath, relativePath)); | ||
|
|
||
| builder.AppendLine($"import '{importPath}'"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't the syntax @import 'url';, not import 'url'? (leading @ and trailing ;)
Just want to check we've definitely got coverage to say this works for real. If there's a possibility of adding an E2E test that uses getComputedStyle to verify that a external-package-provided scoped style has actually been applied, that would be ideal. If that's too much to add at this stage, let's have an issue tracking the need for E2E coverage and make sure we do a fair amount of manual verification.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I don't know how I missed that. I'll change it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Let's not miss the E2E test need though too, to check the style truly appears in the browser.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW there's already an E2E test that verifies styles from an RCL get applied properly: see CanUseComponentAndStaticContentFromExternalNuGetPackage in Microsoft.AspNetCore.Components.E2ETest.Tests.ComponentRenderingTest. It reads a computed style.
So it's probably not too hard to update how that test works so the style comes from a scoped CSS file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added an E2E test for this, just took one of the styles in the TestContentPackage styles and put it on a scoped css file.
...r/Microsoft.NET.Sdk.Razor/src/build/netstandard2.0/Microsoft.NET.Sdk.Razor.ScopedCss.targets
Show resolved
Hide resolved
src/Razor/Microsoft.NET.Sdk.Razor/test/Microsoft.NET.Sdk.Razor.Test.csproj
Show resolved
Hide resolved
src/Razor/Microsoft.NET.Sdk.Razor/test/ResolveAllScopedCssAssetsTest.cs
Outdated
Show resolved
Hide resolved
src/Razor/Microsoft.NET.Sdk.Razor/test/ResolveAllScopedCssAssetsTest.cs
Outdated
Show resolved
Hide resolved
|
Amazing work on the extensive test coverage here! |
I don't want to do it until I know everything else is working fine, there should be no discussion for that update. |
ba1655d to
0206eb6
Compare
…rom referenced projects to the app output folder
0206eb6 to
831d18b
Compare
|
🆙📅 |
Co-authored-by: Steve Sanderson <[email protected]>
171bb8e to
54efc22
Compare
SteveSandersonMS
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Brilliant!
|
@Pilchie this is good to go |
|
Approved for .NET 5 RC2 |
Description
This change includes several improvements to CSS isolation that we have gathered from validation and usage feedback.
We have switched from producing a single scoped CSS bundle file for the entire application with all the scoped css files from the current project, referenced projects and package projects to producing one bundle per referenced project/package and to include those bundles into an "application" bundle throught CSS @import statements.
We have cleaned up the bundle names to make them more unique by including the project name on them and we have also cleaned up the bundle extensions.
We have decided to put the individual bundles generated for the project scoped css assets into the static web assets base path of the project, so that when developers reference assets from their scoped css files (like using the CSS url function) the path they use matches what they have inside their library wwwroot folder.
We have decided to put the application bundle on the root path of the application provided that the developer has not overriden the default StaticWebAssetsBasePath.
This is so that the bundle location is consistent across templates, and can be found at ProjectName.styles.css independent of whether the app is a blazor webassembly app or a server side blazor app.
For cases where the default StaticWebAssetBasePath has been overriden, the value is respected and the bundle is placed at $(StaticWebAssetBasePath)/ProjectName.styles.css.
Packaged razor class libraries with scoped css files now package a "project" bundle instead of the individual files.
Customer Impact
Regression?
No
Risk
Low. The biggest risk factor is that there are MSBuild changes that can potentially impact the experience in Visual Studio, which is something that is hard to verify, but by getting this change in early we can access an early RC2 build, validate with a coherent build of the product and confirm there are no E2E issues. All that is in addition to all the tests that we have in place to prevent regressions and the new tests that we've added to validate the scenarios at the command-line level.
Fixes
#24337
#24583
#24300
#24257