Skip to content
This repository has been archived by the owner on Dec 14, 2018. It is now read-only.

Unable to utilize ModelExpression with [HtmlAttributeName(DictionaryAttributePrefix = "myprefix-")] Dictionary #5310

Closed
cjrosa opened this issue Sep 22, 2016 · 18 comments

Comments

@cjrosa
Copy link
Contributor

cjrosa commented Sep 22, 2016

Within razor, I've created the following:

<div myprefix-test="@Model.Value"></div>

I'd like to be able to create a tag helper with the following so I can create multiple [expression bound] attributes witht he same prefix.

     [HtmlAttributeName(DictionaryAttributePrefix = "myprefix-")]
        public IDictionary<string, ModelExpression> Values { get; set; } = new Dictionary<string, ModelExpression>();

However when running the application, I receive the following as runtime:

Dictionary values must not be of type 'Microsoft.AspNetCore.Mvc.ViewFeatures.ModelExpression'.

I should note that the razor tooling accepts the markup above.

Any thoughts would be greatly appreciated.

@danroth27
Copy link
Member

@NTaylorMullen @DamianEdwards Is there any specific reason to not support this?

@dougbu
Copy link
Member

dougbu commented Sep 22, 2016

MVC supports ModelExpression only when used as the type of a tag helper property.

Further this is a rather large feature request with an unclear upside. With no enforced commonality between the dictionary values (beyond being lambda expressions), about all the tag helper could do is evaluate the expressions and convert them to strings. What would you like to do with those values?

I should note that the razor tooling accepts the markup above.

How is that relevant?

@cjrosa
Copy link
Contributor Author

cjrosa commented Sep 22, 2016

Thanks for the quick responses @danroth27 & @dougbu...

The use case centers around [strongly-typed] template generation for Angular 2. For example:

<input ng-bind-ngModel="MyProperty" ... />

becomes...

<input [ngModel]="myProperty" ... />

Utilizing DictionaryAttributePrefix would enable a simple set of TagHelpers that correspond to Angular's binding syntax:

https://angular.io/docs/ts/latest/guide/template-syntax.html#!#binding-syntax

Being "generic" (e.g. ng-bind-XXX) would allow for any Component property being able to be bound in this way. This becomes especially important when targeting @input annotations on UI Components. For example:

<my-button ng-bind-title="someTitleProperty" ... />

becomes...

<my-button [title]="someTitleProperty" ... />

While I certainly appreciate the effort to support ModelExpression the commonality [or lack of it] should be of no consequence to the TagHelper framework. The Dictionary's value being a primitive type, a user-defined class/interface, object, etc provides no more "clarity" then ModelExpression as it's up to the author of the TagHelper to interpret these values accordingly, regardless.

As far as my note regarding the razor tooling, I was simply pointing out that this issue was not surfaced until run time. Ideally, such "things" are flagged at design time.

@dougbu
Copy link
Member

dougbu commented Sep 22, 2016

ModelExpression is not just a primitive type. Something needs to create the ModelExpressions and the lambda expression needs to be based on the Model type.

I'm still not understanding the value of generating property.Add("title", new ModelExpression(m => m.someTitleProperty)) versus property.Add("title", someTitleProperty) i.e. using a Dictionary<string, string> property and ng-bind-title="@someTitleProperty".

@cjrosa
Copy link
Contributor Author

cjrosa commented Sep 22, 2016

ModelExpression is not just a primitive type. Something needs to create the ModelExpressions and the lambda expression needs to be based on the Model type.

My apologies, I was not asserting that ModelExpression is a primitive type. Instead I was indicating that the Dictionary's Value type should be honored, regardless of type (e.g. primitive, class/interface, object, and ideally ModelExpression). I do understand, ModelExpression is special!!!

I'm still not understanding the value of generating property.Add("title", new ModelExpression(m => m.someTitleProperty)) versus property.Add("title", someTitleProperty) i.e. using a Dictionary<string, string> property and ng-bind-title="@someTitleProperty".

I need to get at the metadata for the Model's property. At a minimum, I need the property's name, but perhaps other metadata as I look to support more advanced templating scenarios. If I'm not mistaken, "@someTitleProperty" would simply yield that property's value, not its name (just tested and confirmed). Not to mention, that if the property was not a string (e.g. int),Razor seems to doing an implicit convert of int => string. In this case, someInt = 123 would generate [title]="123", this is obviously not the desired result.

@dougbu
Copy link
Member

dougbu commented Sep 22, 2016

Do you need both the property's name and it's value? I'd like to understand whether you need a ModelExpression versus an editing context which helps you type only model property names i.e. use a Dictionary<string, string> property and ng-bind-title="someTitleProperty" but prevent typing ng-bind-title="notAProperty" .

@cjrosa
Copy link
Contributor Author

cjrosa commented Sep 22, 2016

I do not need the value... In fact, in all likelihood, I won't even have a controller given that the Component I've been referencing is being utilized in lieu of an actual controller. So, no return View(new SomeModel()) necessary.

So I understand, in the ng-bind-title="someTitleProperty" markup, how are the following distinctions to be made/enforced?:

  • ng-bind-title="someTitleProperty" // The model someTitleProperty property's "Editing Context"
  • ng-bind-title="someTitleProperty" // The string literal "someTitleProperty"

@dougbu
Copy link
Member

dougbu commented Sep 22, 2016

how are the following distinctions to be made/enforced?

