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

Required parameters to blazor components #11815

Closed
Tracked by #27883
montyclt opened this issue Jul 2, 2019 · 36 comments · Fixed by #33384
Closed
Tracked by #27883

Required parameters to blazor components #11815

montyclt opened this issue Jul 2, 2019 · 36 comments · Fixed by #33384
Assignees
Labels
affected-most This issue impacts most of the customers area-blazor Includes: Blazor, Razor Components Components Big Rock This issue tracks a big effort which can span multiple issues Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-blazor-component-model Any feature that affects the component model for Blazor (Parameters, Rendering, Lifecycle, etc) Priority:2 Work that is important, but not critical for the release severity-minor This label is used by an internal tool User Story A single user-facing feature. Can be grouped under an epic.
Milestone

Comments

@montyclt
Copy link

montyclt commented Jul 2, 2019

Is your feature request related to a problem? Please describe.

I'm building a component where optional parameter doesn't make sense.

Describe the solution you'd like

Add required parameter to Parameter attribute

[Parameter(required = true)]
protected string Name { get; set; }
@Eilon Eilon added the area-blazor Includes: Blazor, Razor Components label Jul 2, 2019
@Andrzej-W
Copy link

This is an old idea. It was moved from original Blazor repository to this #5455. Unfortunately it is now closed.

@montyclt
Copy link
Author

montyclt commented Jul 3, 2019

Thank you @Andrzej-W. I close it as duplicate.

@montyclt montyclt closed this as completed Jul 3, 2019
@Andrzej-W
Copy link

@montyclt Maybe you shouldn't close. As I wrote above issue #5455 was closed a few months ago but I'm afraid [Parameter(required = true)] hasn't been implemented yet and I cannot find any other tracking item for this.

@rynowak rynowak reopened this Jul 3, 2019
@rynowak
Copy link
Member

rynowak commented Jul 3, 2019

Reopening this. We do plan to do a feature like this but it won't happen in 3.0.

@rynowak rynowak added this to the Backlog milestone Jul 3, 2019
@rynowak rynowak added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Jul 3, 2019
@mikewodarczyk
Copy link

The implementation of this may need to mix with the #nullable variable feature in c# 8. The null checks will flag [Parameter(required = true)] protected T myVar {get; set;} as uninitialized, but in fact this attribute is saying that the variable must be initialized; but it is initialized elsewhere.

@danroth27 danroth27 removed this from the Backlog milestone Sep 1, 2019
@danroth27
Copy link
Member

Clearing the milestone as I'd like to consider this for 5.0 now that we have a bunch of components that have required parameters.

@mkArtakMSFT mkArtakMSFT added this to the 5.0.0-preview1 milestone Sep 3, 2019
@buddyfavors
Copy link

This could have came in handy for me too, for now I just wrapped the property.

        [Parameter]
        public string MessageText
        {
            get => _MessageText ?? throw new NotImplementedException();
            set => _MessageText = value;
        }

        private string _MessageText { get; set; }

@kattunga
Copy link

Is not possible to add this to 3.1?
now we to set #nullable disable in each params section to disable nullable warnings.

@ZimM-LostPolygon
Copy link

now we to set #nullable disable in each params section to disable nullable warnings.
@kattunga

Use null! instead, much more nice:

    [Parameter]
    public UserEditViewModel UserEdit { get; set; } = null!;

@chucker
Copy link

chucker commented May 20, 2020

Use null! instead, much more nice:

That works, but it really just tells the compiler to ignore the problem.

It'd be nicer if we can have the compiler surface the problem on the component consumer's end:

<MyComponent /> <!-- compiler warning: UserEdit isn't set -->
<MyComponent UserEdit="@foo" /> <!-- no warning! -->

@javiercn javiercn added the feature-blazor-component-model Any feature that affects the component model for Blazor (Parameters, Rendering, Lifecycle, etc) label Apr 19, 2021
@JvanderStad
Copy link

JvanderStad commented May 14, 2021

I have solved this in a basic way with the following extension method during runtime:

#nullable enable

using System;
using System.Collections.Concurrent;
using System.Collections.Generic;
using System.ComponentModel.DataAnnotations;
using System.Linq;
using System.Reflection;
using FluentAssertions.Common;
using Microsoft.AspNetCore.Components;

namespace Blazor
{
	public static class ComponentExtension
	{
		public record ParameterInfo(PropertyInfo PropertyInfo, bool IsRequired);
		private static readonly ConcurrentDictionary<Type, ParameterInfo[]> _typeInfo = new();

