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.

Sign up for GitHub

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

Fix issue where property directive compiled without errors on a regular page #1639

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix issue where property directive compiled without errors on a regular page #1639

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 6 additions & 9 deletions src/Framework/Framework/Compilation/Directives/BaseTypeDirectiveCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ protected override IAbstractBaseTypeDirective Resolve(DothtmlDirectiveNode direc
=> TreeBuilder.BuildBaseTypeDirective(directiveNode, ParseDirective(directiveNode, p => p.ReadDirectiveTypeName()), imports);

protected override ITypeDescriptor CreateArtefact(IReadOnlyList<IAbstractBaseTypeDirective> resolvedDirectives) {
var isMarkupControl = IsMarkupControl();
var wrapperType = GetDefaultWrapperType();

var baseControlDirective = resolvedDirectives.SingleOrDefault();
Expand Down Expand Up @@ -65,7 +66,7 @@ protected override ITypeDescriptor CreateArtefact(IReadOnlyList<IAbstractBaseTyp
}
}

if (DirectiveNodesByName.TryGetValue(ParserConstants.PropertyDeclarationDirective, out var propertyDirectives) && propertyDirectives.Any())
if (isMarkupControl && DirectiveNodesByName.TryGetValue(ParserConstants.PropertyDeclarationDirective, out var propertyDirectives) && propertyDirectives.Any())
{
wrapperType = CreateDynamicDeclaringType(wrapperType, propertyDirectives) ?? wrapperType;
}
Expand Down Expand Up @@ -107,19 +108,15 @@ IEnumerable<DothtmlDirectiveNode> propertyDirectives
? new ResolvedTypeDescriptor(createdTypeInfo)
: null;
}

/// <summary>
/// Gets the default type of the wrapper for the view.
/// </summary>
private ITypeDescriptor GetDefaultWrapperType()
{
if (fileName.EndsWith(".dotcontrol", StringComparison.Ordinal))
{
return new ResolvedTypeDescriptor(typeof(DotvvmMarkupControl));
}
=> new ResolvedTypeDescriptor(IsMarkupControl() ? typeof(DotvvmMarkupControl) : typeof(DotvvmView));
Copy link
Member

Choose a reason for hiding this comment

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

Determining the wrapper type based on file extension is fragile. Now everything worked even when the base type was incorrectly inferred as DotvvmView (#1479), so people can use dothtml or html extension for controls. I think it would be better to determine if a page is control based on the registration in dotvvm configuration:

configuration.Markup.Controls.Any(c => fileName.Equals(c.Src, StringComparison.OrdinalIgnoreCase))

However, that would run into problem every time a new control is added in VS Extension, so I'd use an "extension OR configuration" condition in there (this class is overwritten anyway, so if this method is abstract it should be easy, right?)


return new ResolvedTypeDescriptor(typeof(DotvvmView));
}
private bool IsMarkupControl()
=> fileName.EndsWith(".dotcontrol", StringComparison.Ordinal);

private static ModuleBuilder CreateDynamicMarkupControlAssembly()
{
Expand Down
5 changes: 3 additions & 2 deletions src/Framework/Framework/Compilation/Directives/MarkupDirectiveCompilerPipeline.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,15 +53,16 @@ public MarkupPageMetadata Compile(DothtmlRootNode dothtmlRoot, string fileName)
var baseType = baseTypeResult.Artefact;
resolvedDirectives.AddIfAny(baseTypeCompiler.DirectiveName, baseTypeResult.Directives);

var isMarkupControl = !baseType.IsEqualTo(ResolvedTypeDescriptor.Create(typeof(DotvvmView)));
var viewModuleDirectiveCompiler = new ViewModuleDirectiveCompiler(
directivesByName,
treeBuilder,
!baseType.IsEqualTo(ResolvedTypeDescriptor.Create(typeof(DotvvmView))),
isMarkupControl,
resourceRepository);
var viewModuleResult = viewModuleDirectiveCompiler.Compile();
resolvedDirectives.AddIfAny(viewModuleDirectiveCompiler.DirectiveName, viewModuleResult.Directives);

var propertyDirectiveCompiler = new PropertyDeclarationDirectiveCompiler(directivesByName, treeBuilder, baseType, imports);
var propertyDirectiveCompiler = new PropertyDeclarationDirectiveCompiler(directivesByName, treeBuilder, isMarkupControl, baseType, imports);
var propertyResult = propertyDirectiveCompiler.Compile();
resolvedDirectives.AddIfAny(propertyDirectiveCompiler.DirectiveName, propertyResult.Directives);

Expand Down
9 changes: 8 additions & 1 deletion src/Framework/Framework/Compilation/Directives/PropertyDeclarationDirectiveCompiler.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,18 +17,25 @@ public class PropertyDeclarationDirectiveCompiler : DirectiveCompiler<IAbstractP
{
private readonly ITypeDescriptor controlWrapperType;
private readonly ImmutableList<NamespaceImport> imports;
private readonly bool isMarkupControl;

public override string DirectiveName => ParserConstants.PropertyDeclarationDirective;

public PropertyDeclarationDirectiveCompiler(IReadOnlyDictionary<string, IReadOnlyList<DothtmlDirectiveNode>> directiveNodesByName, IAbstractTreeBuilder treeBuilder, ITypeDescriptor controlWrapperType, ImmutableList<NamespaceImport> imports)
public PropertyDeclarationDirectiveCompiler(IReadOnlyDictionary<string, IReadOnlyList<DothtmlDirectiveNode>> directiveNodesByName, IAbstractTreeBuilder treeBuilder, bool isMarkupControl, ITypeDescriptor controlWrapperType, ImmutableList<NamespaceImport> imports)
: base(directiveNodesByName, treeBuilder)
{
this.isMarkupControl = isMarkupControl;
this.controlWrapperType = controlWrapperType;
this.imports = imports;
}

protected override IAbstractPropertyDeclarationDirective Resolve(DothtmlDirectiveNode directiveNode)
{
if (!isMarkupControl)
{
directiveNode.AddError("Can not use the @property directive outside of a markup control");
}

var valueSyntaxRoot = ParseDirective(directiveNode, p => p.ReadPropertyDirectiveValue());

var declaration = valueSyntaxRoot as PropertyDeclarationBindingParserNode;
Expand Down