Replace @bind with bind-... #406

rynowak · 2018-03-27T22:37:40Z

This change introduces a 'tag helper' that replaces @Bind with custom
code generation that accomplishes roughly the same thing.

This feature lights up by dynamically generating tag helpers that are
visible to tooling and affect the code generation based on:

pattern recognition of component properties
attributes that create definitions for elements
a 'fallback' case for elements

'bind' also supports format strings (currently only for DateTime) via
a separate attribute.

This change introduces the basic framework for bind and tooling support.
We know that we'll have to do more work to define the set of default
'bind' cases for the DOM and to flesh out the conversion/formatting
infrastructure.

This change gets us far enough to replace all of the cases we currently
have tests for :) with the new features. The old @Bind technique still
works for now.

Examples:

@* bind an input element to an expression *@
<input bind="@SelectedDate" format="mm/dd/yyyy" />
@functions {
    public DateTime SelectedDate { get; set; }
}

@* bind an arbitrary expression to an arbitrary set of attributes *@
<div bind-myvalue-myevent="@SomeExpression">...</div>

@* write a component that supports bind *@

@* in Counter.cshtml *@
<div>...html omitted for brevity...</div>
@functions {
    public int Value { get; set; } = 1;
    public Action<int> ValueChanged { get; set; }
}

@* in another file *@
<Counter bind-Value="@CurrentValue" />
@functions {
    public int CurrentValue { get; set; }
}

tdinucci · 2018-03-29T10:05:54Z

@rynowak

The old @Bind technique still works for now.

I take this to mean you will be removing the old @Bind support fairly soon which will mean https://github.com/aspnet/Blazor/issues/386 will then no longer be an issue?

SteveSandersonMS

Looks superb! It's really great that we're able to add such significant (and nontrivial) helpers and have them so fully integrated into the tooling.

Let me know if you want to discuss renaming format-* to bind-format-*. I don't feel strongly about any of the other CR comments that I left :)

SteveSandersonMS · 2018-03-29T11:30:21Z

src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BindLoweringPass.cs

+                    child.TagHelper != null &&
+                    child.TagHelper == attributeNode.TagHelper &&
+                    ((child.AttributeName == "format" && attributeNode.AttributeName == "bind") ||
+                    (child.AttributeName == "format-" + valueAttributeName)))


I think we should rename this to bind-format rather than just format.

Reason: By using bind, we're already taking a bit of a gamble that people don't want to set regular HTML attributes with the name bind. As of today that's almost certainly true. But there is some chance that in the future, bind could be given native DOM behaviors as part of new web standards, and then we'd need to change the name we use for this (e.g., to Bind with a leading capital, which will never be a legal native attribute name, or to data-bind, which will never be used in specs, or something else).

I'm fine with this tradeoff for bind since it's so important that it feels friendly in typical uses today, and we always have a way to change it in the future if we need (breaking, but still a solution).

For format, however, I don't feel quite that the tradeoff is worth it. format is perfectly likely to be a reserved word either in future native DOM standards or for third-party UI libraries. Renaming to bind-format arguably makes its meaning even clearer, since it emphasises how it's a modification of bind rather than something in its own right.

What do you think @rynowak?

it it possible we could pick something that doesn't share a prefix with bind-? This degrades the UX of the fallback cases - bind-value-onchange and bind-format-value are the same thing from tooling's point of view. We do whatever we want for code generation, but intellisense and completion are fairly primitive.

We could also consider doing something like value-format and just decoupling format from bind totally. I'm pretty sure we have the ability to do matching based on a suffix.

Oh OK that's a good point. From the point of view of not clashing with native web standards, anything with a hyphen is very likely sufficient based on historical precedent. Whatever you think makes most sense and works best in tooling, and also contains a hyphen, would be fine with me!

format-value or value-format (particularly the latter) sound good to me.

OK, I'm going to try value-format, and keeping it coupled to bind-... for now.

So what that means is that you'd have to write <input type="date" bind="@SelectedDate" value-format="MM/dd/yyyy" /> - you have to know what the attribute is that you're formatting. Do you think that makes sense?

@rynowak @SteveSandersonMS I think that could be confusing. How would I format the content of a textarea? My vote goes to bind and bind-format.

Actually, I didn't mind the @Bind notation. But since you removed the leading @ why keep it at all? Wouldn't bind="CurrentValue" be enough?

@jhu78

I think that could be confusing. How would I format the content of a textarea?

If I'm not mistaken, that would look like <textarea bind="@MyValue" value-format="...">

I think you might be expecting a level of sophistication from this initial feature add that won't be here for some time.

Actually, I didn't mind the @Bind notation. But since you removed the leading @ why keep it at all? Wouldn't bind="CurrentValue" be enough?

This feature is replacing something that was hacked in for prototype with something much more Razor-like, with a much better tooling experience.

Yes, you're correct that bind="CurrentValue" would work as well. I suppose that it's an old habit of mind to be explicit 😆