		/// <summary>
		/// Validate parameters
		/// </summary>
		/// <param name="component"></param>
		public static void ValidateParameters(this IComponent component)
		{
#if DEBUG
			var componentType = component.GetType();
			if (!_typeInfo.TryGetValue(componentType, out var parameterInfo))
			{
				var props = componentType
					.GetProperties()
					.Where(x => x.IsDecoratedWith<ParameterAttribute>());

				var list = new List<ParameterInfo>();
				foreach (var prop in props)
				{
					list.Add(new ParameterInfo(prop, prop.GetCustomAttribute<RequiredAttribute>() != null));
				}

				_typeInfo[component.GetType()] = parameterInfo = list.ToArray();
			}

			foreach (var property in parameterInfo.Where(x=>x.IsRequired) )
			{
				var value = property.PropertyInfo.GetValue(component);
				if (value == null)
					throw new ArgumentNullException(property.PropertyInfo.Name);

				switch (property.PropertyInfo.PropertyType)
				{
					case Type t when t == typeof(Guid):
					{
						if ((Guid)value == default)
							throw new ArgumentNullException(property.PropertyInfo.Name);

						break;
					}
				}
			}
#endif
		}
	}
}

In every component where I want to have validation I call this method

[Parameter] [Required] public Guid Id { get; set; } = default!;
protected override void OnInitialized()
{
	this.ValidateParameters();
}

@mrpmorris
Copy link

@JvanderStad the issue isn't checking in the component at runtime, but checking in the consumer at compile time.

@JvanderStad
Copy link

@JvanderStad the issue isn't checking in the component at runtime, but checking in the consumer at compile time.

I'm aware of this, but it's better than nothing. I'm just sharing what works for me right now.
With a little luck the compile-time version will use the same attribute.

@stefanloerwald
Copy link

Solving it at compile time has been possible (and stable) for many months now (see above comments). No need to work with runtime-only. The only real question is whether this should be a separate attribute ([Required][Parameter] public T Value {get;set;}) or as part of the Parameter attribute ([Parameter(required=true)] public T Value {get;set;}).

@JvanderStad
Copy link

Didn't see that. Ah wel ¯_(ツ)_/¯

@mikewodarczyk
Copy link

The only real question is whether this should be a separate attribute ([Required][Parameter] public T Value {get;set;}) or as part of the Parameter attribute ([Parameter(required=true)] public T Value {get;set;}).

Would one ever use a [Required] attribute without a [Parameter] attribute? I don't think so. Based on that, it seems like the [Parameter(required=true)] makes more sense. The "required" modifies the "Parameter" attribute, but is not an attribute on its own.

@stefanloerwald
Copy link

I'm not in the position to make that decision, nor do I have a strong preference either way. But there is a reason for a separate attribute, and that is precedent and existing code: RequiredAttribute (System.ComponentModel.DataAnnotations) https://docs.microsoft.com/en-us/dotnet/api/system.componentmodel.dataannotations.requiredattribute?view=net-5.0

@mrpmorris
Copy link

One attribute = fewer reflection calls

@stefanloerwald
Copy link

At compile time, there's no concern about reflection. It doesn't matter to a source generator whether there are one or two attributes.

@Dalqin
Copy link

Dalqin commented May 14, 2021

What precedent is there? In data annotations, [Required] is independent; it doesn't rely on any other attribute to define its usefulness.
In this case, [Required] would only mean something if [Parameter] is present, yeah?

@montyclt
Copy link
Author

I think using [Required] could be confusing because [Required] is used by some components like EditForm in other properties. I vote for using [Parameter(Required = true)].

@ghost ghost added the Working label May 27, 2021
@ghost ghost added Done This issue has been fixed and removed Working labels Jun 11, 2021
pranavkm added a commit that referenced this issue Jun 11, 2021
… parameters are not specified. (#33384)

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

Fixes #11815
@ghost ghost locked as resolved and limited conversation to collaborators Jul 11, 2021
dougbu pushed a commit to dougbu/razor-compiler that referenced this issue Nov 17, 2021
… 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
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
affected-most This issue impacts most of the customers area-blazor Includes: Blazor, Razor Components Components Big Rock This issue tracks a big effort which can span multiple issues Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one feature-blazor-component-model Any feature that affects the component model for Blazor (Parameters, Rendering, Lifecycle, etc) Priority:2 Work that is important, but not critical for the release severity-minor This label is used by an internal tool User Story A single user-facing feature. Can be grouped under an epic.
Projects
None yet