Remove old workaround @onclick and @bind #525

rynowak · 2018-04-10T05:22:11Z

This change removes support for the old syntax used for event handlers
and two-way binding.

See the relevant issues for details on the new features and
improvements:

bind https://github.com/aspnet/Blazor/issues/409
event handlers https://github.com/aspnet/Blazor/issues/503

Along with this change we've removed a few additional things Blazor
could do that aren't part of Razor's usual syntax.

The features that was used to make something like:

<button @onclick(...) />

is an expression that's embedded in a an element's attribute. This
feature might be useful in the future if we want to support 'splatting'
arbitrary attributes into a tag, but the runtime support for this isn't
accessible outside the Blazor core.

The features that implement:

<button onclick=@{ } />

have been removed in favor of a better design for lambdas, method group
conversions and other things for event handler attributes.

use <button onclick=@(x => ...} /> instead.

We think is a better approach in general, because we want the app
developer to write and see the parameter list.

Both syntactic features that have been removed have dedicated error
messages in the compiler. If you're porting old code it should help you
figure out what to do.

rynowak · 2018-04-10T05:24:55Z

samples/StandaloneApp/Pages/Counter.cshtml


 @functions {
    int currentCount = 0;

-    void IncrementCount()
+    void IncrementCount(UIMouseEventArgs e)


I didn't mess with it, because I wanted to get some feedback first, but I had two main thoughts while doing this porting that might improve usability....

We could add a special case for Action (no arg) for the case where you don't need the args, if we think it will be common. This would be pretty easy for us to do, but we would need to push knowledge of this case through a few levels of the stack.

We might want to consider adding @using Microsoft.AspNetCore.Blazor to the default usings. Now that we've got more useful stuff you'll want, we might want to do this as a practical matter. This will help keep docs and articles concise.

We could add a special case for Action (no arg) for the case where you don't need the args

If it's not difficult, I think that would be really good. For click events, possibly the most common of all, you normally don't want to receive any args. It would be good to keep the tutorials simpler and cleaner-looking.

We might want to consider adding @using Microsoft.AspNetCore.Blazor to the default usings

Sounds good to me.

I'm going to go ahead with this PR as is, then I'll follow up on these two items separately. The Action one carries some risk of not making it.

rynowak · 2018-04-10T05:25:42Z

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

-        /// Not used. Do not invoke this method.
-        /// </summary>
-        /// <returns>Always throws an exception.</returns>
-        public virtual Task ExecuteAsync()


This was missed from my cleanup a while back

rynowak · 2018-04-10T05:26:11Z

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

+        // This is the old syntax used by @bind and @onclick, it's explicitly unsupported
+        // and has its own diagnostic.
+        [Fact]
+        public void OldEventHandlerSyntax_ReportsError()


Moved these tests from the rendering tests since they are now error cases.

rynowak · 2018-04-10T05:26:50Z

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

@@ -403,7 +374,7 @@ public void SupportsTwoWayBindingForTextboxes()
            // Assert
            var frames = GetRenderTree(component);
            Assert.Collection(frames,
-                frame => AssertFrame.Element(frame, "input", 3, 0),
+                frame => AssertFrame.Element(frame, "textarea", 3, 0),


The diff is wacky. I created another test for textareas.

I don't think the markup on line 368 is valid. textarea isn't a void element. It should be <textarea bind="MyValue"></textarea>.

Cool, will fix.

rynowak

There were fortunately no big surprises doing this work. We were missing the bind support for textarea, but other than that, everything just lit up.

SteveSandersonMS

Excellent!

SteveSandersonMS · 2018-04-10T15:35:50Z

samples/StandaloneApp/Pages/Counter.cshtml


 @functions {
    int currentCount = 0;

-    void IncrementCount()
+    void IncrementCount(UIMouseEventArgs e)


We could add a special case for Action (no arg) for the case where you don't need the args

If it's not difficult, I think that would be really good. For click events, possibly the most common of all, you normally don't want to receive any args. It would be good to keep the tutorials simpler and cleaner-looking.

We might want to consider adding @using Microsoft.AspNetCore.Blazor to the default usings

Sounds good to me.

SteveSandersonMS · 2018-04-10T16:14:03Z

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

+            // We provide an error for this case just to be friendly.
+            var content = string.Join("", node.Children.OfType<IntermediateToken>().Select(t => t.Content));
+            context.Diagnostics.Add(BlazorDiagnosticFactory.Create_CodeBlockInAttribute(node.Source, content));
+            return;


It's great to have this migration guidance right now, though I also think it would be good to remove it soon after 0.2.0 ships.

SteveSandersonMS · 2018-04-10T16:16:03Z

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

        {
            // Arrange/Act
            var component = CompileToComponent(
-                @"<elem @onclick(MyHandler) />
+                @"<input bind=""MyValue"" />


Previously we were talking about requiring the leading @ for bind for consistency with events. I'm not super worried about this as we can add the requirement later if we choose, but what do you think?

We can always change it later, the problem will be that folk will not get any sort of warning, it would just become HTML

SteveSandersonMS · 2018-04-10T16:17:39Z

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

@@ -403,7 +374,7 @@ public void SupportsTwoWayBindingForTextboxes()
            // Assert
            var frames = GetRenderTree(component);
            Assert.Collection(frames,
-                frame => AssertFrame.Element(frame, "input", 3, 0),
+                frame => AssertFrame.Element(frame, "textarea", 3, 0),


I don't think the markup on line 368 is valid. textarea isn't a void element. It should be <textarea bind="MyValue"></textarea>.

SteveSandersonMS · 2018-04-10T16:18:08Z

test/testapps/BasicTestApp/AddRemoveChildComponents.cshtml

-<button class="addChild" @onclick(AddChild)>Add</button>
-<button class="removeChild" @onclick(RemoveChild)>Remove</button>
+<button class="addChild" onclick="@AddChild">Add</button>
+<button class="removeChild" onclick="@RemoveChild">Remove</button>


Looks so much better :)

SteveSandersonMS · 2018-04-10T16:18:46Z

test/testapps/BasicTestApp/BindCasesComponent.cshtml

@@ -63,7 +64,7 @@
    enum SelectableValue { First, Second, Third, Fourth }
    SelectableValue selectValue = SelectableValue.Second;

-    void AddAndSelectNewSelectOption()
+    void AddAndSelectNewSelectOption(UIMouseEventArgs e)


The more of these I see, the more I think it would be great if we didn't mandate the new argument, at least for mouse events.

SteveSandersonMS · 2018-04-10T16:19:12Z

test/testapps/BasicTestApp/HttpClientTest/HttpRequestsComponent.cshtml

 </p>

 <p>
    <div>Method:</div>
-    <select id="request-method" value=@method @onchange(value => { method = (string)value; })>
+    <select id="request-method" bind="@method">


That is a big improvement!

SteveSandersonMS · 2018-04-10T16:20:35Z

test/testapps/TestContentPackage/ComponentFromPackage.cshtml

 </div>

-@functions
-{
+@functions {


Really minor point, but I'm curious about why we often use this brace style for @functions in Razor, but nowhere else for C# code.

Some earlier versions of Razor tooling produced pretty weird autoformatting when you pasted into a block with the opening brace on the same line, but behaved better if the opening brace was on the line below. Don't know if that's fixed yet.

I think the answer is that @if (foo) { is different than putting the { on a separate line.

@OnClick

This change removes support for the old syntax used for event handlers and two-way binding. See the relevant issues for details on the new features and improvements: bind https://github.com/aspnet/Blazor/issues/409 event handlers https://github.com/aspnet/Blazor/issues/503 Along with this change we've removed a few additional things Blazor could do that aren't part of Razor's usual syntax. ---- The features that was used to make something like: ``` <button @OnClick(...) /> ``` is an expression that's embedded in a an element's attribute. This feature might be useful in the future if we want to support 'splatting' arbitrary attributes into a tag, but the runtime support for this isn't accessible outside the Blazor core. ---- The features that implement: ``` <button onclick=@{ } /> ``` have been removed in favor of a better design for lambdas, method group conversions and other things for event handler attributes. use `<button onclick=@(x => ...} />` instead. We think is a better approach in general, because we want the app developer to write and see the parameter list. ---- Both syntactic features that have been removed have dedicated error messages in the compiler. If you're porting old code it should help you figure out what to do.

rynowak commented Apr 10, 2018

View reviewed changes

SteveSandersonMS approved these changes Apr 10, 2018

View reviewed changes

rynowak force-pushed the rynowak/remove-workarounds branch from 5fd4bbd to fe57c67 Compare April 10, 2018 22:47

rynowak force-pushed the rynowak/remove-workarounds branch from fe57c67 to cda57e7 Compare April 10, 2018 23:39

rynowak merged commit 3fc723a into dev Apr 10, 2018

rynowak deleted the rynowak/remove-workarounds branch April 10, 2018 23:54

SteveSandersonMS added enhancement 3 - Done labels Apr 16, 2018

SteveSandersonMS added this to the 0.2.0 milestone Apr 16, 2018

Psvensso mentioned this pull request May 7, 2018

Update to 0.3.0 torhovland/blazor-realworld-example-app#6

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove old workaround @onclick and @bind #525

Remove old workaround @onclick and @bind #525

rynowak commented Apr 10, 2018

rynowak Apr 10, 2018

SteveSandersonMS Apr 10, 2018

rynowak Apr 10, 2018

rynowak Apr 10, 2018

rynowak Apr 10, 2018

rynowak Apr 10, 2018

SteveSandersonMS Apr 10, 2018

rynowak Apr 10, 2018

rynowak left a comment

SteveSandersonMS left a comment

SteveSandersonMS Apr 10, 2018

SteveSandersonMS Apr 10, 2018

SteveSandersonMS Apr 10, 2018

rynowak Apr 10, 2018

SteveSandersonMS Apr 10, 2018

SteveSandersonMS Apr 10, 2018

SteveSandersonMS Apr 10, 2018

SteveSandersonMS Apr 10, 2018

SteveSandersonMS Apr 10, 2018

rynowak Apr 10, 2018

Remove old workaround @onclick and @bind #525

Remove old workaround @onclick and @bind #525

Conversation

rynowak commented Apr 10, 2018

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 left a comment

Choose a reason for hiding this comment

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