allow overwriting hook properties (#3867)

This is required for backwards compatibility. When re-writing the code
to support giving classes, I added a check to prevent overwriting the hook's
own properties, but this is actually used by hooks in the wild, so we only
log a warning instead.

Author: SteffenDE

assets/js/phoenix_live_view/view_hook.ts

@@ -288,28 +288,12 @@ export class ViewHook implements HookInterface {
 
       for (const key in callbacks) {
         if (Object.prototype.hasOwnProperty.call(callbacks, key)) {
+          (this as any)[key] = callbacks[key];
+          // for backwards compatibility, we allow the overwrite, but we log a warning
           if (protectedProps.has(key)) {
-            // Optionally log a warning if a user tries to overwrite a protected property/method
-            // For now, we silently prioritize the ViewHook's own properties/methods.
-            if (
-              typeof (this as any)[key] === "function" &&
-              typeof callbacks[key] !== "function" &&
-              ![
-                "mounted",
-                "beforeUpdate",
-                "updated",
-                "destroyed",
-                "disconnected",
-                "reconnected",
-              ].includes(key)
-            ) {
-              // If core method is a function and callback is not, likely an error from user.
-              console.warn(
-                `Hook object for element #${el.id} attempted to overwrite core method '${key}' with a non-function value. This is not allowed.`,
-              );
-            }
-          } else {
-            (this as any)[key] = callbacks[key];
+            console.warn(
+              `Hook object for element #${el.id} overwrites core property '${key}'!`,
+            );
           }
         }
       }

assets/test/view_test.ts

@@ -1477,6 +1477,47 @@ describe("View Hooks", function () {
     expect(toHTML).toBe("updated");
     expect(view.el.firstChild.innerHTML).toBe("updated");
   });
+
+  test("can overwrite property", async () => {
+    let customHandleEventCalled = false;
+    const Hooks = {
+      Upcase: {
+        mounted() {
+          this.handleEvent = () => {
+            customHandleEventCalled = true;
+          };
+          this.el.innerHTML = this.el.innerHTML.toUpperCase();
+        },
+        updated() {
+          this.handleEvent();
+        },
+      },
+    };
+    liveSocket = new LiveSocket("/live", Socket, { hooks: Hooks });
+    const el = liveViewDOM();
+
+    const view = simulateJoinedView(el, liveSocket);
+
+    view.onJoin({
+      rendered: {
+        s: ['<h2 id="up" phx-hook="Upcase">test mount</h2>'],
+        fingerprint: 123,
+      },
+      liveview_version,
+    });
+    expect(view.el.firstChild.innerHTML).toBe("TEST MOUNT");
+    expect(Object.keys(view.viewHooks)).toHaveLength(1);
+
+    expect(customHandleEventCalled).toBe(false);
+    view.update(
+      {
+        s: ['<h2 id="up" phx-hook="Upcase">test update</h2>'],
+        fingerprint: 123,
+      },
+      [],
+    );
+    expect(customHandleEventCalled).toBe(true);
+  });
 });
 
 function liveViewComponent() {