Merge pull request #1804 from riganti/jscomponent-global-errors

JsComponent: improve error reporting and doccomments

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<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)

src/Framework/Framework/Compilation/ControlTree/IAbstractControl.cs

@@ -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);
 

src/Framework/Framework/Compilation/ControlTree/Resolved/ResolvedControl.cs

@@ -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) { }
 

src/Framework/Framework/Controls/JsComponent.cs

@@ -1,19 +1,30 @@
+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;
 
 namespace DotVVM.Framework.Controls
 {
     /// <summary> Control which initializes a client-side component. </summary>
+    /// <remark>
+    /// 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 <c>$controls?: { [name:string]: DotvvmJsComponentFactory }</c>)
+    /// </remark>
+    /// <seealso href="https://www.dotvvm.com/docs/latest/pages/concepts/client-side-development/integrate-third-party-controls/react" />
     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)!; }
@@ -23,7 +34,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)!; }
@@ -33,6 +44,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)!; }
@@ -132,7 +144,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 +167,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 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
+                );
+            }
+        }
+
     }
 }

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<any> },
-        templates: { [key: string]: string }, // TODO
+        templates: { [key: string]: string },
         setProps: (p: { [key: string]: any }) => void
     ): DotvvmJsComponent
 }

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 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)}: ... }`)
+        }
+    }
+    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
 }
 

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)}}
+                <js:Test />
+            """);
+            Assert.AreEqual(1, warnings.Length);
+            StringAssert.Contains(warnings[0].warning, "This view does not have any view modules registered");
+            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")]
@@ -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

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": {