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

JsComponent: improve error reporting and doccomments #1804

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
using System;
using System.Collections.Generic;
using System.Linq;
using DotVVM.Framework.Compilation.Parser.Dothtml.Parser;

Expand Down Expand Up @@ -27,6 +28,19 @@ public static bool HasPropertyValue(this IAbstractControl control, IPropertyDesc
return value;
}

public static Dictionary<string, IAbstractPropertySetter> GetPropertyGroup(this IAbstractControl control, IPropertyGroupDescriptor group)
{
var result = new Dictionary<string, IAbstractPropertySetter>();
foreach (var prop in control.Properties)
{
if (prop.Key is IGroupedPropertyDescriptor member && member.PropertyGroup == group)
{
result.Add(member.GroupMemberName, prop.Value);
}
}
return result;
}

public static IPropertyDescriptor GetHtmlAttributeDescriptor(this IControlResolverMetadata metadata, string name)
=> metadata.GetPropertyGroupMember("", name);
public static IPropertyDescriptor GetPropertyGroupMember(this IControlResolverMetadata metadata, string prefix, string name)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ namespace DotVVM.Framework.Compilation.ControlTree
public interface IAbstractControl : IAbstractContentNode
{
IEnumerable<IPropertyDescriptor> PropertyNames { get; }
IEnumerable<KeyValuePair<IPropertyDescriptor, IAbstractPropertySetter>> Properties { get; }

bool TryGetProperty(IPropertyDescriptor property, [NotNullWhen(true)] out IAbstractPropertySetter? value);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,10 @@ public class ResolvedControl : ResolvedContentNode, IAbstractControl

IEnumerable<IPropertyDescriptor> IAbstractControl.PropertyNames => Properties.Keys;

IEnumerable<KeyValuePair<IPropertyDescriptor, IAbstractPropertySetter>> IAbstractControl.Properties =>
Properties.Select(p => new KeyValuePair<IPropertyDescriptor, IAbstractPropertySetter>(p.Key, p.Value));


public ResolvedControl(ControlResolverMetadata metadata, DothtmlNode? node, DataContextStack dataContext)
: base(metadata, node, dataContext) { }

Expand Down
43 changes: 40 additions & 3 deletions src/Framework/Framework/Controls/JsComponent.cs
Original file line number Diff line number Diff line change
@@ -1,9 +1,14 @@
using System;
using System.Collections.Generic;
using System.Linq;
using DotVVM.Framework.Binding;
using DotVVM.Framework.Binding.Expressions;
using DotVVM.Framework.Binding.Properties;
using DotVVM.Framework.Compilation.ControlTree;
using DotVVM.Framework.Compilation.ControlTree.Resolved;
using DotVVM.Framework.Compilation.Parser.Dothtml.Parser;
using DotVVM.Framework.Compilation.Styles;
using DotVVM.Framework.Compilation.Validation;
using DotVVM.Framework.Hosting;
using DotVVM.Framework.ResourceManagement;
using DotVVM.Framework.Utils;
Expand All @@ -13,7 +18,8 @@ namespace DotVVM.Framework.Controls
/// <summary> Control which initializes a client-side component. </summary>
public class JsComponent : DotvvmControl
{
/// <summary> If set to true, only globally registered JsComponents will be considered for rendering client-side. </summary>
/// <summary> If set to true, view modules are ignored and JsComponents registered using <c>dotvvm.registerGlobalComponent</c> will be considered for client-side rendering. </summary>
[MarkupOptions(AllowBinding = false)]
public bool Global
{
get { return (bool)GetValue(GlobalProperty)!; }
Expand All @@ -23,7 +29,7 @@ public bool Global
DotvvmProperty.Register<bool, JsComponent>(nameof(Global));

/// <summary> Name by which the client-side component was registered. The name is case sensitive. </summary>
[MarkupOptions(Required = true)]
[MarkupOptions(Required = true, AllowBinding = false)]
public string Name
{
get { return (string)GetValue(NameProperty)!; }
Expand All @@ -33,6 +39,7 @@ public string Name
DotvvmProperty.Register<string, JsComponent>(nameof(Name));

/// <summary> The JsComponent must have a wrapper HTML tag, this property configures which tag is used. By default, `div` is used. </summary>
[MarkupOptions(AllowBinding = false)]
public string WrapperTagName
{
get { return (string)GetValue(WrapperTagNameProperty)!; }
Expand Down Expand Up @@ -132,7 +139,8 @@ protected override void RenderContents(IHtmlWriter writer, IDotvvmRequestContext
{ "props", props },
{ "templates", templates },
};
if (GetValue(Internal.ReferencedViewModuleInfoProperty) is ViewModuleReferenceInfo viewModule)
if (GetValue(Internal.ReferencedViewModuleInfoProperty) is ViewModuleReferenceInfo viewModule &&
GetValue(GlobalProperty) is not true)
binding.Add("view", ViewModuleHelpers.GetViewIdJsExpression(viewModule, this));

writer.AddKnockoutDataBind("dotvvm-js-component", binding);
Expand All @@ -154,5 +162,34 @@ public static void AddReferencedViewModuleInfoProperty(ResolvedControl control)
control.ConstructorParameters = null;
}
}

[ControlUsageValidator]
public static IEnumerable<ControlUsageError> ValidateUsage(ResolvedControl control)
{
if (!control.TreeRoot.HasProperty(Internal.ReferencedViewModuleInfoProperty) &&
control.GetProperty(GlobalProperty) is null or ResolvedPropertyValue { Value: false })
{
yield return new ControlUsageError(
$"This view does not have any view modules registred, only global JsComponent will work. Add the `Global` property to this component, to make the intent clear.",
DiagnosticSeverity.Warning,
(control.DothtmlNode as DothtmlElementNode)?.TagNameNode
);
}

var props = control.GetPropertyGroup(PropsGroupDescriptor);
var templates = control.GetPropertyGroup(TemplatesGroupDescriptor);

foreach (var name in props.Keys.Intersect(templates.Keys))
{
var templateElement = templates[name].DothtmlNode;
yield return new ControlUsageError(
$"JsComponent property and template must not share the same name ('{name}').",
DiagnosticSeverity.Error,
props[name].DothtmlNode,
(templateElement as DothtmlElementNode)?.TagNameNode ?? templateElement
);
}
}

}
}
17 changes: 14 additions & 3 deletions src/Framework/Framework/Resources/Scripts/global-declarations.ts
Original file line number Diff line number Diff line change
Expand Up @@ -316,15 +316,26 @@ type DotvvmFileSize = {
}

type DotvvmJsComponent = {
updateProps(p: { [key: string]: any }): void
dispose(): void
/** Called after each update of dotvvm.state which changes any of the bound properties. Only the changed properties are listed in the `updatedProps` argument. */
updateProps(updatedProps: { [key: string]: any }): void
/** Called after the HTMLElement is removed from DOM.
* The component does not need to remove any child elements, but should clean any external data, such as subscription to dotvvm events */
dispose?(): void
}
type DotvvmJsComponentFactory = {
/** Initializes the component on the specified HTMLElement.
* @param element The root HTMLElement of this component
* @param props An object listing all constants and `value` bindings from the `dot:JsComponent` instance
* @param commands An object listing all `command` and `staticCommand` bindings from the `dot:JsComponent` instance
* @param templates An object listing all content properties of the `dot:JsComponent`. The template is identified using its HTML id attribute, it can be rendered using ko.renderTemplate, KnockoutTemplateReactComponent or KnockoutTemplateSvelteComponent
* @param setProps A function which will attempt to write a value back into the bound property. Only certain `value` bindings can be updated, an exception is thown if it isn't possible
* @returns An object which will be notified about subsequent changes to the bound properties and when the component
*/
create(
element: HTMLElement,
props: { [key: string]: any },
commands: { [key: string]: (args: any[]) => Promise<any> },
templates: { [key: string]: string }, // TODO
templates: { [key: string]: string },
setProps: (p: { [key: string]: any }) => void
): DotvvmJsComponent
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -131,12 +131,23 @@ export function findComponent(
}
if (name in globalComponent)
return [null, globalComponent[name]]
throw Error("can not find control " + name)
if (compileConstants.debug) {
if (viewIdOrElement == null) {
throw Error(`Cannot find a global JsComponent ${name}. Make sure it is registred using dotvvm.registerGlobalComponent(${JSON.stringify(name)}, ...)`)
} else {
const moduleNames = [...getModules(viewIdOrElement)].map(m => m.moduleName)
throw Error(`Cannot find a JsComponent ${name} in modules ${moduleNames.join(", ")}, or the global registry. Make sure it is exported as $controls: { ${JSON.stringify(name)}: ... }`)
}
}
else {
throw Error("Cannot find JsComponent " + name)
}
}

/** Registers a component to be used in any JsComponent, independent of view modules */
export function registerGlobalComponent(name: string, c: DotvvmJsComponentFactory) {
if (name in globalComponent)
throw new Error(`Component ${name} is already registered`)
throw new Error(`JsComponent ${name} is already registered`)
globalComponent[name] = c
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,32 @@ public void CompilationWarning_ValidationTargetPrimitiveType()
Assert.AreEqual("BoolProp", warnings[0].tokens.Trim());
}

[TestMethod]
public void CompilationWarning_JsComponentNoModules()
{
var warnings = GetWarnings($$"""
@viewModel {{typeof(TestViewModel)}}
<js:Test />
""");
Assert.AreEqual(1, warnings.Length);
StringAssert.Contains(warnings[0].warning, "This view does not have any view modules registred");
Assert.AreEqual("Test", warnings[0].tokens.Trim());
}

[TestMethod]
public void CompilationWarning_JsComponentFine()
{
XAssert.Empty(GetWarnings($$"""
@viewModel {{typeof(TestViewModel)}}
@js dotvvm.internal
<js:Test />
"""));
XAssert.Empty(GetWarnings($$"""
@viewModel {{typeof(TestViewModel)}}
<js:Test Global />
"""));
}

[DataTestMethod]
[DataRow("TestViewModel2")]
[DataRow("VMArray")]
Expand All @@ -51,7 +77,7 @@ public void CompilationWarning_ValidationTargetPrimitiveType_Negative(string pro
}

[TestMethod]
public void DefaultViewCompiler_NonExistenPropertyWarning()
public void DefaultViewCompiler_NonExistentPropertyWarning()
{
var markup = $@"
@viewModel System.Boolean
Expand All @@ -72,7 +98,7 @@ @viewModel System.Boolean
}

[TestMethod]
public void DefaultViewCompiler_NonExistenPropertyWarning_PrefixedGroup()
public void DefaultViewCompiler_NonExistentPropertyWarning_PrefixedGroup()
{
var markup = $@"
@viewModel System.Boolean
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1129,7 +1129,8 @@
"DotVVM.Framework.Controls.JsComponent": {
"Global": {
"type": "System.Boolean",
"defaultValue": false
"defaultValue": false,
"onlyHardcoded": true
},
"Html:ID": {
"type": "System.String",
Expand All @@ -1142,11 +1143,13 @@
},
"Name": {
"type": "System.String",
"required": true
"required": true,
"onlyHardcoded": true
},
"WrapperTagName": {
"type": "System.String",
"defaultValue": "div"
"defaultValue": "div",
"onlyHardcoded": true
}
},
"DotVVM.Framework.Controls.Label": {
Expand Down