It turns out that binding to a suffix isn't currently possible. So we'd have to do format-value and prevent format from matching. I'm going to go with that for now and we can revise if it doesn't feel good.

SteveSandersonMS · 2018-03-29T11:32:54Z

src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BindLoweringPass.cs

+            {
+                // This case can be hit for a 'string' attribute. We want to turn it into
+                // an expression.
+                return "\"" + ((IntermediateToken)htmlContentNode.Children.Single()).Content + "\"";


Does this mean you'd always the leading @, e.g., bind="@MyProperty", or would bind="MyProperty" also work?

Oh wait, I see from the tests below that bind="MyProperty" is supported. Nice :)

Yeah, regarding this I think we need a language feature that allows tag helpers to specify whether they want the attribute's context to be HTML or C#. Right now this is controlled by the type, so if it's a string attribute, the @ is always required to use a variable/expression because you're in an HTML context by default. I think we want authors to be able to control this.

SteveSandersonMS · 2018-03-29T11:34:28Z

src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BindTagHelperDescriptorProvider.cs

+            //
+            // We handle a few different cases here:
+            //
+            //  1.  When given an attribute like **anywhere**'bind-value-changed="@FirstName"' we will


Is it really bind-value-changed, or is it bind-value-change? I'm guessing we don't auto-generate the past-tense of event names :)

SteveSandersonMS · 2018-03-29T11:35:40Z

src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BindTagHelperDescriptorProvider.cs

+            {
+                // If we can't find BindMethods, then just bail. We won't be able to compile the
+                // generated code anyway.
+                return;


Is there any legal case where this happens? I'm curious about why we don't throw in these cases for debuggability.

We don't throw because this happens in the compiler.

The case where this would happen in unresolved references. If you don't have a reference to Microsoft.AspNet.Blazor, you'll know :)

SteveSandersonMS · 2018-03-29T11:40:20Z

src/Microsoft.AspNetCore.Blazor.Razor.Extensions/OrphanTagHelperLoweringPass.cs

+    // that happens, the default lowering pass will map the whole element as a tag helper.
+    //
+    // This phase exists to turn these 'orphan' tag helpers back into HTML elements so that
+    // go down the property path for rendering.


property -> proper

SteveSandersonMS · 2018-03-29T11:51:47Z

src/Microsoft.AspNetCore.Blazor/Components/BindMethods.cs

+        {
+            if (!typeof(T).IsEnum)
+            {
+                throw new ArgumentException($"'bind' does not accept values of type {typeof(T).FullName}. To read and write this value type, wrap it in a property of type string with suitable getters and setters.");


That's cool - will most likely avoid some confusion :)

This isn't new ....

Oh, didn't realise! Let's find whoever originally implemented that and give them a medal :)

SteveSandersonMS · 2018-03-29T11:56:45Z

test/Microsoft.AspNetCore.Blazor.Build.Test/BindRazorIntegrationTest.cs

+                frames,
+                frame => AssertFrame.Element(frame, "input", 4, 0),
+                frame => AssertFrame.Attribute(frame, "type", "checkbox", 1),
+                frame => AssertFrame.Attribute(frame, "value", "False", 2),


I know this is probably not a regression from the earlier behavior, but setting the value attribute to False is quite strange. It means that's what gets included in an HTML form post if the checkbox is checked. Not a massive problem because people don't normally do HTML form posts from a SPA, but it is odd and unrelated to our goal.

Do we already have the behavior that binding to a true bool value causes the attribute checked to be added, and not when binding to a false bool? That's what controls the checked state in the UI.

Ah, I see from the E2E tests below that the checked behavior does work (not that I really expected it to be broken). I remember now that the conversion of value to checked is already implemented on the JS side, so we wouldn't actually set a value attribute on the DOM element in this case.

Yeah, I took this as a follow up to do the conditional attributes work. The problem is that the .ToString() on bool doesn't do what we want. Conditional attributes should resolve this case.

See: https://github.com/aspnet/Blazor/issues/373#issuecomment-377439700

Treating this as a follow up.

SteveSandersonMS · 2018-03-29T11:58:31Z

test/Microsoft.AspNetCore.Blazor.Build.Test/BindRazorIntegrationTest.cs

+                frame => AssertFrame.Element(frame, "input", 4, 0),
+                frame => AssertFrame.Attribute(frame, "type", "text", 1),
+                frame => AssertFrame.Attribute(frame, "value", "01/01", 2),
+                frame => AssertFrame.Attribute(frame, "changed", typeof(UIEventHandler), 3),


Would it also be possible to assert that the event handler delegate does actually write updated values back to the original property, and does so respecting the specified format? Or is that covered elsewhere?

Oh, I see that is covered in the integration tests below.

Yeah, I left the existing integration tests in place so that I could ensure that they all still worked with no modifications other than the source changes. I plan to de-dupe them when we remove @Bind

SteveSandersonMS · 2018-03-29T11:59:46Z

test/Microsoft.AspNetCore.Blazor.Build.Test/ComponentRenderingRazorIntegrationTest.cs

@@ -286,49 +287,6 @@ public class MyComponent : BlazorComponent
                frame => AssertFrame.Attribute(frame, "BoolProperty", true, 1));
        }

