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

Flow RootNamespace from MsBuild #243

Merged
merged 1 commit into from
Feb 26, 2019

Conversation

ajaybhargavb
Copy link
Contributor

@ajaybhargavb ajaybhargavb commented Feb 23, 2019

Fixes dotnet/aspnetcore#5106

  • Modified Razor sdk targets to include RootNamespace for command line scenarios.
  • Flow RootNamespace through RazorCodeGenerationOptions
  • Modified DefaultRazorProjectHost to bring in RootNamespace from MsBuild for design time scenarios.
  • Flow RootNamespace through HostProject and ProjectSnapshot
  • Updated related infrastructure as well as VSMac and liveshare specific types
  • Updated tests (I didn't add any new tests because it looks like it is tested implicitly. Let me know if there are any scenarios I can add tests for.)

@@ -125,7 +125,7 @@ internal async Task OnProjectChanged(IProjectVersionedValue<IProjectSubscription
}

var configuration = FallbackRazorConfiguration.SelectConfiguration(version);
var hostProject = new HostProject(CommonServices.UnconfiguredProject.FullPath, configuration);
var hostProject = new HostProject(CommonServices.UnconfiguredProject.FullPath, configuration, rootNamespace: null);
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'm not setting RootNamespace here. I assumed we don't care about RootNamespace in unconfigured project case.

Copy link
Member

Choose a reason for hiding this comment

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

What's the user-visible behavior of that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The design time code gen will be generated with namespace __GeneratedComponent instead of the RootNamespace. So intellisense for all components will be available regardless of the folder structure. What I am not sure of is in what cases FallbackRazorConfiguration be used for Component apps?

Copy link
Contributor

Choose a reason for hiding this comment

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

Only time this will ever impact components is if something is severely misconfigured in a users app and we can't determine what type of project it is. IMO not something we should worry about because this is a severe error case.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I think this is OK.

@@ -464,6 +464,7 @@ internal void OnDocumentStructureChanged(object state)

private void ConfigureProjectEngine(RazorProjectEngineBuilder builder)
{
builder.SetRootNamespace(_documentTracker.ProjectSnapshot.RootNamespace);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's possible the snapshot is null depending on how quick everything instantiates. You'll need to handle the situation when you can't resolve the root namespace here. For instance, in the TagHelper case below the document tracker just returns null where as this will null ref

Copy link
Contributor

Choose a reason for hiding this comment

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

Also note, if the snapshot is null, when it populates everything will be reconfigured so there'd only be a slight lapse in a user having an unconfigured root namespace.

@@ -125,7 +125,7 @@ internal async Task OnProjectChanged(IProjectVersionedValue<IProjectSubscription
}

var configuration = FallbackRazorConfiguration.SelectConfiguration(version);
var hostProject = new HostProject(CommonServices.UnconfiguredProject.FullPath, configuration);
var hostProject = new HostProject(CommonServices.UnconfiguredProject.FullPath, configuration, rootNamespace: null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Only time this will ever impact components is if something is severely misconfigured in a users app and we can't determine what type of project it is. IMO not something we should worry about because this is a severe error case.

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.

This is awesome! Let's hold off on merging this to master because it can't go into preview 4 of ASP.NET Core.

If you want, feel free to set up a long-lived branch where you can do this and other preview5 work and merge there.

@ajaybhargavb ajaybhargavb force-pushed the rynowak/root-namespace branch from 012b2ac to 2fc55b8 Compare February 26, 2019 02:00
@ajaybhargavb ajaybhargavb changed the base branch from master to release/3.0.0-preview5 February 26, 2019 02:01
@ajaybhargavb
Copy link
Contributor Author

I've created release/3.0.0-preview5 branch for all the preview5 work.

- Flow Root namespace through in command line builds

- Flow Root namespace through in design time builds

- Feedback
@ajaybhargavb ajaybhargavb force-pushed the rynowak/root-namespace branch from 2fc55b8 to 5632cb1 Compare February 26, 2019 02:05
@ajaybhargavb ajaybhargavb merged commit 5632cb1 into release/3.0.0-preview5 Feb 26, 2019
@ajaybhargavb ajaybhargavb deleted the rynowak/root-namespace branch February 26, 2019 02:20
@rynowak
Copy link
Member

rynowak commented Feb 26, 2019

uhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhhh

Ok. I hope that doesn't cause trouble when we create the actual one. AZDO doesn't let us delete branches that have releases.

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.

3 participants