From 599fbb8c643bfd146b3b6879071a0ad2e27ee357 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Wed, 21 Mar 2018 10:03:19 -0700 Subject: [PATCH 1/2] Force LF for package-lock.json Most versions of NPM in use always use LF for line endings anyway. Forcing LF on windows should limit the number of no-op changes. This issue has been fixed in NPM (see https://github.com/npm/npm/issues/1716) but not everyone will have the fix. --- .gitattributes | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .gitattributes diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 000000000..fa4a904c0 --- /dev/null +++ b/.gitattributes @@ -0,0 +1,2 @@ +# Force LF for package-lock files - not all version of NPM detect line endings. +package-lock.json text eol=lf \ No newline at end of file From 502ea246fba30087fa6ffe70ecc52a07006b7ff7 Mon Sep 17 00:00:00 2001 From: Ryan Nowak Date: Wed, 21 Mar 2018 12:31:36 -0700 Subject: [PATCH 2/2] Fix for #314 - streamline lambda component args This change removes the magic 'auto-lambda' feature that has some unconvincing UX. Also working around a razor bug where explicit expressions are lowered incorrectly. This should make it possible to write code like: --- .../BlazorDesignTimeNodeWriter.cs | 25 ++--- .../BlazorExtensionInitializer.cs | 1 + .../BlazorRuntimeNodeWriter.cs | 35 ++---- .../ComplexAttributeContentPass.cs | 101 ++++++++++++++++++ .../ComponentTagHelperDescriptorProvider.cs | 6 +- ...elperBoundAttributeDescriptorExtensions.cs | 16 +-- .../ComponentRenderingRazorIntegrationTest.cs | 24 +++-- ...nTimeCodeGenerationRazorIntegrationTest.cs | 6 +- ...ntimeCodeGenerationRazorIntegrationTest.cs | 4 +- 9 files changed, 148 insertions(+), 70 deletions(-) create mode 100644 src/Microsoft.AspNetCore.Blazor.Razor.Extensions/ComplexAttributeContentPass.cs diff --git a/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BlazorDesignTimeNodeWriter.cs b/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BlazorDesignTimeNodeWriter.cs index 47187c2ea..f003a1fc8 100644 --- a/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BlazorDesignTimeNodeWriter.cs +++ b/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BlazorDesignTimeNodeWriter.cs @@ -425,30 +425,27 @@ public override void WriteComponentAttribute(CodeRenderingContext context, Compo } else if (node.BoundAttribute?.IsDelegateProperty() ?? false) { - // See the runtime version of this code for a thorough description of what we're doing here + // We always surround the expression with the delegate constructor. This makes type + // inference inside lambdas, and method group conversion do the right thing. + IntermediateToken token = null; if ((cSharpNode = node.Children[0] as CSharpExpressionIntermediateNode) != null) { - // This is an escaped event handler - context.CodeWriter.Write(DesignTimeVariable); - context.CodeWriter.Write(" = "); - context.CodeWriter.Write("new "); - context.CodeWriter.Write(node.BoundAttribute.TypeName); - context.CodeWriter.Write("("); - context.CodeWriter.WriteLine(); - WriteCSharpToken(context, ((IntermediateToken)cSharpNode.Children[0])); - context.CodeWriter.Write(");"); - context.CodeWriter.WriteLine(); + token = cSharpNode.Children[0] as IntermediateToken; } else + { + token = node.Children[0] as IntermediateToken; + } + + if (token != null) { context.CodeWriter.Write(DesignTimeVariable); context.CodeWriter.Write(" = "); context.CodeWriter.Write("new "); context.CodeWriter.Write(node.BoundAttribute.TypeName); context.CodeWriter.Write("("); - context.CodeWriter.Write(node.BoundAttribute.GetDelegateSignature()); - context.CodeWriter.Write(" => "); - WriteCSharpToken(context, ((IntermediateToken)node.Children[0])); + context.CodeWriter.WriteLine(); + WriteCSharpToken(context, token); context.CodeWriter.Write(");"); context.CodeWriter.WriteLine(); } diff --git a/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BlazorExtensionInitializer.cs b/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BlazorExtensionInitializer.cs index a324ce593..982d11121 100644 --- a/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BlazorExtensionInitializer.cs +++ b/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BlazorExtensionInitializer.cs @@ -66,6 +66,7 @@ public static void Register(RazorProjectEngineBuilder builder) builder.Features.Add(new ConfigureBlazorCodeGenerationOptions()); builder.Features.Add(new ComponentDocumentClassifierPass()); + builder.Features.Add(new ComplexAttributeContentPass()); builder.Features.Add(new ComponentLoweringPass()); builder.Features.Add(new ComponentTagHelperDescriptorProvider()); diff --git a/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BlazorRuntimeNodeWriter.cs b/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BlazorRuntimeNodeWriter.cs index b1de0647c..7722b3b46 100644 --- a/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BlazorRuntimeNodeWriter.cs +++ b/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/BlazorRuntimeNodeWriter.cs @@ -447,39 +447,24 @@ public override void WriteComponentAttribute(CodeRenderingContext context, Compo } else if (node.BoundAttribute?.IsDelegateProperty() ?? false) { - // This is a UIEventHandler property. We do some special code generation for this - // case so that it's easier to write for common cases. - // - // Example: - // - // --> builder.AddAttribute(X, "OnClick", new UIEventHandler((e) => Foo())); - // - // The constructor is important because we want to put type inference into a state where - // we know the delegate's type should be UIEventHandler. AddAttribute has an overload that - // accepts object, so without the 'new UIEventHandler' things will get ugly. - // - // The escape for this behavior is to prefix the expression with @. This is similar to - // how escaping works for ModelExpression in MVC. - // Example: - // - // --> builder.AddAttribute(X, "OnClick", new UIEventHandler(Foo)); + // We always surround the expression with the delegate constructor. This makes type + // inference inside lambdas, and method group conversion do the right thing. + IntermediateToken token = null; if ((cSharpNode = node.Children[0] as CSharpExpressionIntermediateNode) != null) { - // This is an escaped event handler; - context.CodeWriter.Write("new "); - context.CodeWriter.Write(node.BoundAttribute.TypeName); - context.CodeWriter.Write("("); - context.CodeWriter.Write(((IntermediateToken)cSharpNode.Children[0]).Content); - context.CodeWriter.Write(")"); + token = cSharpNode.Children[0] as IntermediateToken; } else + { + token = node.Children[0] as IntermediateToken; + } + + if (token != null) { context.CodeWriter.Write("new "); context.CodeWriter.Write(node.BoundAttribute.TypeName); context.CodeWriter.Write("("); - context.CodeWriter.Write(node.BoundAttribute.GetDelegateSignature()); - context.CodeWriter.Write(" => "); - context.CodeWriter.Write(((IntermediateToken)node.Children[0]).Content); + context.CodeWriter.Write(token.Content); context.CodeWriter.Write(")"); } } diff --git a/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/ComplexAttributeContentPass.cs b/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/ComplexAttributeContentPass.cs new file mode 100644 index 000000000..763dddb85 --- /dev/null +++ b/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/ComplexAttributeContentPass.cs @@ -0,0 +1,101 @@ +// 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 Microsoft.AspNetCore.Razor.Language; +using Microsoft.AspNetCore.Razor.Language.Intermediate; + +namespace Microsoft.AspNetCore.Blazor.Razor +{ + // We don't support 'complex' content for components (mixed C# and markup) right now. + // It's not clear yet if Blazor will have a good scenario to use these constructs. + // + // This is where a lot of the complexity in the Razor/TagHelpers model creeps in and we + // might be able to avoid it if these features aren't needed. + internal class ComplexAttributeContentPass : IntermediateNodePassBase, IRazorOptimizationPass + { + // Run before other Blazor passes + public override int Order => -1000; + + protected override void ExecuteCore(RazorCodeDocument codeDocument, DocumentIntermediateNode documentNode) + { + var nodes = documentNode.FindDescendantNodes(); + for (var i = 0; i < nodes.Count; i++) + { + ProcessAttributes(nodes[i]); + } + } + + private void ProcessAttributes(TagHelperIntermediateNode node) + { + for (var i = node.Children.Count - 1; i >= 0; i--) + { + if (node.Children[i] is TagHelperPropertyIntermediateNode propertyNode) + { + if (HasComplexChildContent(propertyNode)) + { + node.Diagnostics.Add(BlazorDiagnosticFactory.Create_UnsupportedComplexContent( + propertyNode, + propertyNode.AttributeName)); + node.Children.RemoveAt(i); + continue; + } + + node.Children[i] = new ComponentAttributeExtensionNode(propertyNode); + } + else if (node.Children[i] is TagHelperHtmlAttributeIntermediateNode htmlNode) + { + if (HasComplexChildContent(htmlNode)) + { + node.Diagnostics.Add(BlazorDiagnosticFactory.Create_UnsupportedComplexContent( + htmlNode, + htmlNode.AttributeName)); + node.Children.RemoveAt(i); + continue; + } + } + } + } + + private static bool HasComplexChildContent(IntermediateNode node) + { + if (node.Children.Count == 1 && + node.Children[0] is HtmlAttributeIntermediateNode htmlNode && + htmlNode.Children.Count > 1) + { + // This case can be hit for a 'string' attribute + return true; + } + else if (node.Children.Count == 1 && + node.Children[0] is CSharpExpressionIntermediateNode cSharpNode && + cSharpNode.Children.Count > 1) + { + // This case can be hit when the attribute has an explicit @ inside, which + // 'escapes' any special sugar we provide for codegen. + // + // There's a special case here for explicit expressions. See https://github.com/aspnet/Razor/issues/2203 + // handling this case as a tactical matter since it's important for lambdas. + if (cSharpNode.Children.Count == 3 && + cSharpNode.Children[0] is IntermediateToken token0 && + cSharpNode.Children[2] is IntermediateToken token2 && + token0.Content == "(" && + token2.Content == ")") + { + cSharpNode.Children.RemoveAt(2); + cSharpNode.Children.RemoveAt(0); + + // We were able to simplify it, all good. + return false; + } + + return true; + } + else if (node.Children.Count > 1) + { + // This is the common case for 'mixed' content + return true; + } + + return false; + } + } +} diff --git a/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/ComponentTagHelperDescriptorProvider.cs b/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/ComponentTagHelperDescriptorProvider.cs index f04736247..076f8762c 100644 --- a/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/ComponentTagHelperDescriptorProvider.cs +++ b/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/ComponentTagHelperDescriptorProvider.cs @@ -114,11 +114,7 @@ private TagHelperDescriptor CreateDescriptor(INamedTypeSymbol type) if (property.kind == PropertyKind.Delegate) { - var propertyType = (INamedTypeSymbol)property.property.Type; - var parameters = propertyType.DelegateInvokeMethod.Parameters; - - var signature = "(" + string.Join(", ", parameters.Select(p => p.Name)) + ")"; - pb.Metadata.Add(DelegateSignatureMetadata, signature); + pb.Metadata.Add(DelegateSignatureMetadata, bool.TrueString); } xml = property.property.GetDocumentationCommentXml(); diff --git a/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/TagHelperBoundAttributeDescriptorExtensions.cs b/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/TagHelperBoundAttributeDescriptorExtensions.cs index e09cfb0f0..fe4dade43 100644 --- a/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/TagHelperBoundAttributeDescriptorExtensions.cs +++ b/src/Microsoft.AspNetCore.Blazor.Razor.Extensions/TagHelperBoundAttributeDescriptorExtensions.cs @@ -16,19 +16,9 @@ public static bool IsDelegateProperty(this BoundAttributeDescriptor attribute) } var key = ComponentTagHelperDescriptorProvider.DelegateSignatureMetadata; - return attribute.Metadata.TryGetValue(key, out var value); - } - - public static string GetDelegateSignature(this BoundAttributeDescriptor attribute) - { - if (attribute == null) - { - throw new ArgumentNullException(nameof(attribute)); - } - - var key = ComponentTagHelperDescriptorProvider.DelegateSignatureMetadata; - attribute.Metadata.TryGetValue(key, out var value); - return value; + return + attribute.Metadata.TryGetValue(key, out var value) && + string.Equals(value, bool.TrueString); } } } diff --git a/test/Microsoft.AspNetCore.Blazor.Build.Test/ComponentRenderingRazorIntegrationTest.cs b/test/Microsoft.AspNetCore.Blazor.Build.Test/ComponentRenderingRazorIntegrationTest.cs index f590d4369..22d44a2b1 100644 --- a/test/Microsoft.AspNetCore.Blazor.Build.Test/ComponentRenderingRazorIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Blazor.Build.Test/ComponentRenderingRazorIntegrationTest.cs @@ -153,8 +153,15 @@ void IComponent.SetParameters(ParameterCollection parameters) } - [Fact] - public void Render_ChildComponent_WithLambdaEventHandler() + [Theory] + [InlineData("e => Increment(e)")] + [InlineData("(e) => Increment(e)")] + [InlineData("@(e => Increment(e))")] + [InlineData("@(e => { Increment(e); })")] + [InlineData("Increment")] + [InlineData("@Increment")] + [InlineData("@(Increment)")] + public void Render_ChildComponent_WithEventHandler(string expression) { // Arrange AdditionalSyntaxTrees.Add(CSharpSyntaxTree.ParseText(@" @@ -171,16 +178,17 @@ public class MyComponent : BlazorComponent } ")); - var component = CompileToComponent(@" + var component = CompileToComponent($@" @addTagHelper *, TestAssembly - +@using Microsoft.AspNetCore.Blazor + -@functions { +@functions {{ private int counter; - private void Increment() { + private void Increment(UIEventArgs e) {{ counter++; - } -}"); + }} +}}"); // Act var frames = GetRenderTree(component); diff --git a/test/Microsoft.AspNetCore.Blazor.Build.Test/DesignTimeCodeGenerationRazorIntegrationTest.cs b/test/Microsoft.AspNetCore.Blazor.Build.Test/DesignTimeCodeGenerationRazorIntegrationTest.cs index 2a3f2cad6..1932146b6 100644 --- a/test/Microsoft.AspNetCore.Blazor.Build.Test/DesignTimeCodeGenerationRazorIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Blazor.Build.Test/DesignTimeCodeGenerationRazorIntegrationTest.cs @@ -268,7 +268,7 @@ public class MyComponent : BlazorComponent // Act var generated = CompileToCSharp(@" @addTagHelper *, TestAssembly - + { Increment(); })""/> @functions { private int counter; @@ -308,9 +308,9 @@ protected override void BuildRenderTree(Microsoft.AspNetCore.Blazor.RenderTree.R { base.BuildRenderTree(builder); - __o = new Microsoft.AspNetCore.Blazor.UIEventHandler((eventArgs) => + __o = new Microsoft.AspNetCore.Blazor.UIEventHandler( #line 2 ""x:\dir\subdir\Test\TestComponent.cshtml"" - Increment() + (e) => { Increment(); } #line default #line hidden diff --git a/test/Microsoft.AspNetCore.Blazor.Build.Test/RuntimeCodeGenerationRazorIntegrationTest.cs b/test/Microsoft.AspNetCore.Blazor.Build.Test/RuntimeCodeGenerationRazorIntegrationTest.cs index 51adae432..db989dac4 100644 --- a/test/Microsoft.AspNetCore.Blazor.Build.Test/RuntimeCodeGenerationRazorIntegrationTest.cs +++ b/test/Microsoft.AspNetCore.Blazor.Build.Test/RuntimeCodeGenerationRazorIntegrationTest.cs @@ -248,7 +248,7 @@ public class MyComponent : BlazorComponent // Act var generated = CompileToCSharp(@" @addTagHelper *, TestAssembly - + { Increment(); })""/> @functions { private int counter; @@ -277,7 +277,7 @@ protected override void BuildRenderTree(Microsoft.AspNetCore.Blazor.RenderTree.R { base.BuildRenderTree(builder); builder.OpenComponent(0); - builder.AddAttribute(1, ""OnClick"", new Microsoft.AspNetCore.Blazor.UIEventHandler((eventArgs) => Increment())); + builder.AddAttribute(1, ""OnClick"", new Microsoft.AspNetCore.Blazor.UIEventHandler(e => { Increment(); })); builder.CloseComponent(); builder.AddContent(2, ""\n\n""); }