-
Notifications
You must be signed in to change notification settings - Fork 10.4k
Provide a tooling gesture that suggests values for required component parameters are not specified. #33384
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
Provide a tooling gesture that suggests values for required component parameters are not specified. #33384
Changes from 4 commits
deb9d89
26ae54b
1bdd51d
0b581f3
7d673f9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
|
||
namespace Microsoft.AspNetCore.Components | ||
{ | ||
/// <summary> | ||
/// Specifies that the component parameter is required to be provided by the user when authoring it in the editor. | ||
/// <para> | ||
/// If a value for this parameter is not provided, editors or build tools may provide warnings indicating the user to | ||
/// specify a value. This attribute is only valid on properties marked with <see cref="ParameterAttribute"/>. | ||
/// </para> | ||
/// </summary> | ||
[AttributeUsage(AttributeTargets.Property, AllowMultiple = false)] | ||
public sealed class EditorRequiredAttribute : Attribute | ||
{ | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,4 +1,4 @@ | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Copyright (c) .NET Foundation. All rights reserved. | ||
// Licensed under the Apache License, Version 2.0. See License.txt in the project root for license information. | ||
|
||
using System; | ||
|
@@ -28,6 +28,8 @@ public abstract class BoundAttributeDescriptorBuilder | |
|
||
public abstract RazorDiagnosticCollection Diagnostics { get; } | ||
|
||
internal bool IsEditorRequired { get; set; } | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Conflicted with this concept being on the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @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. |
||
|
||
public virtual IReadOnlyList<BoundAttributeParameterDescriptorBuilder> BoundAttributeParameters { get; } | ||
|
||
public virtual void BindAttributeParameter(Action<BoundAttributeParameterDescriptorBuilder> configure) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -67,7 +67,7 @@ protected override void ExecuteCore(RazorCodeDocument codeDocument, DocumentInte | |
} | ||
} | ||
|
||
private ComponentIntermediateNode RewriteAsComponent(TagHelperIntermediateNode node, TagHelperDescriptor tagHelper) | ||
private static ComponentIntermediateNode RewriteAsComponent(TagHelperIntermediateNode node, TagHelperDescriptor tagHelper) | ||
{ | ||
var component = new ComponentIntermediateNode() | ||
{ | ||
|
@@ -89,13 +89,54 @@ private ComponentIntermediateNode RewriteAsComponent(TagHelperIntermediateNode n | |
// because we see the nodes in the wrong order. | ||
foreach (var childContent in component.ChildContents) | ||
{ | ||
childContent.ParameterName = childContent.ParameterName ?? component.ChildContentParameterName ?? ComponentMetadata.ChildContent.DefaultParameterName; | ||
childContent.ParameterName ??= component.ChildContentParameterName ?? ComponentMetadata.ChildContent.DefaultParameterName; | ||
} | ||
|
||
ValidateRequiredAttributes(node, tagHelper, component); | ||
|
||
return component; | ||
} | ||
|
||
private MarkupElementIntermediateNode RewriteAsElement(TagHelperIntermediateNode node) | ||
private static void ValidateRequiredAttributes(TagHelperIntermediateNode node, TagHelperDescriptor tagHelper, ComponentIntermediateNode intermediateNode) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Any pointers on where that would be? This seemed like the earliest place where things like child components were resolved. |
||
{ | ||
if (intermediateNode.Children.Any(c => c is TagHelperDirectiveAttributeIntermediateNode node && (node.TagHelper?.IsSplatTagHelper() ?? false))) | ||
{ | ||
// If there are any splat attributes, assume the user may have provided all values. | ||
// This pass runs earlier than ComponentSplatLoweringPass, so we cannot rely on the presence of SplatIntermediateNode to make this check. | ||
return; | ||
} | ||
|
||
foreach (var requiredAttribute in tagHelper.EditorRequiredAttributes) | ||
{ | ||
if (!IsPresentAsAttribute(requiredAttribute.Name, intermediateNode)) | ||
{ | ||
intermediateNode.Diagnostics.Add( | ||
RazorDiagnosticFactory.CreateComponent_EditorRequiredParameterNotSpecified( | ||
node.Source ?? SourceSpan.Undefined, | ||
intermediateNode.TagName, | ||
requiredAttribute.Name)); | ||
} | ||
} | ||
|
||
static bool IsPresentAsAttribute(string attributeName, ComponentIntermediateNode intermediateNode) | ||
{ | ||
foreach (var child in intermediateNode.Children) | ||
{ | ||
if (child is ComponentAttributeIntermediateNode attributeNode && attributeName == attributeNode.AttributeName) | ||
{ | ||
return true; | ||
} | ||
else if (child is ComponentChildContentIntermediateNode childContent && attributeName == childContent.AttributeName) | ||
{ | ||
return true; | ||
} | ||
} | ||
|
||
return false; | ||
} | ||
} | ||
|
||
private static MarkupElementIntermediateNode RewriteAsElement(TagHelperIntermediateNode node) | ||
{ | ||
var result = new MarkupElementIntermediateNode() | ||
{ | ||
|
Uh oh!
There was an error while loading. Please reload this page.