This repository has been archived by the owner on Feb 25, 2021. It is now read-only.
-
Notifications
You must be signed in to change notification settings - Fork 649
Remove old workaround @onclick and @bind #525
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -5,14 +5,12 @@ | |
using System.Collections.Generic; | ||
using System.Linq; | ||
using System.Text; | ||
using System.Text.RegularExpressions; | ||
using AngleSharp; | ||
using AngleSharp.Html; | ||
using AngleSharp.Parser.Html; | ||
using Microsoft.AspNetCore.Razor.Language; | ||
using Microsoft.AspNetCore.Razor.Language.CodeGeneration; | ||
using Microsoft.AspNetCore.Razor.Language.Intermediate; | ||
using Microsoft.CodeAnalysis.CSharp; | ||
|
||
namespace Microsoft.AspNetCore.Blazor.Razor | ||
{ | ||
|
@@ -27,27 +25,18 @@ private readonly static HashSet<string> htmlVoidElementsLookup | |
= new HashSet<string>( | ||
new[] { "area", "base", "br", "col", "embed", "hr", "img", "input", "link", "meta", "param", "source", "track", "wbr" }, | ||
StringComparer.OrdinalIgnoreCase); | ||
private readonly static Regex bindExpressionRegex = new Regex(@"^bind\((.+)\)$"); | ||
private readonly static CSharpParseOptions bindArgsParseOptions | ||
= CSharpParseOptions.Default.WithKind(CodeAnalysis.SourceCodeKind.Script); | ||
|
||
private readonly ScopeStack _scopeStack = new ScopeStack(); | ||
private string _unconsumedHtml; | ||
private List<IntermediateToken> _currentAttributeValues; | ||
private IDictionary<string, PendingAttribute> _currentElementAttributes = new Dictionary<string, PendingAttribute>(); | ||
private IList<PendingAttributeToken> _currentElementAttributeTokens = new List<PendingAttributeToken>(); | ||
private int _sourceSequence = 0; | ||
|
||
private struct PendingAttribute | ||
{ | ||
public List<IntermediateToken> Values { get; set; } | ||
} | ||
|
||
private struct PendingAttributeToken | ||
{ | ||
public IntermediateToken AttributeValue; | ||
} | ||
|
||
public override void WriteCSharpCode(CodeRenderingContext context, CSharpCodeIntermediateNode node) | ||
{ | ||
if (context == null) | ||
|
@@ -118,27 +107,26 @@ public override void WriteCSharpCodeAttributeValue(CodeRenderingContext context, | |
throw new InvalidOperationException($"Invoked {nameof(WriteCSharpCodeAttributeValue)} while {nameof(_currentAttributeValues)} was null."); | ||
} | ||
|
||
// For attributes like "onsomeevent=@{ /* some C# code */ }", we treat it as if you | ||
// wrote "onsomeevent=@(_ => { /* some C# code */ })" because then it works as an | ||
// event handler and is a reasonable syntax for that. | ||
var innerCSharp = (IntermediateToken)node.Children.Single(); | ||
innerCSharp.Content = $"_ => {{ {innerCSharp.Content} }}"; | ||
_currentAttributeValues.Add(innerCSharp); | ||
// We used to support syntaxes like <elem onsomeevent=@{ /* some C# code */ } /> but this is no longer the | ||
// case. | ||
// | ||
// 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; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
} | ||
|
||
public override void WriteCSharpExpression(CodeRenderingContext context, CSharpExpressionIntermediateNode node) | ||
{ | ||
// To support syntax like <elem @completeAttributePair /> (which in turn supports syntax | ||
// like <elem @OnSomeEvent(Handler) />), check whether we are currently in the middle of | ||
// writing an element. If so, treat this C# expression as something that should evaluate | ||
// as a RenderTreeFrame of type Attribute. | ||
// We used to support syntaxes like <elem @completeAttributePair /> but this is no longer the case. | ||
// The APIs that a user would need to do this correctly aren't accessible outside of Blazor's core | ||
// anyway. | ||
// | ||
// We provide an error for this case just to be friendly. | ||
if (_unconsumedHtml != null) | ||
{ | ||
var token = (IntermediateToken)node.Children.Single(); | ||
_currentElementAttributeTokens.Add(new PendingAttributeToken | ||
{ | ||
AttributeValue = token | ||
}); | ||
var content = string.Join("", node.Children.OfType<IntermediateToken>().Select(t => t.Content)); | ||
context.Diagnostics.Add(BlazorDiagnosticFactory.Create_ExpressionInAttributeList(node.Source, content)); | ||
return; | ||
} | ||
|
||
|
@@ -279,15 +267,6 @@ public override void WriteHtmlContent(CodeRenderingContext context, HtmlContentI | |
_currentElementAttributes.Clear(); | ||
} | ||
|
||
if (_currentElementAttributeTokens.Count > 0) | ||
{ | ||
foreach (var token in _currentElementAttributeTokens) | ||
{ | ||
WriteElementAttributeToken(context, nextTag, token); | ||
} | ||
_currentElementAttributeTokens.Clear(); | ||
} | ||
|
||
_scopeStack.OpenScope( tagName: nextTag.Data, isComponent: false); | ||
} | ||
|
||
|
@@ -325,56 +304,6 @@ public override void WriteHtmlContent(CodeRenderingContext context, HtmlContentI | |
} | ||
} | ||
|
||
private void WriteElementAttributeToken(CodeRenderingContext context, HtmlTagToken tag, PendingAttributeToken token) | ||
{ | ||
var bindMatch = bindExpressionRegex.Match(token.AttributeValue.Content); | ||
if (bindMatch.Success) | ||
{ | ||
// TODO: Consider alternatives to the @bind syntax. The following is very strange. | ||
|
||
// The @bind(X, Y, Z, ...) syntax is special. We convert it to a pair of attributes: | ||
// [1] [email protected](X, Y, Z, ...) | ||
var valueParams = bindMatch.Groups[1].Value; | ||
WriteAttribute(context.CodeWriter, "value", new[] | ||
{ | ||
new IntermediateToken | ||
{ | ||
Kind = TokenKind.CSharp, | ||
Content = $"{BlazorApi.BindMethods.GetValue}({valueParams})" | ||
} | ||
}); | ||
|
||
// [2] @onchange(BindSetValue(parsed => { X = parsed; }, X, Y, Z, ...)) | ||
var parsedArgs = CSharpSyntaxTree.ParseText(valueParams, bindArgsParseOptions); | ||
var parsedArgsSplit = parsedArgs.GetRoot().ChildNodes().Select(x => x.ToString()).ToList(); | ||
if (parsedArgsSplit.Count > 0) | ||
{ | ||
parsedArgsSplit.Insert(0, $"_parsedValue_ => {{ {parsedArgsSplit[0]} = _parsedValue_; }}"); | ||
} | ||
var parsedArgsJoined = string.Join(", ", parsedArgsSplit); | ||
var onChangeAttributeToken = new PendingAttributeToken | ||
{ | ||
AttributeValue = new IntermediateToken | ||
{ | ||
Kind = TokenKind.CSharp, | ||
Content = $"onchange({BlazorApi.BindMethods.SetValue}({parsedArgsJoined}))" | ||
} | ||
}; | ||
WriteElementAttributeToken(context, tag, onChangeAttributeToken); | ||
} | ||
else | ||
{ | ||
// For any other attribute token (e.g., @onclick(...)), treat it as an expression | ||
// that will evaluate as an attribute frame | ||
context.CodeWriter | ||
.WriteStartMethodInvocation($"{_scopeStack.BuilderVarName}.{nameof(BlazorApi.RenderTreeBuilder.AddAttribute)}") | ||
.Write((_sourceSequence++).ToString()) | ||
.WriteParameterSeparator() | ||
.Write(token.AttributeValue.Content) | ||
.WriteEndMethodInvocation(); | ||
} | ||
} | ||
|
||
public override void WriteUsingDirective(CodeRenderingContext context, UsingDirectiveIntermediateNode node) | ||
{ | ||
context.CodeWriter.WriteUsing(node.Content, endLine: true); | ||
|
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -178,50 +178,5 @@ void IHandleEvent.HandleEvent(UIEventHandler handler, UIEventArgs args) | |
// at the end of every event callback. | ||
StateHasChanged(); | ||
} | ||
|
||
// At present, if you have a .cshtml file in a project with <Project Sdk="Microsoft.NET.Sdk.Web">, | ||
// Visual Studio will run design-time builds for it, codegenning a class that attempts to override | ||
// this method. Therefore the virtual method must be defined, even though it won't be used at runtime, | ||
// because otherwise VS will display a design-time error in its 'Error List' pane. | ||
// TODO: Track down what triggers the design-time build for .cshtml files and how to stop it, then | ||
// this method can be removed. | ||
/// <summary> | ||
/// Not used. Do not invoke this method. | ||
/// </summary> | ||
/// <returns>Always throws an exception.</returns> | ||
public virtual Task ExecuteAsync() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was missed from my cleanup a while back |
||
=> throw new NotImplementedException($"Blazor components do not implement {nameof(ExecuteAsync)}."); | ||
|
||
/// <summary> | ||
/// Applies two-way data binding between the element and the property. | ||
/// </summary> | ||
/// <param name="value">The model property to be bound to the element.</param> | ||
protected RenderTreeFrame bind(object value) | ||
=> throw new NotImplementedException($"{nameof(bind)} is a compile-time symbol only and should not be invoked."); | ||
|
||
/// <summary> | ||
/// Applies two-way data binding between the element and the property. | ||
/// </summary> | ||
/// <param name="value">The model property to be bound to the element.</param> | ||
protected RenderTreeFrame bind(DateTime value, string format) | ||
=> throw new NotImplementedException($"{nameof(bind)} is a compile-time symbol only and should not be invoked."); | ||
|
||
/// <summary> | ||
/// Handles click events by invoking <paramref name="handler"/>. | ||
/// </summary> | ||
/// <param name="handler">The handler to be invoked when the event occurs.</param> | ||
/// <returns>A <see cref="RenderTreeFrame"/> that represents the event handler.</returns> | ||
protected RenderTreeFrame onclick(Action handler) | ||
// Note that the 'sequence' value is updated later when inserted into the tree | ||
=> RenderTreeFrame.Attribute(0, "onclick", handler != null ? (_ => handler()) : (UIEventHandler)null); | ||
|
||
/// <summary> | ||
/// Handles change events by invoking <paramref name="handler"/>. | ||
/// </summary> | ||
/// <param name="handler">The handler to be invoked when the event occurs. The handler will receive the new value as a parameter.</param> | ||
/// <returns>A <see cref="RenderTreeFrame"/> that represents the event handler.</returns> | ||
protected RenderTreeFrame onchange(Action<object> handler) | ||
// Note that the 'sequence' value is updated later when inserted into the tree | ||
=> RenderTreeFrame.Attribute(0, "onchange", args => handler(((UIChangeEventArgs)args).Value)); | ||
} | ||
} |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -47,5 +47,45 @@ public void RejectsEndTagWithDifferentNameToStartTag() | |
Assert.Equal(20, item.Span.CharacterIndex); | ||
}); | ||
} | ||
|
||
// This is the old syntax used by @bind and @onclick, it's explicitly unsupported | ||
// and has its own diagnostic. | ||
[Fact] | ||
public void OldEventHandlerSyntax_ReportsError() | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Moved these tests from the rendering tests since they are now error cases. |
||
{ | ||
// Arrange/Act | ||
var generated = CompileToCSharp(@" | ||
<elem @foo(MyHandler) /> | ||
@functions { | ||
void MyHandler() | ||
{ | ||
} | ||
string foo(Action action) | ||
{ | ||
return action.ToString(); | ||
} | ||
}"); | ||
|
||
// Assert | ||
var diagnostic = Assert.Single(generated.Diagnostics); | ||
Assert.Equal("BL9980", diagnostic.Id); | ||
} | ||
|
||
// This used to be a sugar syntax for lambdas, but we don't support that anymore | ||
[Fact] | ||
public void OldCodeBlockAttributeSyntax_ReportsError() | ||
{ | ||
// Arrange/Act | ||
var generated = CompileToCSharp(@" | ||
<elem attr=@{ DidInvokeCode = true; } /> | ||
@functions { | ||
public bool DidInvokeCode { get; set; } = false; | ||
}"); | ||
|
||
// Assert | ||
var diagnostic = Assert.Single(generated.Diagnostics); | ||
Assert.Equal("BL9979", diagnostic.Id); | ||
} | ||
} | ||
} |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Sounds good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.