From 5d6cb56751d0207e444e1ca7eb1862db2aa010c6 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Standa=20Luke=C5=A1?= Date: Fri, 5 Apr 2024 09:50:54 +0200 Subject: [PATCH 1/3] JsComponent: improve error reporting and doccomments partially adresses #1802 --- .../Framework/Controls/JsComponent.cs | 29 ++++++++++++++++-- .../Scripts/viewModules/viewModuleManager.ts | 15 ++++++++-- .../CompilationWarningsTests.cs | 30 +++++++++++++++++-- ...alizationTests.SerializeDefaultConfig.json | 9 ++++-- 4 files changed, 73 insertions(+), 10 deletions(-) diff --git a/src/Framework/Framework/Controls/JsComponent.cs b/src/Framework/Framework/Controls/JsComponent.cs index 215fa50ec9..68840f76a7 100644 --- a/src/Framework/Framework/Controls/JsComponent.cs +++ b/src/Framework/Framework/Controls/JsComponent.cs @@ -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; @@ -13,7 +18,8 @@ namespace DotVVM.Framework.Controls /// Control which initializes a client-side component. public class JsComponent : DotvvmControl { - /// If set to true, only globally registered JsComponents will be considered for rendering client-side. + /// If set to true, view modules are ignored and JsComponents registered using dotvvm.registerGlobalComponent will be considered for client-side rendering. + [MarkupOptions(AllowBinding = false)] public bool Global { get { return (bool)GetValue(GlobalProperty)!; } @@ -23,7 +29,7 @@ public bool Global DotvvmProperty.Register(nameof(Global)); /// Name by which the client-side component was registered. The name is case sensitive. - [MarkupOptions(Required = true)] + [MarkupOptions(Required = true, AllowBinding = false)] public string Name { get { return (string)GetValue(NameProperty)!; } @@ -33,6 +39,7 @@ public string Name DotvvmProperty.Register(nameof(Name)); /// The JsComponent must have a wrapper HTML tag, this property configures which tag is used. By default, `div` is used. + [MarkupOptions(AllowBinding = false)] public string WrapperTagName { get { return (string)GetValue(WrapperTagNameProperty)!; } @@ -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); @@ -154,5 +162,20 @@ public static void AddReferencedViewModuleInfoProperty(ResolvedControl control) control.ConstructorParameters = null; } } + + [ControlUsageValidator] + public static IEnumerable 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 + ); + } + } + } } diff --git a/src/Framework/Framework/Resources/Scripts/viewModules/viewModuleManager.ts b/src/Framework/Framework/Resources/Scripts/viewModules/viewModuleManager.ts index ed5f6bec58..e20cd2ee46 100644 --- a/src/Framework/Framework/Resources/Scripts/viewModules/viewModuleManager.ts +++ b/src/Framework/Framework/Resources/Scripts/viewModules/viewModuleManager.ts @@ -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 } diff --git a/src/Tests/Runtime/ControlTree/DefaultControlTreeResolver/CompilationWarningsTests.cs b/src/Tests/Runtime/ControlTree/DefaultControlTreeResolver/CompilationWarningsTests.cs index 763cd1fbc8..85d7861743 100644 --- a/src/Tests/Runtime/ControlTree/DefaultControlTreeResolver/CompilationWarningsTests.cs +++ b/src/Tests/Runtime/ControlTree/DefaultControlTreeResolver/CompilationWarningsTests.cs @@ -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)}} + + """); + 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 + + """)); + XAssert.Empty(GetWarnings($$""" + @viewModel {{typeof(TestViewModel)}} + + """)); + } + [DataTestMethod] [DataRow("TestViewModel2")] [DataRow("VMArray")] @@ -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 @@ -72,7 +98,7 @@ @viewModel System.Boolean } [TestMethod] - public void DefaultViewCompiler_NonExistenPropertyWarning_PrefixedGroup() + public void DefaultViewCompiler_NonExistentPropertyWarning_PrefixedGroup() { var markup = $@" @viewModel System.Boolean diff --git a/src/Tests/Runtime/config-tests/ConfigurationSerializationTests.SerializeDefaultConfig.json b/src/Tests/Runtime/config-tests/ConfigurationSerializationTests.SerializeDefaultConfig.json index 7b86fe0b8e..fbd5fa8710 100644 --- a/src/Tests/Runtime/config-tests/ConfigurationSerializationTests.SerializeDefaultConfig.json +++ b/src/Tests/Runtime/config-tests/ConfigurationSerializationTests.SerializeDefaultConfig.json @@ -1127,7 +1127,8 @@ "DotVVM.Framework.Controls.JsComponent": { "Global": { "type": "System.Boolean", - "defaultValue": false + "defaultValue": false, + "onlyHardcoded": true }, "Html:ID": { "type": "System.String", @@ -1140,11 +1141,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": { From bc7ac1636d7a710189f0e45ead8fd6ec51cfd193 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Standa=20Luke=C5=A1?= Date: Mon, 15 Apr 2024 16:04:45 +0200 Subject: [PATCH 2/3] JsComponent: error when template and property collides --- .../ControlTree/ControlTreeHelper.cs | 14 ++++++++++++++ .../Compilation/ControlTree/IAbstractControl.cs | 1 + .../ControlTree/Resolved/ResolvedControl.cs | 4 ++++ src/Framework/Framework/Controls/JsComponent.cs | 16 +++++++++++++++- .../Resources/Scripts/global-declarations.ts | 17 ++++++++++++++--- .../Scripts/viewModules/viewModuleManager.ts | 2 +- .../CompilationWarningsTests.cs | 2 +- 7 files changed, 50 insertions(+), 6 deletions(-) diff --git a/src/Framework/Framework/Compilation/ControlTree/ControlTreeHelper.cs b/src/Framework/Framework/Compilation/ControlTree/ControlTreeHelper.cs index a1726d4549..2840188ad1 100644 --- a/src/Framework/Framework/Compilation/ControlTree/ControlTreeHelper.cs +++ b/src/Framework/Framework/Compilation/ControlTree/ControlTreeHelper.cs @@ -1,4 +1,5 @@ using System; +using System.Collections.Generic; using System.Linq; using DotVVM.Framework.Compilation.Parser.Dothtml.Parser; @@ -27,6 +28,19 @@ public static bool HasPropertyValue(this IAbstractControl control, IPropertyDesc return value; } + public static Dictionary GetPropertyGroup(this IAbstractControl control, IPropertyGroupDescriptor group) + { + var result = new Dictionary(); + 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) diff --git a/src/Framework/Framework/Compilation/ControlTree/IAbstractControl.cs b/src/Framework/Framework/Compilation/ControlTree/IAbstractControl.cs index 5431488b32..7babee5d33 100644 --- a/src/Framework/Framework/Compilation/ControlTree/IAbstractControl.cs +++ b/src/Framework/Framework/Compilation/ControlTree/IAbstractControl.cs @@ -8,6 +8,7 @@ namespace DotVVM.Framework.Compilation.ControlTree public interface IAbstractControl : IAbstractContentNode { IEnumerable PropertyNames { get; } + IEnumerable> Properties { get; } bool TryGetProperty(IPropertyDescriptor property, [NotNullWhen(true)] out IAbstractPropertySetter? value); diff --git a/src/Framework/Framework/Compilation/ControlTree/Resolved/ResolvedControl.cs b/src/Framework/Framework/Compilation/ControlTree/Resolved/ResolvedControl.cs index 9f8cfcede7..135f70047b 100644 --- a/src/Framework/Framework/Compilation/ControlTree/Resolved/ResolvedControl.cs +++ b/src/Framework/Framework/Compilation/ControlTree/Resolved/ResolvedControl.cs @@ -18,6 +18,10 @@ public class ResolvedControl : ResolvedContentNode, IAbstractControl IEnumerable IAbstractControl.PropertyNames => Properties.Keys; + IEnumerable> IAbstractControl.Properties => + Properties.Select(p => new KeyValuePair(p.Key, p.Value)); + + public ResolvedControl(ControlResolverMetadata metadata, DothtmlNode? node, DataContextStack dataContext) : base(metadata, node, dataContext) { } diff --git a/src/Framework/Framework/Controls/JsComponent.cs b/src/Framework/Framework/Controls/JsComponent.cs index 68840f76a7..273f40885b 100644 --- a/src/Framework/Framework/Controls/JsComponent.cs +++ b/src/Framework/Framework/Controls/JsComponent.cs @@ -170,11 +170,25 @@ public static IEnumerable ValidateUsage(ResolvedControl contr 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.", + $"This view does not have any view modules registered, 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 + ); + } } } diff --git a/src/Framework/Framework/Resources/Scripts/global-declarations.ts b/src/Framework/Framework/Resources/Scripts/global-declarations.ts index e875e05ef8..cf7ffc5b73 100644 --- a/src/Framework/Framework/Resources/Scripts/global-declarations.ts +++ b/src/Framework/Framework/Resources/Scripts/global-declarations.ts @@ -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 }, - templates: { [key: string]: string }, // TODO + templates: { [key: string]: string }, setProps: (p: { [key: string]: any }) => void ): DotvvmJsComponent } diff --git a/src/Framework/Framework/Resources/Scripts/viewModules/viewModuleManager.ts b/src/Framework/Framework/Resources/Scripts/viewModules/viewModuleManager.ts index e20cd2ee46..08b09c2248 100644 --- a/src/Framework/Framework/Resources/Scripts/viewModules/viewModuleManager.ts +++ b/src/Framework/Framework/Resources/Scripts/viewModules/viewModuleManager.ts @@ -133,7 +133,7 @@ export function findComponent( return [null, globalComponent[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)}, ...)`) + throw Error(`Cannot find a global JsComponent ${name}. Make sure it is registered 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)}: ... }`) diff --git a/src/Tests/Runtime/ControlTree/DefaultControlTreeResolver/CompilationWarningsTests.cs b/src/Tests/Runtime/ControlTree/DefaultControlTreeResolver/CompilationWarningsTests.cs index 85d7861743..e75e4b1493 100644 --- a/src/Tests/Runtime/ControlTree/DefaultControlTreeResolver/CompilationWarningsTests.cs +++ b/src/Tests/Runtime/ControlTree/DefaultControlTreeResolver/CompilationWarningsTests.cs @@ -45,7 +45,7 @@ public void CompilationWarning_JsComponentNoModules() """); Assert.AreEqual(1, warnings.Length); - StringAssert.Contains(warnings[0].warning, "This view does not have any view modules registred"); + StringAssert.Contains(warnings[0].warning, "This view does not have any view modules registered"); Assert.AreEqual("Test", warnings[0].tokens.Trim()); } From 2b8aa1a1a76ea94400a07503181fa0f6ff3a4d5d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Standa=20Luke=C5=A1?= Date: Sat, 8 Jun 2024 21:53:48 +0200 Subject: [PATCH 3/3] JsComponent: Add doccomment remark --- src/Framework/Framework/Controls/JsComponent.cs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/Framework/Framework/Controls/JsComponent.cs b/src/Framework/Framework/Controls/JsComponent.cs index 273f40885b..5a1d4e0f2f 100644 --- a/src/Framework/Framework/Controls/JsComponent.cs +++ b/src/Framework/Framework/Controls/JsComponent.cs @@ -16,6 +16,11 @@ namespace DotVVM.Framework.Controls { /// Control which initializes a client-side component. + /// + /// The client-side component is either exported from a view module referenced in the page's @js directive, or registered using the dotvvm.registerGlobalComponent method. + /// The module should export a $controls field with any number of named components, (TypeScript signature is $controls?: { [name:string]: DotvvmJsComponentFactory }) + /// + /// public class JsComponent : DotvvmControl { /// If set to true, view modules are ignored and JsComponents registered using dotvvm.registerGlobalComponent will be considered for client-side rendering.