-        [Fact]
-        public void Render_ChildComponent_WithChildContent()


Out of interest, why was this removed? Does it duplicate another test?

Hmm, that's a good question. I don't remember doing this. Bad merge maybe? I will resurrect it.

SteveSandersonMS · 2018-03-29T12:02:14Z

test/Microsoft.AspNetCore.Blazor.Razor.Extensions.Test/BaseTagHelperDescriptorProviderTest.cs

+
+        protected static Compilation BaseCompilation { get; }
+
+        // For simplicity in testing, exlude the built-in components. We'll add more and we


exlude -> exclude

rynowak · 2018-03-29T17:39:46Z

I take this to mean you will be removing the old @Bind support fairly soon which will mean #386 will then no longer be an issue?

That's the plan 👍

@Bind

This change introduces a 'tag helper' that replaces @Bind with custom code generation that accomplishes roughly the same thing. This feature lights up by dynamically generating tag helpers that are visible to tooling and affect the code generation based on: - pattern recognition of component properties - attributes that create definitions for elements - a 'fallback' case for elements 'bind' also supports format strings (currently only for DateTime) via a separate attribute. This change introduces the basic framework for bind and tooling support. We know that we'll have to do more work to define the set of default 'bind' cases for the DOM and to flesh out the conversion/formatting infrastructure. This change gets us far enough to replace all of the cases we currently have tests for :) with the new features. The old @Bind technique still works for now. Examples: @* bind an input element to an expression *@ <input bind="@SelectedDate" format="mm/dd/yyyy" /> @functions { public DateTime SelectedDate { get; set; } } @* bind an arbitrary expression to an arbitrary set of attributes *@ <div bind-myvalue-myevent="@SomeExpression">...</div> @* write a component that supports bind *@ @* in Counter.cshtml *@ <div>...html omitted for brevity...</div> @functions { public int Value { get; set; } = 1; public Action<int> ValueChanged { get; set; } } @* in another file *@ <Counter bind-Value="@CurrentValue" /> @functions { public int CurrentValue { get; set; } }

rynowak · 2018-03-30T05:04:11Z

Appveyor succeeded on rerun. The first failure was due to network problems.

SteveSandersonMS reviewed Mar 29, 2018

View reviewed changes

rynowak force-pushed the rynowak/bind branch 3 times, most recently from dbb8bf6 to be0d40e Compare March 30, 2018 04:13

rynowak force-pushed the rynowak/bind branch from be0d40e to ab0afc0 Compare March 30, 2018 04:41

rynowak merged commit 4407de1 into dev Mar 30, 2018

rynowak deleted the rynowak/bind branch March 30, 2018 05:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Replace @bind with bind-... #406

Replace @bind with bind-... #406

rynowak commented Mar 27, 2018 •

edited

Loading

tdinucci commented Mar 29, 2018

SteveSandersonMS left a comment

SteveSandersonMS Mar 29, 2018

rynowak Mar 29, 2018

SteveSandersonMS Mar 29, 2018

rynowak Mar 29, 2018

jhu78 Mar 29, 2018

rynowak Mar 30, 2018

rynowak Mar 30, 2018

SteveSandersonMS Mar 29, 2018

SteveSandersonMS Mar 29, 2018

rynowak Mar 29, 2018

SteveSandersonMS Mar 29, 2018

rynowak Mar 29, 2018

SteveSandersonMS Mar 29, 2018

rynowak Mar 29, 2018

SteveSandersonMS Mar 29, 2018

SteveSandersonMS Mar 29, 2018

rynowak Mar 29, 2018

SteveSandersonMS Mar 29, 2018

SteveSandersonMS Mar 29, 2018

SteveSandersonMS Mar 29, 2018

rynowak Mar 29, 2018

rynowak Mar 30, 2018

SteveSandersonMS Mar 29, 2018

SteveSandersonMS Mar 29, 2018

rynowak Mar 30, 2018

SteveSandersonMS Mar 29, 2018

rynowak Mar 30, 2018

SteveSandersonMS Mar 29, 2018

rynowak commented Mar 29, 2018

rynowak commented Mar 30, 2018


		protected static Compilation BaseCompilation { get; }

		// For simplicity in testing, exlude the built-in components. We'll add more and we

Replace @bind with bind-... #406

Replace @bind with bind-... #406

Conversation

rynowak commented Mar 27, 2018 • edited Loading

tdinucci commented Mar 29, 2018

SteveSandersonMS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rynowak commented Mar 29, 2018

rynowak commented Mar 30, 2018

rynowak commented Mar 27, 2018 •

edited

Loading