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.

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

Components should be based on usings not on @addTagHelper #5577

Closed
14 of 15 tasks
SteveSandersonMS opened this issue Aug 15, 2018 · 15 comments
Closed
14 of 15 tasks

Components should be based on usings not on @addTagHelper #5577

SteveSandersonMS opened this issue Aug 15, 2018 · 15 comments
Assignees
Labels
area-blazor Includes: Blazor, Razor Components Components Big Rock This issue tracks a big effort which can span multiple issues cost: L Will take from 5 - 10 days to complete Done This issue has been fixed

Comments

@SteveSandersonMS
Copy link
Member

SteveSandersonMS commented Aug 15, 2018

Some rough experience notes:

Component binding will be based on using and C# namespaces instead of directives:

  • Components can bind with the short name when in scope (C# name binding rules)
  • Components can bind with a fully-qualified name
  • Components can bind with an alias (using Foo = Bar)
  • Binding considers using in the same document and imports
  • Binding considers the current namespace of the document
  • Usings that bring in components aren't grey-ed out
  • no support for global:: qualification
  • no support for partially-qualified names (@using MyCompany + <MyLibary.FooBar> => MyCompany.MyLibrary.FooBar
  • Components respect @namespace directive (blocked by Add support for @namespace directive for components #8007)

Directive attributes:

  • directive attributes like bind, onclick, ref are always bound in a component document

Avoiding crosstalk:

  • ITagHelper and VCTH tag helpers are never bound in a component document
  • Components are never bound in a view document (will be supported in the future)
  • addTagHelper/removeTagHelper/tagHelperPrefix are errors in a component document or _Imports.razor
  • directive attributes like bind, onclick, ref are never bound in a view document

Tooling:

  • Completion supports all of the above points

If it's not specified here then we're probably OK with the way it currently works for Blazor 0.7 - please feel free to ask and add more notes

@aspnet-hello aspnet-hello transferred this issue from dotnet/blazor Dec 17, 2018
@aspnet-hello aspnet-hello added this to the Backlog milestone Dec 17, 2018
@aspnet-hello aspnet-hello added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates area-blazor Includes: Blazor, Razor Components labels Dec 17, 2018
@rynowak rynowak changed the title Remove the need for @addTagHelper Components should be based on usings not on @addTagHelper Jan 3, 2019
@rynowak
Copy link
Member

rynowak commented Jan 24, 2019

This can also be removed once we have this feature: https://github.com/aspnet/AspNetCore-Tooling/blob/master/src/Razor/src/Microsoft.AspNetCore.Razor.Language/Components/ComponentImportProjectFeature.cs#L50

Currently we generate an addTagHelper for our best guess at the project's root namespace. I'm doing the work to separately flow the root namespace, but I can't clean up this part as part of that work. Ultimately we don't want to dynamically generate imports at all, it's a hack.

@ajaybhargavb
Copy link
Contributor

ajaybhargavb commented Jan 24, 2019

Ultimately we don't want to dynamically generate imports at all, it's a hack.

We still want to dynamically generate @using statements right?
Especially @using Microsoft.AspNetCore.Components

@rynowak
Copy link
Member

rynowak commented Jan 25, 2019

that's not dynamic though.

My point is that if you're inside MyApp.Shared you don't need a using for MyApp.Shared nor do you for MyApp, those things are already in scope. So everything that works today with components will just work without any dynamic import generation.

@ajaybhargavb
Copy link
Contributor

Current State:
All the components are already implemented as tag helpers and is associated with a TagHelperDescriptor.
DefaultRazorTagHelperBinderPhase filters these TagHelperDescriptors by visiting all the @addTagHelper and @removeTagHelper in the document and imports and binds accordingly.

Proposed implementation:

  • Make DefaultRazorTagHelperBinderPhase visit @using directives. And use TagHelperDescriptor.Kind to differentiate components from regular tag helpers and use the appropriate directive.
  • Make TagHelperBinder ignore tag helper prefix for components
  • The descriptors created in ComponentTagHelperDescriptorProvider will include another rule to match full qualified tag names as well aka both <MyApp.Counter /> and <Counter />
  • No tooling change is necessary. We should still get correct intellisense based on the presence of @usings

Note:

  • We may need to add some errors for cases when components and tag helpers clash but that should probably be a separate issue.
  • When I add a using it is always grayed out with "Using directive is unnecessary". We may want to eventually fix that by modifying the code gen.

Thoughts? Am I missing any requirements that can't be accomplished by this implementation?

@rynowak
Copy link
Member

rynowak commented Jan 25, 2019

What about the special things that components have:

  • bind
  • ref
  • onclick

Also there will be a change in the completion provider. The completion provider interacts with tag helpers.

Also consider aliases using Foo = Bar.Baz
Also consider the ambient namespace of the current document.

This seems like a good start.

@rynowak
Copy link
Member

rynowak commented Jan 25, 2019

/cc @NTaylorMullen

@rynowak
Copy link
Member

rynowak commented Jan 25, 2019

/cc @SteveSandersonMS @danroth27 any other comments or concerns?

@ajaybhargavb
Copy link
Contributor

ajaybhargavb commented Jan 25, 2019

What about the special things that components have:
bind
ref
onclick

They all have catch-all rules which means we don't need to include any special fully qualified name rules. They will continue to work the same way without any changes,

Also there will be a change in the completion provider.

I assume you mean the TagHelperCompletionService and TagHelperFactsService. They still call into the same TagHelperBinder. That should still continue to work as long as we restrict weird tag helper/component clashes (We have some complex AllowedChildren/AllowedParent logic). I'll investigate this more to see what exact cases might be affected. @NTaylorMullen, can you think of anything of the top of your mind?

@NTaylorMullen
Copy link
Contributor

@NTaylorMullen, can you think of anything of the top of your mind?

Given my current context of how components work, nope. As long as the same criteria for associating a Components html element => TagHelperDescriptor is the same or a subset of the existing system, we can ensure the descriptors we create for Components fulfill Components tag targeting requirements. At that point we can the filter which TagHelperDescriptors are available (by @using) for the document before attempting to translate HtmlElementSyntaxNodes into TagHelpers.

Basically, just re-iterating all you said in your comment (I think you're on the right track).

Also there will be a change in the completion provider.

Ya I think I also need some more context on this one, not quite sure what @rynowak is referring to.

@rynowak
Copy link
Member

rynowak commented Jan 26, 2019

https://github.com/aspnet/AspNetCore-Tooling/blob/master/src/Razor/src/Microsoft.VisualStudio.Editor.Razor/DefaultTagHelperFactsService.cs#L111 (get on my level)

I only know about this because I already started to do this work: https://github.com/aspnet/AspNetCore-Tooling/tree/rynowak/lets-blow-it-up


So I think we largely agree on the implementation plan so there's no reason to delay starting on this.

I still think we need some more level of detail on the overall experience. Saying they will continue to work the same way isn't a super high level of precision 😆

@rynowak
Copy link
Member

rynowak commented Jan 26, 2019

I'm going to start filling up the top post with experience notes. Just because I wrote it there shouldnt' be taken as gospel, this is just a starting point.

@rynowak
Copy link
Member

rynowak commented Jan 26, 2019

@danroth27 @SteveSandersonMS please have a look at the top post

@SteveSandersonMS
Copy link
Member Author

My main question is about this:

Components can bind with the short name

Is the idea that you can always bind with the short name, even if you haven't done the necessary @using directives to bring it into scope? If so that seems pretty odd; I'd hope to be able to disambiguate between same-named components using namespaces, like you do with C# classes. Without disambiguation, component libraries are going to be hazardous to combine.

I know it would be a breaking change to only bind to things that are in scope, but it would be a good change IMHO.

@rynowak
Copy link
Member

rynowak commented Jan 26, 2019

Components can bind with the short name

No. The intention is that this is based on the C# name binding rules (usings, current namespace, and using aliases). I think the checklist format is making it unclear.

@ajaybhargavb
Copy link
Contributor

Saying they will continue to work the same way isn't a super high level of precision

Fair enough 😄 . I'll do some more investigation and add more details here.

@mkArtakMSFT mkArtakMSFT modified the milestones: Backlog, 3.0.0-preview5 Feb 6, 2019
@mkArtakMSFT mkArtakMSFT added the cost: L Will take from 5 - 10 days to complete label Feb 6, 2019
@rynowak rynowak added the Components Big Rock This issue tracks a big effort which can span multiple issues label Mar 4, 2019
@rynowak rynowak mentioned this issue Mar 4, 2019
56 tasks
@ajaybhargavb ajaybhargavb added 2 - Working Done This issue has been fixed and removed 1 - Ready labels Mar 25, 2019
@mkArtakMSFT mkArtakMSFT removed area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates labels May 9, 2019
@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-blazor Includes: Blazor, Razor Components Components Big Rock This issue tracks a big effort which can span multiple issues cost: L Will take from 5 - 10 days to complete Done This issue has been fixed
Projects
None yet
Development

No branches or pull requests

7 participants