Your requirements are now much more clear. They might be satisfied any number of ways e.g. a new [HtmlAttributeName(OnlyModelProperties = true)] property and some tooling support. But your original Dictionary<string, ModelExpression> idea is likely less work. Put another way, I agree this is a valid use case for such a tag helper property -- the first we've heard about.

This issue is currently in the backlog, meaning we would accept a well-written PR filling the gap.

In the meantime, I'd recommend using a Dictionary<string, string> and having your customers use nameof in the .cshtml file e.g. ng-bind-title="nameof(Model.someTitleProperty)".

Your tag helper could also confirm the dictionary values match one of ViewContext.ViewData.ModelMetadata.Properties.Select(p => p.PropertyName). Hmm, that might be the cheapest solution here though it wouldn't be enforced at design time.

@cjrosa
Copy link
Contributor Author

cjrosa commented Sep 22, 2016

@dougbu, Thanks for the great exchange! The issue with @nameof is that you loose support for dot-notation:

@nameof(Model.MyClass.MyProperty) produces [title]="myProperty" when it should render [title]="myClass.myProperty", but...

Please pardon my ignorance, first time getting to this point. As for a PR, should I create one? is there a template? How to submit?

Thanks again!

@dougbu
Copy link
Member

dougbu commented Sep 22, 2016

I agree deeper properties get more complex when using the tag helper enforcement approach. Probasbly best to have users type in arbitrary dotted property names and recursively checking each segment (at runtime) in the tag helper.

Note Dictionary<string, ModelExpression> has something of the opposite problem: There's no restrictions on the lambda expression used: While asp-for="myClass.myProperty" is common, asp-for="@randomVariable[23].someOtherProperty" is completely supported.

Feel free to create a PR. I suspect the core change will be in MvcTagHelperAttributeValueCodeRenderer but the bulk of the effort will be on tests. Fork the MVC repo in your own GitHub repo, create a branch containing the feature, then submit a PR in this repo using that branch.

@danroth27 danroth27 added this to the Backlog milestone Sep 23, 2016
@danroth27 danroth27 modified the milestones: 1.0.1, Backlog, 1.1.0 Sep 23, 2016
@Eilon
Copy link
Member

Eilon commented Sep 28, 2016

@dougbu confirmed that this ought to be fairly straight-forward to fix - it mostly consists of removing the exception and updating tests.

@cjrosa
Copy link
Contributor Author

cjrosa commented Sep 28, 2016

I've just forked the repo and have begun to dig into the existing tests to see where best to incorporate then necessary test(s) as part of my TDD. I'm struggling to locate an existing set of tests that explicitly parses, generates AND renders the necessary markup.

@dougbu, You were right... the heavy-lifting was in the tests!

I was able to build out an specific TagHelper test (based on InputTagHelperTest for ModelExpression and FormTagHelperTest for prefix-value specification examples) which of course worked b/c the ModelExpression was explicitly added to the Dictionary of values annotated with DictionaryAttributePrefix.

In addition, I was able to adjust the Razor.Host test's ModelExpressionTagHelper.cshtml file to include an instance of my DictionaryAttributePrefix-based test tag helper. The code generated properly [seemingly] but never "renders" it thus the tests do not throw the aforementioned exception, thereby allowing me to resolve.

Any guidance would be greatly appreciated. I'd really like to run this down to completion.

@dougbu
Copy link
Member

dougbu commented Sep 28, 2016

@cjrosa updating the tests using InputTestTagHelper and ModelExpressionTagHelper.cshtml to cover IDictionary<string, ModelExpression> is a great start. That basically ensures the generated C# looks as expected.

Note even that test will fail unless you change MvcRazorParser. But you're correct confirming compilation succeeds and everything connects end-to-end would be useful here.

I suggest adding one similar functional test, extending the TagHelpersWebSite, that adds a couple of expressions to the dictionary. Not sure exactly what the new tag helper should do w/ the dictionary but fine as long as the response makes it obvious the expected entries are present.

cjrosa pushed a commit to cjrosa/Mvc that referenced this issue Sep 28, 2016
@dougbu dougbu self-assigned this Sep 30, 2016
dougbu pushed a commit that referenced this issue Sep 30, 2016
@dougbu dougbu added 3 - Done and removed 1 - Ready labels Sep 30, 2016
@dougbu
Copy link
Member

dougbu commented Sep 30, 2016

60d600d

@dougbu dougbu closed this as completed Sep 30, 2016
@dougbu
Copy link
Member

dougbu commented Sep 30, 2016

@cjrosa many thanks for finding this issue and then fixing it!

@deap82
Copy link

deap82 commented Jun 9, 2017

I have also created a tag helper that uses a dictionary of types <string, ModelExpression>, and it works great.

But when a view (.cshtml) that uses this taghelper is open in VS, building the solution still gives this output in the VS Error List:

Error Invalid tag helper property 'SomeNamespace.SomeTagHelper.some-prefix'. Dictionary values must not be of type 'Microsoft.AspNetCore.Mvc.ViewFeatures.ModelExpression'.

Is this some error in VS? I'll report it in the VS feedback tool as well.

@rynowak
Copy link
Member

rynowak commented Jun 9, 2017

@deap82 can you please open a new issue on https://github.com/aspnet/Razor and include a screenshot of the error in the errors window.

Posting feedback on a closed issue makes it really easy for us to lose track of it.

@deap82
Copy link

deap82 commented Jun 16, 2017

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants