Provide a tooling gesture that suggests values for required component parameters are not specified. #33384

pranavkm · 2021-06-08T21:22:56Z

… parameters are not specified.

src/Components/Components/src/DynamicComponent.cs

src/Razor/Microsoft.AspNetCore.Razor.Language/src/BoundAttributeDescriptor.cs

NTaylorMullen · 2021-06-08T23:16:32Z

src/Razor/Microsoft.AspNetCore.Razor.Language/src/BoundAttributeDescriptorBuilder.cs

@@ -28,6 +28,8 @@ public abstract class BoundAttributeDescriptorBuilder

        public abstract RazorDiagnosticCollection Diagnostics { get; }

+        internal bool IsEditorRequired { get; set; }


Conflicted with this concept being on the BoundAttributeDescriptor vs. the RequiredAttributeDescriptor. I know the RequiredAttributeDescriptor has a slightly different implementation detail (aka effects if the TagHelper binds) but I still wonder if we're maybe crossing concerns. Did you happen to think more about the two options?

@SteveSandersonMS brought this up yesterday.

Right now, a tag helper attribute that is required but is missing causes the binding to fail. As a result, you lose colorization, don't get completion / documentation / tooltips, and you get the warning that says this tag looks like a component but did not bind.

If there's a way to improve this experience (keep colorization and completion and provide a better warning), we could use the current implementation.

NTaylorMullen · 2021-06-08T23:19:13Z

src/Razor/Microsoft.AspNetCore.Razor.Language/src/Components/ComponentLoweringPass.cs

            return component;
        }

-        private MarkupElementIntermediateNode RewriteAsElement(TagHelperIntermediateNode node)
+        private static void ValidateRequiredAttributes(TagHelperIntermediateNode node, TagHelperDescriptor tagHelper, ComponentIntermediateNode intermediateNode)


Hmmm, I'm not sure that this (the IR pass) is the right location to log diagnostics for this. Would it be possibel to log these diagnostics on the TagHelperElement syntax nodes? It'd enable us to have a better association with what element was the "problem" element and would enable tooling to react appropriately (aka generate the required attributes)

Any pointers on where that would be? This seemed like the earliest place where things like child components were resolved.

javiercn

Pretty solid change!

The only high level question I have is whether or not you have a way to turn off the warnings per component or whether we need to.

I'm thinking about how this works with component hierarchies where more specialized components might relax the requirements (for example provide a default value for a required parameter).

pranavkm · 2021-06-09T15:55:26Z

The only high level question I have is whether or not you have a way to turn off the warnings per component or whether we need to.

So an iffy workaround right now is to pass a splat attribute if you wanted to suppress it per callsite. Using a #pragma in a code block should let you apply suppressions inside the BuildRenderTree for a component. Neither of them are great, but we could punt on this for a bit until we get around to designing a @pragma directive.

javiercn · 2021-06-09T16:04:15Z

So an iffy workaround right now is to pass a splat attribute if you wanted to suppress it per callsite. Using a #pragma in a code block should let you apply suppressions inside the BuildRenderTree for a component. Neither of them are great, but we could punt on this for a bit until we get around to designing a @pragma directive.

I'm fine as long as we have a way, no matter how ugly it is.

Another way to go about this would be to have a directive to explicitly disable the checks.

…teDescriptor.cs

ghost · 2021-06-10T17:57:37Z

Thank you for submitting this for API review. This will be reviewed by @dotnet/aspnet-api-review at the next meeting of the ASP.NET Core API Review group. Please ensure you take a look at the API review process documentation and ensure that:

The PR contains changes to the reference-assembly that describe the API change. Or, you have included a snippet of reference-assembly-style code that illustrates the API change.
The PR describes the impact to users, both positive (useful new APIs) and negative (breaking changes).
Someone is assigned to "champion" this change in the meeting, and they understand the impact and design of the change.

SteveSandersonMS · 2021-06-11T10:28:01Z

...Microsoft.AspNetCore.Razor.Language/test/IntegrationTests/ComponentCodeGenerationTestBase.cs

+
+            var diagnostics = Assert.Single(generated.Diagnostics);
+            Assert.Equal(RazorDiagnosticSeverity.Warning, diagnostics.Severity);
+            Assert.Equal("RZ2012", diagnostics.Id);


This is a bit weird. It looks like there is child content:

<ComponentWithEditorRequiredChildContent> </ComponentWithEditorRequiredChildContent>

Syntactically it looks like the child content is a "newline" character. What makes this illegal? Maybe you actually want a single newline character as child content. Does the Razor compiler special-case this into "no child content parameter"?

Overall since [EditorRequired] isn't default, I wouldn't expect most developers to put it on a ChildContent parameter. I suppose if they do put it on, then they are really determined to get some child content so perhaps "whitespace" should count as violating their rule.

Summary: it's not obvious what the right behavior should be, but I guess I'm fine with [EditorRequired] erring on the side of strictness since if the developer wants not to be strict, they just wouldn't use it in the first place.

What do you think?

In this particular case, the child contents are named (it's Found and NotFound like the Router component), so this particular error makes sense. However, I wasn't sure what the behavior looked like when specifying text as a child content:

For a component with a parameter like so, here's the behavior:

[EditorRequired][Parameter] public RenderFragment ChildContent { get; set; }

✔️ Having non-whitespace text does not produce a warning eg. <MyComponent>Hello world</MyComponent>

✔️ Having any markup or component tag does not produce a warning e.g. <MyComponent><Counter /></MyComponent>

❌ One or more whitespace is treated as missing ChildContent which produces a warning: <MyComponent></MyComponent>

✔️ You can specify an explicit tag if you do want whitespace to be treated as valid content <MyComponent><ChildContent> </ChildContent></MyComponent>

I feel bullet no 3 should help guide the most common use case of the editor completing the tag for the user while 4 covers the case if you need to workaround any undue compiler restrictions.

SteveSandersonMS

Looks great. Really nice!

pranavkm · 2021-06-11T13:45:01Z

@NTaylorMullen my plan is to get this in for p6. We should be able to refactor this at a later time since none of the implementation details are public / breaking to anything outside the tooling.

En3Tho · 2021-06-21T09:37:09Z

@pranavkm @SteveSandersonMS
Will Razor support required properties in C# 10 with similar behaviour? Maybe it's quite early to ask but I'm interested in your opinion.

ghost · 2021-06-21T09:37:11Z

Hi @En3Tho. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

… parameters are not specified. (dotnet/aspnetcore#33384) * Provide a tooling gesture that suggests values for required component parameters are not specified. Fixes dotnet/aspnetcore#11815 Commit migrated from dotnet/aspnetcore@e02723975a97

fardarter · 2022-12-22T07:58:35Z

Something worth noting. Adding RZ2012 as a warning treated as an error will turn this into a build error: https://learn.microsoft.com/en-us/dotnet/csharp/language-reference/compiler-options/errors-warnings

ghost · 2022-12-22T07:58:39Z

Hi @fardarter. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

Provide a tooling gesture that suggests values for required component…

deb9d89

… parameters are not specified.

pranavkm requested review from Pilchie and a team as code owners June 8, 2021 21:22

pranavkm added the area-blazor Includes: Blazor, Razor Components label Jun 8, 2021

Clean up code

26ae54b

pranavkm requested a review from NTaylorMullen June 8, 2021 22:19

Delete more code

1bdd51d

NTaylorMullen reviewed Jun 8, 2021

View reviewed changes

javiercn reviewed Jun 9, 2021

View reviewed changes

Update src/Razor/Microsoft.AspNetCore.Razor.Language/src/BoundAttribu…

0b581f3

…teDescriptor.cs

pranavkm added the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jun 10, 2021

SteveSandersonMS reviewed Jun 11, 2021

View reviewed changes

SteveSandersonMS approved these changes Jun 11, 2021

View reviewed changes

Add a test for plain text child component

7d673f9

pranavkm enabled auto-merge (squash) June 11, 2021 13:49

pranavkm merged commit e027239 into main Jun 11, 2021

pranavkm deleted the prkrishn/required-parameters branch June 11, 2021 14:45

ghost added this to the 6.0-preview6 milestone Jun 11, 2021

pranavkm removed the api-ready-for-review API is ready for formal API review - https://github.com/dotnet/apireviews label Jun 14, 2021

guardrex mentioned this pull request Jul 9, 2021

Blazor 6.0 content tracking issue dotnet/AspNetCore.Docs#22045

Closed

41 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Provide a tooling gesture that suggests values for required component parameters are not specified. #33384

Provide a tooling gesture that suggests values for required component parameters are not specified. #33384

pranavkm commented Jun 8, 2021

NTaylorMullen Jun 8, 2021

pranavkm Jun 9, 2021

NTaylorMullen Jun 8, 2021

pranavkm Jun 9, 2021

javiercn left a comment

pranavkm commented Jun 9, 2021

javiercn commented Jun 9, 2021 •

edited

Loading

ghost commented Jun 10, 2021

SteveSandersonMS Jun 11, 2021

pranavkm Jun 11, 2021

SteveSandersonMS left a comment

pranavkm commented Jun 11, 2021

En3Tho commented Jun 21, 2021

ghost commented Jun 21, 2021

fardarter commented Dec 22, 2022

ghost commented Dec 22, 2022

		@@ -28,6 +28,8 @@ public abstract class BoundAttributeDescriptorBuilder

		public abstract RazorDiagnosticCollection Diagnostics { get; }

		internal bool IsEditorRequired { get; set; }

Provide a tooling gesture that suggests values for required component parameters are not specified. #33384

Provide a tooling gesture that suggests values for required component parameters are not specified. #33384

Conversation

pranavkm commented Jun 8, 2021

NTaylorMullen Jun 8, 2021

Choose a reason for hiding this comment

pranavkm Jun 9, 2021

Choose a reason for hiding this comment

NTaylorMullen Jun 8, 2021

Choose a reason for hiding this comment

pranavkm Jun 9, 2021

Choose a reason for hiding this comment

javiercn left a comment

Choose a reason for hiding this comment

pranavkm commented Jun 9, 2021

javiercn commented Jun 9, 2021 • edited Loading

ghost commented Jun 10, 2021

SteveSandersonMS Jun 11, 2021

Choose a reason for hiding this comment

pranavkm Jun 11, 2021

Choose a reason for hiding this comment

SteveSandersonMS left a comment

Choose a reason for hiding this comment

pranavkm commented Jun 11, 2021

En3Tho commented Jun 21, 2021

ghost commented Jun 21, 2021

fardarter commented Dec 22, 2022

ghost commented Dec 22, 2022

javiercn commented Jun 9, 2021 •

edited

Loading