Skip to content

Make View types in app assembly internal #30891

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

Closed
wants to merge 4 commits into from

Conversation

captainsafia
Copy link
Member

@captainsafia captainsafia commented Mar 12, 2021

  • Adds new ConsolidatedMvcViewDocumentClassifierPass that's applied on source generator scenarios
    • Makes view types internal
    • Sets views under ApsNetCoreGeneratedDocument namespace
  • Updates tests and test files

@ghost ghost added the area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates label Mar 12, 2021
@captainsafia captainsafia force-pushed the safia/fix-public-types branch from 130d93b to 03a8b1f Compare March 15, 2021 18:09
@captainsafia captainsafia force-pushed the safia/fix-public-types branch from 03a8b1f to 554c21b Compare March 15, 2021 18:51
@captainsafia captainsafia marked this pull request as ready for review March 16, 2021 04:49
@captainsafia captainsafia requested a review from pranavkm March 16, 2021 04:49
_razorLanguageVersion = razorLanguageVersion;
}

// ConsolidatedMvcViewDocumentClassifier should not apply for design time
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is that?

// intended to be used alongside source generator support in the Razor compiler.
protected override bool IsMatch(RazorCodeDocument codeDocument, DocumentIntermediateNode documentNode)
{
return documentNode.Options.DesignTime != true && _razorLanguageVersion == RazorLanguageVersion.Latest;
Copy link
Contributor

Choose a reason for hiding this comment

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

Right now RazorLangVersion.Latest = 5.0 which would mean this applies to .net5.0 apps. I think you would need to invent a new version. Alternatively, could we add an option in RazorExtensions.Register that causes this type to be used?

Comment on lines +30 to +31


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change

Comment on lines +60 to +61


Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change


@class.BaseType = "global::Microsoft.AspNetCore.Mvc.Razor.RazorPage<TModel>";
@class.Modifiers.Clear();
@class.Modifiers.Add("internal");
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we also toss in sealed?

@@ -7,6 +7,7 @@
<PublishWindowsPdb>true</PublishWindowsPdb>
<!-- Need to build this project in source build -->
<ExcludeFromSourceBuild>false</ExcludeFromSourceBuild>
<NoWarn>RS0016;RS0017</NoWarn>
Copy link
Contributor

Choose a reason for hiding this comment

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

What's this for?

@@ -37,7 +37,9 @@ public static void Register(RazorProjectEngineBuilder builder)
builder.Features.Add(new PagesPropertyInjectionPass());
builder.Features.Add(new ViewComponentTagHelperPass());
builder.Features.Add(new RazorPageDocumentClassifierPass());
builder.Features.Add(new MvcViewDocumentClassifierPass());

builder.Features.Add(new ConsolidatedMvcViewDocumentClassifierPass(builder.Configuration.LanguageVersion));
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we accept an option here? bool generateConsolidatedViews or something like that?

@pranavkm pranavkm requested a review from NTaylorMullen March 16, 2021 05:30
@captainsafia captainsafia deleted the safia/fix-public-types branch June 11, 2021 02:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants