-
Notifications
You must be signed in to change notification settings - Fork 195
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.
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
Support importing components with @using directives #276
Conversation
@@ -8,7 +8,7 @@ namespace Microsoft.AspNetCore.Razor.Language | |||
{ | |||
internal class DefaultRazorParserOptions : RazorParserOptions | |||
{ | |||
public DefaultRazorParserOptions(DirectiveDescriptor[] directives, bool designTime, bool parseLeadingDirectives, RazorLanguageVersion version) | |||
public DefaultRazorParserOptions(DirectiveDescriptor[] directives, bool designTime, bool parseLeadingDirectives, RazorLanguageVersion version, string fileKind) |
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 noticed RazorParserOptionsBuilder
already had a FileKind
property but it wasn't being passed through to RazorParserOptions
. I needed this info to suppress tag helper directive completion for component documents.
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.
😻
At least I had the idea that we should do this.
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.
Talked offline, FileKind
shouldn't be part of the parser options since 1 project engine can parse both Components and Legacy Views. Implications of removing this means that we'll need to change the consumers to rely on the CodeDocument
or other better per-file information to more accurately represent when a parse is operating on a Component document vs. a Legacy document. Parser options are technically configured per-parse but introduce issues when you need to fabricate parser options in a static environment without a file present (we do this in a few places).
src/Razor/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorTagHelperBinderPhase.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorTagHelperBinderPhase.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorTagHelperBinderPhase.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorTagHelperBinderPhase.cs
Show resolved
Hide resolved
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.
It's awesome to see it all come together. Looking shnazzzy!
var typeArgumentCount = node.Component.GetTypeParameters().Count(); | ||
for (var i = 1; i < typeArgumentCount; i++) | ||
{ | ||
context.CodeWriter.Write(","); |
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.
So it generates typeof(abcd<..........>)
and ignores nested generics? Is that expected?
I know Components do something similar at runtime but I believe they use Roslyn to restringify their type name.
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.
There is no need to worry about nested generics. All we need to know is the count of generic type arguments.
Counter => typeof(Counter)
Counter<string, int> => typeof(Counter<,>)
Counter<string, string, int> => typeof(Counter<,,>)
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 see 👍
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.
Would it be crazy to just avoid using qualified names during design time?
So instead of builder.OpenComponent<MyApp.FooBar.Counter>()
just generate builder.OpenComponent<Counter>()
.
One of the things we may need to do eventually is give a line-mapping to one of the statements we generate so that FAR can target it.
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.
So instead of
builder.OpenComponent<MyApp.FooBar.Counter>()
just generatebuilder.OpenComponent<Counter>()
But we don't generate any builder.OpenComponent<>()
during design time (Except for type inference for generic components). Is this a bug?
Here is what gets generated for this input,
@using MvcWithComponents.Components
<Counter></Counter>
<Generic Item="17"></Generic>
Generated design time code:
// <auto-generated/>
#pragma warning disable 1591
namespace MvcWithComponents
{
#line hidden
using TModel = global::System.Object;
using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.AspNetCore.Components;
#line 1 "C:\Users\ajbaaska\Source\Repos\MvcWithComponents\MvcWithComponents\TestComponent.razor"
using MvcWithComponents.Components;
#line default
#line hidden
public class __TestComponent : Microsoft.AspNetCore.Components.ComponentBase
{
#pragma warning disable 219
private void __RazorDirectiveTokenHelpers__() {
}
#pragma warning restore 219
#pragma warning disable 0414
private static System.Object __o = null;
#pragma warning restore 0414
#pragma warning disable 1998
protected override void BuildRenderTree(Microsoft.AspNetCore.Components.RenderTree.RenderTreeBuilder builder)
{
base.BuildRenderTree(builder);
builder.AddAttribute(-1, "ChildContent", (Microsoft.AspNetCore.Components.RenderFragment)((builder2) => {
}
));
__o = typeof(Counter);
__Blazor.MvcWithComponents.__TestComponent.TypeInference.CreateGeneric_0(builder, -1, -1,
#line 5 "C:\Users\ajbaaska\Source\Repos\MvcWithComponents\MvcWithComponents\TestComponent.razor"
17
#line default
#line hidden
);
__o = typeof(Generic<>);
}
#pragma warning restore 1998
}
}
namespace __Blazor.MvcWithComponents.__TestComponent
{
#line hidden
internal static class TypeInference
{
public static void CreateGeneric_0<TItem>(global::Microsoft.AspNetCore.Components.RenderTree.RenderTreeBuilder builder, int seq, int __seq0, TItem __arg0)
{
builder.OpenComponent<global::MvcWithComponents.Components.Generic<TItem>>(seq);
builder.AddAttribute(__seq0, "Item", __arg0);
builder.CloseComponent();
}
}
}
#pragma warning restore 1591
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.
@rynowak ping
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.
But we don't generate any builder.OpenComponent<>() during design time (Except for type inference for generic components). Is this a bug?
It would be a good thing to add I think. It's better than doing something artificial IMO.
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.
It would be a good thing to add I think. It's better than doing something artificial IMO.
I tried this but it gets sort of complicated because we need to do things differently when we do type inference vs when we don't. Generating just a typeof()
in all cases seems cleaner and simpler.
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.
ok
src/Razor/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorTagHelperBinderPhase.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorTagHelperBinderPhase.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorTagHelperBinderPhase.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorTagHelperBinderPhase.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorTagHelperBinderPhase.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorTagHelperBinderPhase.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorTagHelperBinderPhase.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/Legacy/CSharpCodeParser.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.CodeAnalysis.Razor/ComponentTagHelperDescriptorProvider.cs
Show resolved
Hide resolved
🆙 📅 |
As an initial click stop the design LGTM. 👍 |
@NTaylorMullen, could you elaborate a little more? I plan to do whatever I listed in future work separately (It's okay to not support those in the first cut). So, is there anything design-wise you expect will change here? |
Nope. |
src/Razor/src/Microsoft.AspNetCore.Razor.Language/Components/ComponentDiagnosticFactory.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/Components/ComponentDiagnosticFactory.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/Components/ComponentImportProjectFeature.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/Components/ComponentImportProjectFeature.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/Components/TagHelperDescriptorExtensions.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorTagHelperBinderPhase.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorTagHelperBinderPhase.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorTagHelperBinderPhase.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorTagHelperBinderPhase.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/DefaultRazorTagHelperBinderPhase.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/Legacy/AddImportChunkGenerator.cs
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/RazorParserOptions.cs
Outdated
Show resolved
Hide resolved
src/Razor/src/Microsoft.AspNetCore.Razor.Language/RazorParserOptions.cs
Outdated
Show resolved
Hide resolved
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.
cool beans
8e52f19
to
09c5ad5
Compare
src/Razor/src/Microsoft.AspNetCore.Razor.Language/Components/ComponentDiagnosticFactory.cs
Show resolved
Hide resolved
3d96be8
to
0e48a9f
Compare
e7e8e5b
to
59469d5
Compare
🆙 📅 Added a lot more tests. |
return true; | ||
} | ||
|
||
private static string NormalizePath(string path) |
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.
Had to add this. Without this some tests were breaking cross-plat.
4d467d8
to
2b171db
Compare
[InlineData("SomeProject.Foo<Bar.Baz>", true, "SomeProject", "Foo<Bar.Baz>")] | ||
[InlineData("SomeProject.Foo<Bar.Baz>>", true, "", "SomeProject.Foo<Bar.Baz>>")] | ||
[InlineData("SomeProject..Foo<Bar>", true, "SomeProject.", "Foo<Bar>")] | ||
public void TrySplitNamespaceAndType_WorksAsExpected(string fullTypeName, bool expectedResult, string expectedNamespace, string expectedTypeName) |
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.
What about namespaces that contain a .
- would it be worth added more InlineData
to cover these scenarios?
E.g.
[InlineData("SomeProject.SomeNamespace.Foo", true, "SomeProject.SomeNamespace", "Foo")]
[InlineData("SomeProject.SomeNamespace.Foo<Bar>", true, "SomeProject.SomeNamespace", "Foo<Bar>")]
[InlineData("SomeProject.SomeNamespace.Foo<Bar.Baz>", true, "SomeProject.SomeNamespace", "Foo<Bar.Baz>")]
[InlineData("SomeProject.SomeNamespace.Foo<Bar.Baz>>", true, "", "SomeProject.SomeNamespace.Foo<Bar.Baz>>")]
[InlineData("SomeProject.SomeNamespace..Foo<Bar>", true, "SomeProject.SomeNamespace.", "Foo<Bar>")]
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.
@adrianwright109, that probably won't add value as we stop and return whatever is left once we parse the type name.
30a79d8
to
10fde3f
Compare
* Support importing components with @using directives * Suppress taghelper directive completion in component documents * feedback * More feedback * Update tests * Update CodeAnalysis.Razor tests * Flow filekind * Changes * More code gen tests * More tests * fix * Added more tests * Made stuff internal * Filter out temporary tag helper descriptors * update * Do the needful \n\nCommit migrated from dotnet/razor@343f377
* Support importing components with @using directives * Suppress taghelper directive completion in component documents * feedback * More feedback * Update tests * Update CodeAnalysis.Razor tests * Flow filekind * Changes * More code gen tests * More tests * fix * Added more tests * Made stuff internal * Filter out temporary tag helper descriptors * update * Do the needful \n\nCommit migrated from dotnet/razor@343f377 Commit migrated from dotnet/aspnetcore@92d931c229ba
…e-package Temporarily produce Microsoft.AspNetCore.Razor.SourceGenerator.Tooling.Internal package
dotnet/aspnetcore#5577
ComponentTagHelperDescriptorProvider
to allow component binding using short name as well as fully qualified name.TagHelperBindingPhase
to support importing components that are already in scope.Future work: