Skip to content

Add Blazor partial class support in Visual Studio. #1182

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

Merged
merged 1 commit into from
Oct 1, 2019

Conversation

NTaylorMullen
Copy link

@NTaylorMullen NTaylorMullen commented Sep 27, 2019

  • No longer mark declaration files as single file generators. Prior to this we relied on SingleFileGenerators to dynamically update the declaration files when .razor files changed. However, to make partial classes work we can no longer depend on declaration files being available because their existence causes us to have to mangle class names for opened documents; otherwise you get two files with same name and result in ambiguous definition errors.
  • Stopped including declaration files as part of the users compilation. This was intended to make the design time experience operate more similar to how Blazor apps function at runtime (directly access each component instead of their declarations). We now rely on the background code generation effort built from the find all references work to supply users with strongly typed component names.
  • Stop mangling class names for Visual Studio. Razor.VSCode has its own set of configurations which i'm not addressing as part of this changeset.
  • Start generating components with the partial modifier to their class name to enable partial class support.
  • Updated existing tests to expect partial modifier.

dotnet/aspnetcore#5487

- No longer mark declaration files as single file generators. Prior to this we relied on SingleFileGenerators to dynamically update the declaration files when .razor files changed. However, to make partial classes work we can no longer depend on declaration files being available because their existence causes us to have to mangle class names for opened documents; otherwise you get two files with same name and result in ambiguous definition errors.
- Stopped including declaration files as part of the users compilation. This was intended to make the design time experience operate more similar to how Blazor apps function at runtime (directly access each component instead of their declarations). We now rely on the background code generation effort built from the find all references work to supply users with strongly typed component names.
- Stop mangling class names for Visual Studio. Razor.VSCode has its own set of configurations which i'm not addressing as part of this changeset.
- Start generating components with the partial modifier to their class name to enable partial class support.
- Updated existing tests to expect partial modifier.

dotnet/aspnetcore#5487
@rynowak
Copy link
Member

rynowak commented Sep 27, 2019

What's the user experience of this like re: switching context between files. Today when you, save, give focus to, or remove focus from a component file, it results in a chain of events that cause us to update component descriptors. This is critical for productivity - that you can add a [Parameter] in a component, and then immediate use it elsewhere.

@NTaylorMullen
Copy link
Author

What's the user experience of this like

It's exactly the same as today with 1-2 known bugs that I plan to fix later:

  1. If you add a [Parameter] to a partial class and then switch to an already opened component file you will not get updated TagHelpers until you start typing in the file.
  2. There's a theoretical bug that I haven't seen rear its head but seems possible to me where if you open a Blazor project and the project system is quick enough to initialize then you may not get component bindings for already-open files. This is because it's possible that we don't do more than 1 parse for all the files in the project meaning we wouldn't get component descriptors. In practice this is incredibly rare / racey since our stuff seems to always initializes first. Again, super theoretical but something I want to verify is or is not an issue when i'm back from vaca.

@rynowak
Copy link
Member

rynowak commented Sep 27, 2019

I'm worried about all of these things.

@NTaylorMullen
Copy link
Author

I'm worried about all of these things.

I want and plan to fix them as well once i'm back from vaca.

@rynowak
Copy link
Member

rynowak commented Sep 27, 2019

What are the fixes with these things? I'm nervous about steaming ahead without knowing what the roadmap is.

@NTaylorMullen
Copy link
Author

What are the fixes with these things? I'm nervous about steaming ahead without knowing what the roadmap is.

For the first case Roslyn has already evaluated the document so we can look to see if the file is a component, if so re-trigger TagHelper completion.

For the second case we re-discover TagHelpers after a component has been parsed.

I haven't fully thought through each of the solutions above but there's a lot of ways we can fix either one of the probs

@rynowak
Copy link
Member

rynowak commented Sep 30, 2019

We tested this kind of stuff out in realtime, and it works pretty well 👍

@@ -81,6 +81,7 @@ protected override CodeTarget CreateTarget(RazorCodeDocument codeDocument, Razor
@class.ClassName = computedClass;
@class.Modifiers.Clear();
@class.Modifiers.Add("public");
@class.Modifiers.Add("partial");
Copy link
Member

Choose a reason for hiding this comment

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

Do we need to target this per-version? I guess not because it's not really a breaking change?

Copy link
Member

Choose a reason for hiding this comment

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

@pranavkm - @ajaybhargavb did we build a 3.1 versioning scheme into the SDK yet?

Copy link
Member

Choose a reason for hiding this comment

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

I think it's simpler for now if we just make this change happen to both 3.0 and 3.1 projects.

Copy link
Contributor

Choose a reason for hiding this comment

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

Don't think so. 3.0 is still the latest.

var componentDocumentClassifier = b.Features.OfType<ComponentDocumentClassifierPass>().FirstOrDefault();
if (componentDocumentClassifier != null)
{
componentDocumentClassifier.MangleClassNames = true;
Copy link
Member

Choose a reason for hiding this comment

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

Can we also remove support for MangleClassNames?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah but there are some places it is used like in DefaultTagHelperBinderPhase so it will take some time to make sure removing it doesn't break that code. I'm going to address this as part of a new PR targeting preview3.

@ajaybhargavb
Copy link
Contributor

Merging for preview2. Will address feedback in a new PR for preview3.

@ajaybhargavb ajaybhargavb merged commit ecb3b1b into release/3.1 Oct 1, 2019
@ghost ghost deleted the nimullen/5487 branch October 1, 2019 02:40
ajaybhargavb pushed a commit that referenced this pull request Oct 1, 2019
- No longer mark declaration files as single file generators. Prior to this we relied on SingleFileGenerators to dynamically update the declaration files when .razor files changed. However, to make partial classes work we can no longer depend on declaration files being available because their existence causes us to have to mangle class names for opened documents; otherwise you get two files with same name and result in ambiguous definition errors.
- Stopped including declaration files as part of the users compilation. This was intended to make the design time experience operate more similar to how Blazor apps function at runtime (directly access each component instead of their declarations). We now rely on the background code generation effort built from the find all references work to supply users with strongly typed component names.
- Stop mangling class names for Visual Studio. Razor.VSCode has its own set of configurations which i'm not addressing as part of this changeset.
- Start generating components with the partial modifier to their class name to enable partial class support.
- Updated existing tests to expect partial modifier.

dotnet/aspnetcore#5487
For now we need to treat component files as if they have a single file generator. This will allow us
to trigger a workspace update for the declaration files when they change.
-->
<ItemGroup>
Copy link
Author

Choose a reason for hiding this comment

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

Shoot we need to make this dependent on > 3.0 projects. Otherwise if a user downloads latest VS it will not properly work with older projects.

JunTaoLuo pushed a commit to dotnet/aspnetcore that referenced this pull request May 17, 2020
- No longer mark declaration files as single file generators. Prior to this we relied on SingleFileGenerators to dynamically update the declaration files when .razor files changed. However, to make partial classes work we can no longer depend on declaration files being available because their existence causes us to have to mangle class names for opened documents; otherwise you get two files with same name and result in ambiguous definition errors.
- Stopped including declaration files as part of the users compilation. This was intended to make the design time experience operate more similar to how Blazor apps function at runtime (directly access each component instead of their declarations). We now rely on the background code generation effort built from the find all references work to supply users with strongly typed component names.
- Stop mangling class names for Visual Studio. Razor.VSCode has its own set of configurations which i'm not addressing as part of this changeset.
- Start generating components with the partial modifier to their class name to enable partial class support.
- Updated existing tests to expect partial modifier.

aspnet/AspNetCoredotnet/aspnetcore-tooling#5487\n\nCommit migrated from dotnet/razor@ecb3b1b
JunTaoLuo pushed a commit to dotnet/aspnetcore that referenced this pull request May 17, 2020
- No longer mark declaration files as single file generators. Prior to this we relied on SingleFileGenerators to dynamically update the declaration files when .razor files changed. However, to make partial classes work we can no longer depend on declaration files being available because their existence causes us to have to mangle class names for opened documents; otherwise you get two files with same name and result in ambiguous definition errors.
- Stopped including declaration files as part of the users compilation. This was intended to make the design time experience operate more similar to how Blazor apps function at runtime (directly access each component instead of their declarations). We now rely on the background code generation effort built from the find all references work to supply users with strongly typed component names.
- Stop mangling class names for Visual Studio. Razor.VSCode has its own set of configurations which i'm not addressing as part of this changeset.
- Start generating components with the partial modifier to their class name to enable partial class support.
- Updated existing tests to expect partial modifier.

aspnet/AspNetCoredotnet/aspnetcore-tooling#5487\n\nCommit migrated from dotnet/razor@73858cd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants