Ensure model directives are mapped at runtime #11007
Conversation
- Add an extension intermediate note that can track both the base type and the model type - Update all the places we set the base type in MVC to use new node - Update model pass to set the model and its source in the new node - Update tests
chsienki
changed the title
|
@dotnet/razor-compiler for review please :) |
| @@ -39,7 +40,7 @@ protected override void OnDocumentStructureCreated( | |||
| @class.ClassName = CSharpIdentifier.GetClassNameFromPath(filePath); | |||
| } | |||
|
|
|||
| @class.BaseType = IntermediateToken.CreateCSharpToken("global::Microsoft.AspNetCore.Mvc.Razor.RazorPage<TModel>"); | |||
| @class.BaseType = new BaseTypeIntermediateNode("global::Microsoft.AspNetCore.Mvc.Razor.RazorPage<TModel>", location: null); | |||
There was a problem hiding this comment.
Should add a constructor that just takes the component parts rather than doing string manipulation in the main ctor in these cases?
...er/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/Extensions/BaseTypeIntermediateNode.cs
Outdated
Show resolved
Hide resolved
...er/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/Extensions/BaseTypeIntermediateNode.cs
Outdated
Show resolved
Hide resolved
...ft.CodeAnalysis.Razor.Compiler/src/Language/Intermediate/ClassDeclarationIntermediateNode.cs
Outdated
Show resolved
Hide resolved
...piler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/Extensions/InheritsDirectivePass.cs
Outdated
Show resolved
Hide resolved
|
|
||
| public IntermediateToken BaseType { get; set; } | ||
|
|
||
| public IntermediateToken? GreaterThan { get; set; } |
There was a problem hiding this comment.
So if these can be null, can we simply use BaseTypeIntermediateNode everywhere like:
- in MVC with
<TModel>, use BaseTypeIntermediateNode with BaseType, GreaterThan, ModelType, LessThan set; - otherwise, use BaseTypeIntermediateNode with only BaseType set and the others being
null.
And that might hopefully simplify the handling where we now have to conditionally handle BaseTypeIntermediateNode vs IntermediateToken.
I see you suggested something similar:
I don't love the fact that we have to type check the base type now. It's probably possible to have a base 'BaseType' node that MVC then extends, so we get a single type, but its used in a whole bunch of places and it didn't seem worth it.
But I was wondering if we could do that without the inheritance.
Also
but its used in a whole bunch of places and it didn't seem worth it
does that mean we would need to refactor a lot of places if we used BaseTypeIntermediateNode for ClassDeclarationIntermediateNode.BaseType? I guess than I'm fine with not doing that. But aren't we touching all those places now anyway since we went from IntermediateToken to IntermediateNode?
There was a problem hiding this comment.
Yeah, I ended up going that route of just having it be a type for everything. I was worried it would be weird for the things that don't use model, but it actually works quite well I think, and is way simpler.
|
@dotnet/razor-compiler for review please :) |
|
Realized that |
...er/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriterExtensions.cs
Show resolved
Hide resolved
src/Compiler/Microsoft.AspNetCore.Mvc.Razor.Extensions.Version1_X/test/ModelDirectiveTest.cs
Outdated
Show resolved
Hide resolved
...er/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriterExtensions.cs
Outdated
Show resolved
Hide resolved
...er/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/CodeGeneration/CodeWriterExtensions.cs
Outdated
Show resolved
Hide resolved
src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/SourceSpan.cs
Outdated
Show resolved
Hide resolved
…e/CodeGeneration/CodeWriterExtensions.cs Co-authored-by: Fred Silberberg <fred@silberberg.xyz>
* Replace baseType token with a dedicated type * Track the generic portion of the baseType in the new type * Update the model portion of the baseType when the @model directive is used * Add tests + update baselines. --------- Co-authored-by: Fred Silberberg <fred@silberberg.xyz>
* upstream/main: (290 commits) Add breaking changes document (dotnet#11064) Do not extract component into code block (dotnet#11069) Fix invalid setttings json (dotnet#11062) update MicrosoftSourceBuildIntermediatearcadePackageVersion Localized file check-in by OneLocBuild Task: Build definition ID 262: Build ID 2566512 Localized file check-in by OneLocBuild Task: Build definition ID 262: Build ID 2566512 Update source-build team references (dotnet#11032) Handle EditorRequired *Changed/*Expression parameters (dotnet#11043) Localized file check-in by OneLocBuild Task: Build definition ID 262: Build ID 2566213 Localized file check-in by OneLocBuild Task: Build definition ID 262: Build ID 2566213 Avoid ambiguous `object` reference in generic component recovery (dotnet#11053) Move culture info check (dotnet#11057) Update dependencies from https://github.com/dotnet/source-build-reference-packages build 20241015.1 Fix code actions integration tests Add option for format on paste (dotnet#11039) Update src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/Components/ComponentDocumentClassifierPass.cs Fix merge to 17.12 version Update src/Compiler/Microsoft.CodeAnalysis.Razor.Compiler/src/Language/Components/ComponentDocumentClassifierPass.cs Ensure model directives are mapped at runtime (dotnet#11007) Fix @inherits mapping for fuse (dotnet#10985) ...
Maps the type provided via
@modelfor MVC and razor pages.As noted in #10987 this interacts with the
@inheritsdirective, such that it replaces the generic type param provided, iff the type param is calledTModel.We introduce a new intermediate node that also tracks the generic parameter as part of the base type. The model directive then just updates this type rather than string replacing. (Note: we still have some string manipulation in the form of the
TModel, we just now do it as part of the inherits directive, as opposed to the model directive so its better encapsulated).Fixes: #10963