Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Directive attributes part 1: Support parameters in bound attributes #597

Merged
merged 8 commits into from
May 21, 2019
Merged

Directive attributes part 1: Support parameters in bound attributes #597

merged 8 commits into from
May 21, 2019

Conversation

ajaybhargavb
Copy link
Contributor

Part of dotnet/aspnetcore#6364

This PR introduces support for things like the following,

 <input type="text" bind="Message" /> 
 <input type="text" bind="Message" bind:event="oninput" /> 
 <input type="text" bind="MyDate" bind:format="asdf" /> 
 <input type="text" bind-custom="Message" bind-custom:event="oninput" bind-custom2="Message" bind-custom2:event="onchanged" />
  • Introduced BoundAttributeParameterDescriptor to represent bind:event etc portion of the bound attribute
  • TagHelperBlockRewriter and IntermediateLoweringPass now respects bound attributes with parameters
  • Updated BindTagHelperDescriptorProvider to bind attributes to event and format parameters
  • Updated TagHelperDescriptorJsonConverter
  • Added diagnostics for cases that are no longer supported
  • Updated code gen tests

ajaybhargavb
ajaybhargavb commented May 15, 2019
View reviewed changes
src/Razor/src/Microsoft.AspNetCore.Razor.Language/BoundAttributeParameterDescriptor.cs Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/Components/ComponentBindLoweringPass.cs Outdated Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/Components/ComponentDiagnosticFactory.cs
new RazorDiagnosticDescriptor(
$"{DiagnosticPrefix}10005",
() => "Specifying event handlers in bind attributes are no longer supported. Specify it using the bind:event=... attribute.",
RazorDiagnosticSeverity.Warning);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've made this a warning but the ErrorList pane doesn't respect it. It still shows up as an error.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One day we'll make that work 😄

NTaylorMullen
NTaylorMullen reviewed May 15, 2019
View reviewed changes
src/Razor/src/Microsoft.AspNetCore.Razor.Language/RazorDiagnosticSeverity.cs
@@ -6,6 +6,7 @@ namespace Microsoft.AspNetCore.Razor.Language
public enum RazorDiagnosticSeverity
{
// Purposely using the same value as Roslyn here.
Warning = 2,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Curious: If you build in VS do you get a warning in the window?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope. Not even an error. Build just succeeds.

NTaylorMullen
NTaylorMullen reviewed May 15, 2019
View reviewed changes
src/Razor/src/Microsoft.AspNetCore.Razor.Language/Components/ComponentBindLoweringPass.cs Outdated Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/Components/ComponentBindLoweringPass.cs Outdated Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/Components/ComponentBindLoweringPass.cs Show resolved Hide resolved
...r/src/Microsoft.AspNetCore.Razor.Language/DefaultBoundAttributeParameterDescriptorBuilder.cs Outdated Show resolved Hide resolved
...r/src/Microsoft.AspNetCore.Razor.Language/DefaultBoundAttributeParameterDescriptorBuilder.cs Outdated Show resolved Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorIntermediateNodeLoweringPhase.cs Outdated Show resolved Hide resolved
@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/directive-attributes branch from b79eb81 to e48c009 Compare May 15, 2019 22:25
@ajaybhargavb
Copy link
Contributor Author

🆙 📅

rynowak
rynowak reviewed May 16, 2019
View reviewed changes
src/Razor/src/Microsoft.AspNetCore.Razor.Language/Components/ComponentBindLoweringPass.cs
@@ -249,6 +252,17 @@ private IntermediateNode[] RewriteUsage(IntermediateNode parent, BindEntry bindE
format = GetAttributeContent(bindEntry.BindFormatNode);
}

if (TryGetFormatNode(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we separate these into a separate pass to make it easy to delete in the future? I'm a little worried to see munging on the happy path, but I could be convinced otherwise.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically this pass removes all the extra nodes like these and rewrites to ComponentAttributes. While this case is easy to do in a separate pass, teh diagnostic for bind-value-onchange will be hard to add later on because it would already be rewritten.
Also, when we want to delete these diagnostics, we also want to cleanup BindTagHelperDescriptorProvider to remove the bound attribute descriptors for format- attributes. So a separate pass here won't contain all the temporary code.

rynowak
rynowak reviewed May 16, 2019
View reviewed changes
src/Razor/src/Microsoft.AspNetCore.Razor.Language/Components/ComponentDiagnosticFactory.cs
@@ -351,7 +351,7 @@ public static RazorDiagnostic CreateBindAttributeParameter_MissingBind(SourceSpa
public static readonly RazorDiagnosticDescriptor BindAttribute_UnsupportedFormat =
new RazorDiagnosticDescriptor(
$"{DiagnosticPrefix}10005",
() => "Specifying event handlers in bind attributes are no longer supported. Specify it using the bind:event=... attribute.",
() => "Specifying event handlers in bind attributes are no longer supported. Specify it using the bind:event=... attribute instead.",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/directive-attributes branch from 0c7afeb to 44a19fb Compare May 20, 2019 18:03
@ajaybhargavb
Copy link
Contributor Author

🆙 📅

NTaylorMullen
NTaylorMullen approved these changes May 20, 2019
View reviewed changes
Copy link
Contributor

@NTaylorMullen NTaylorMullen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit: once the case sensitiveness is accounted for.

rynowak
rynowak reviewed May 21, 2019
View reviewed changes
src/Razor/src/Microsoft.CodeAnalysis.Razor/BindTagHelperDescriptorProvider.cs
parameter.Documentation = string.Format(ComponentResources.BindTagHelper_Fallback_Event_Documentation, attributeName);

parameter.SetPropertyName("Event");
});
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

rynowak
rynowak reviewed May 21, 2019
View reviewed changes
src/Razor/test/RazorLanguage.Test/IntegrationTests/ComponentCodeGenerationTestBase.cs
@@ -982,7 +982,7 @@ public void BuiltIn_BindToInputText_WithFormat_WritesAttributes()

// Act
var generated = CompileToCSharp(@"
<input type=""text"" bind=""@CurrentDate"" format-value=""MM/dd/yyyy""/>
<input type=""text"" bind=""@CurrentDate"" bind:format=""MM/dd/yyyy""/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@SteveSandersonMS for deliciousness 😀

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks 🌶

rynowak
rynowak approved these changes May 21, 2019
View reviewed changes
Copy link
Member

@rynowak rynowak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@rynowak
Copy link
Member

rynowak commented May 21, 2019

Did you test in VS?

@ajaybhargavb
Copy link
Contributor Author

Did you test in VS?

Yes

@ajaybhargavb ajaybhargavb force-pushed the ajbaaska/directive-attributes branch from 6007143 to a47665c Compare May 21, 2019 17:56
ajaybhargavb
ajaybhargavb commented May 21, 2019
View reviewed changes
src/Razor/src/Microsoft.CodeAnalysis.Razor.Workspaces/TagHelperResolver.cs
@@ -28,7 +28,7 @@ protected virtual async Task<TagHelperResolutionResult> GetTagHelpersAsync(Proje
throw new ArgumentNullException(nameof(engine));
}

var providers = engine.Engine.Features.OfType<ITagHelperDescriptorProvider>().ToArray();
var providers = engine.Engine.Features.OfType<ITagHelperDescriptorProvider>().OrderBy(f => f.Order).ToArray();
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rynowak, this bug was causing bind to run before component descriptor provider in design time.

@ajaybhargavb ajaybhargavb merged commit 9bbf240 into master May 21, 2019
@ghost ghost deleted the ajbaaska/directive-attributes branch May 21, 2019 20:07
@ajaybhargavb ajaybhargavb mentioned this pull request May 21, 2019
Merged
JunTaoLuo pushed a commit to dotnet/aspnetcore that referenced this pull request May 17, 2020
@ajaybhargavb
Directive attributes part 1: Support parameters in bound attributes (d…
5bad5de 
…otnet/razor#597)

* Directive attributes part 1: Support parameters in bound attributes

* update

* Fix test

* feedback

* Updated design

* Do case sensitive comparison

* more

* Bug fix
\n\nCommit migrated from dotnet/razor@9bbf240
dougbu pushed a commit to dougbu/razor-compiler that referenced this pull request Nov 17, 2021
@ajaybhargavb
Directive attributes part 1: Support parameters in bound attributes (d…
27bd36e 
…otnet/razor#597)

* Directive attributes part 1: Support parameters in bound attributes

* update

* Fix test

* feedback

* Updated design

* Do case sensitive comparison

* more

* Bug fix
\n\nCommit migrated from dotnet/razor@9bbf240

Commit migrated from dotnet/aspnetcore@5bad5de7ee95
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SteveSandersonMS SteveSandersonMS SteveSandersonMS left review comments

@NTaylorMullen NTaylorMullen NTaylorMullen approved these changes

@rynowak rynowak rynowak approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ajaybhargavb @rynowak @SteveSandersonMS @NTaylorMullen