Fix minimized attributes when using 5.0 SDK with 3.x target #25307
Conversation
ghost
added
the
area-mvc
SteveSandersonMS
requested review from
a team,
NTaylorMullen and
ajaybhargavb
and removed request for
NTaylorMullen
| #nullable disable | ||
| ); | ||
| __builder.AddAttribute(2, "minimized-attr", true); | ||
| __builder.AddAttribute(3, "empty-string-atttr", true); |
There was a problem hiding this comment.
The above two lines show the compiler output for 3.x language version.
| #nullable disable | ||
| ); | ||
| __builder.AddAttribute(2, "minimized-attr"); | ||
| __builder.AddAttribute(3, "empty-string-atttr"); |
There was a problem hiding this comment.
The above two lines show the compiler output for 5.0+ language version.
| // Add the 'temp' compilation as a metadata reference | ||
| var references = BaseCompilation.References.Concat(new[] { tempAssembly.Compilation.ToMetadataReference() }).ToArray(); | ||
| projectEngine = CreateProjectEngine(RazorConfiguration.Default, references); | ||
| projectEngine = CreateProjectEngine(Configuration, references); |
There was a problem hiding this comment.
The two changes in this file from RazorConfiguration.Default to Configuration are to make the test behavior consistent. If a test subclass overrides the pre-existing Configuration property (which defaults to RazorConfiguration.Default) then we really should use that override instead of certain bits of code being hardcoded to use RazorConfiguration.Default directly.
This change is needed for the test that demonstrates the output on 3.x.
|
|
||
| public override bool SuppressNullabilityEnforcement { get; } | ||
|
|
||
| public override bool OmitMinimizedComponentAttributeValues { get; } |
There was a problem hiding this comment.
I've defined this new option, instead of using the RazorLanguageVersion directly inside the code writer, because:
- It seems like the existing philosophy is that the language version should be used to trigger the values for options, rather than code doing things directly based on the language version, so this is consistent.
- The code writer has no existing way of knowing the
RazorLanguageVersion(perhaps because of the philosophy mentioned above), and threading it through many layers would be quite disruptive, including to some public APIs
There was a problem hiding this comment.
Don't have to address in this PR but might want to add a new CodeGenerationFeatureFlags property to contain these similar to this. That makes it cleaner if we introduce more code generation features in the future.
|
Approved for RC1, pending CI completion. |
|
|
||
| public override bool SuppressNullabilityEnforcement { get; } | ||
|
|
||
| public override bool OmitMinimizedComponentAttributeValues { get; } |
There was a problem hiding this comment.
Don't have to address in this PR but might want to add a new CodeGenerationFeatureFlags property to contain these similar to this. That makes it cleaner if we introduce more code generation features in the future.
mkArtakMSFT
deleted the
stevesa/fix-minimized-attributes-with-3x-target
branch
This fixes a regression that was introduced in 5.0-preview8, reported by customers at #25267
What caused the regression
As part of perf work, in #23703 I had added an optimization whereby, for minimized attributes, we called a new
RenderTreeBuilder.AddAttributeoverload specialized to minimized attributes that was able to skip a series of checks and housekeeping steps that were unnecessary for minimized attributes. The Razor compiler was updated to use this newAddAttributeoverload for all minimized attributes.The optimization is particularly valuable because, with CSS scoping, components have a lot more minimized attributes than they used to. Every HTML element within a CSS-scoped component has at least one minimized attribute to represent the scope ID.
Why there's a regression
The above works great when targeting 5.0+ libraries. However if you're using the 5.0 SDK and targeting the 3.x libraries, then you have a problem because the new
AddAttributeoverload doesn't exist there, but the 5.0 compiler generates calls to it.How the fix works
As of this PR, for
RazorLanguageVersion< 5.0, the compiler doesn't call the new method but instead generates the sameAddAttributecall it used to.Risks
There's no new risk as long as the updated code actually works as intended.
Alternate workaround
Customers who want a workaround before 5.0-RC1 ships can either not install 5.0-preview8, or if they already have done, they can tell
dotnetto continue using the 3.x SDK for their project by adding aglobal.jsonfile, for example as suggested at #25267 (comment)