Plugin Extensions: Mutable props for extension components (#107290)

* Plugin Extensions: Mutable props for extension components

* chore: updates after PR feedback

public/app/features/plugins/extensions/usePluginComponent.test.tsx

@@ -333,7 +333,7 @@ describe('usePluginComponent()', () => {
     expect(log.warning).not.toHaveBeenCalled();
   });
 
-  it('should pass a read-only copy of the props (in dev mode)', async () => {
+  it('should pass a writable copy of the props (in dev mode)', async () => {
     config.buildInfo.env = 'development';
 
     type Props = {
@@ -377,12 +377,13 @@ describe('usePluginComponent()', () => {
     const rendered = render(Component && <Component {...originalProps} />);
     expect(rendered.getByText('Foo')).toBeVisible();
 
-    // Should throw an error if it mutates the props
-    jest.spyOn(console, 'error').mockImplementation(() => {});
-    expect(() => render(Component && <Component {...originalProps} override />)).toThrow(
-      TypeError("'set' on proxy: trap returned falsish for property 'c'")
-    );
-    jest.spyOn(console, 'error').mockRestore();
+    // Should not throw an error if it mutates the props
+    expect(() => render(Component && <Component {...originalProps} override />)).not.toThrow();
+
+    // Should log a warning
+    expect(log.warning).toHaveBeenCalledWith('Attempted to mutate object property "c"', {
+      stack: expect.any(String),
+    });
   });
 
   it('should pass a writable copy of the props (in production mode)', async () => {
@@ -429,7 +430,7 @@ describe('usePluginComponent()', () => {
     const rendered = render(Component && <Component {...originalProps} />);
     expect(rendered.getByText('Foo')).toBeVisible();
 
-    // Should throw an error if it mutates the props
+    // Should not throw an error if it mutates the props
     expect(() => render(Component && <Component {...originalProps} override />)).not.toThrow();
 
     // Should log a warning

public/app/features/plugins/extensions/usePluginComponents.test.tsx

@@ -211,7 +211,7 @@ describe('usePluginComponents()', () => {
     });
   });
 
-  it('should pass a read only copy of the props to the components (in dev mode)', async () => {
+  it('should pass a copy of the props to the components (in dev mode)', async () => {
     config.buildInfo.env = 'development';
 
     type Props = {
@@ -264,9 +264,11 @@ describe('usePluginComponents()', () => {
     const rendered = render(<Component foo={originalFoo} />);
     expect(rendered.getByText('Foo')).toBeVisible();
 
-    // Check if it throws a TypeError due to trying to change the prop
-    jest.spyOn(console, 'error').mockImplementation(() => {});
-    expect(() => render(<Component foo={originalFoo} override />)).toThrow(TypeError);
+    // Should also render the component if it wants to change the props
+    expect(() => render(<Component foo={originalFoo} override />)).not.toThrow();
+    expect(log.warning).toHaveBeenCalledWith(`Attempted to mutate object property "foo4"`, {
+      stack: expect.any(String),
+    });
 
     // Check if the original property hasn't been changed
     expect(originalFoo.foo2.foo3.foo4).toBe('bar');

public/app/features/plugins/extensions/utils.test.tsx

@@ -1,4 +1,4 @@
-import { act, render, screen } from '@testing-library/react';
+import { render, screen } from '@testing-library/react';
 import { type Unsubscribable } from 'rxjs';
 
 import { dateTime, usePluginContext, PluginLoadingStrategy } from '@grafana/data';
@@ -476,7 +476,7 @@ describe('Plugin Extensions / Utils', () => {
       }).toThrow(TypeError);
     });
 
-    it('should return a read-only proxy of a deep-copy of the original object in dev mode', () => {
+    it('should return a writable deep-copy of the original object in dev mode', () => {
       config.featureToggles.extensionsReadOnlyProxy = false;
       config.buildInfo.env = 'development';
 
@@ -485,34 +485,16 @@ describe('Plugin Extensions / Utils', () => {
 
       expect(copy).not.toBe(obj);
       expect(copy.a).toBe('a');
-      expect(isReadOnlyProxy(copy)).toBe(true);
+      expect(isMutationObserverProxy(copy)).toBe(true);
       expect(() => {
         copy.a = 'b';
-      }).toThrow(TypeError);
-
-      // Also test that we can handle frozen objects
-      // (This is not possible with getReadOnlyProxy, as it throws an error when the object is already frozen)
-      const obj2 = {
-        a: {
-          b: {
-            c: {
-              d: 'd',
-            },
-          },
-        },
-      };
-
-      Object.freeze(obj2);
-      Object.freeze(obj2.a);
-      Object.freeze(obj2.a.b);
-
-      const copy2 = readOnlyCopy(obj2);
+      }).not.toThrow();
 
-      expect(() => {
-        copy2.a.b.c.d = 'testing';
-      }).toThrow("'set' on proxy: trap returned falsish for property 'd'");
+      expect(log.warning).toHaveBeenCalledWith(`Attempted to mutate object property "a"`, {
+        stack: expect.any(String),
+      });
 
-      expect(copy2.a.b.c.d).toBe('d');
+      expect(copy.a).toBe('b');
     });
 
     it('should return a writable deep-copy of the original object in production mode', () => {
@@ -705,7 +687,7 @@ describe('Plugin Extensions / Utils', () => {
       expect(screen.getByText('Version: 1.0.0')).toBeVisible();
     });
 
-    it('should not be possible to mutate the props in development mode, and it also throws an error', async () => {
+    it('should not be possible to mutate the props in development mode, but it logs a warning', async () => {
       config.buildInfo.env = 'development';
       const pluginId = 'grafana-worldmap-panel';
       const Component = wrapWithPluginContext(pluginId, ExampleComponent, log);
@@ -713,16 +695,17 @@ describe('Plugin Extensions / Utils', () => {
 
       jest.spyOn(console, 'error').mockImplementation();
 
-      await expect(async () => {
-        await act(async () => {
-          render(<Component {...props} override />);
-        });
-      }).rejects.toThrow(`'set' on proxy: trap returned falsish for property 'c'`);
+      render(<Component {...props} override />);
 
-      // Logs an error
-      expect(console.error).toHaveBeenCalledWith(expect.any(String));
+      expect(await screen.findByText('Hello Grafana!')).toBeVisible();
+
+      // Logs a warning
+      expect(log.warning).toHaveBeenCalledTimes(1);
+      expect(log.warning).toHaveBeenCalledWith(`Attempted to mutate object property "c"`, {
+        stack: expect.any(String),
+      });
 
-      // Not able to mutate the props in development mode
+      // Not able to mutate the props in dev mode either
       expect(props.a.b.c).toBe('Grafana');
     });
 

public/app/features/plugins/extensions/utils.tsx

@@ -295,11 +295,6 @@ export function readOnlyCopy<T>(value: T, _log: ExtensionsLog = log): T {
     return getReadOnlyProxy(value);
   }
 
-  // In dev mode: we return a read-only proxy (throws errors for any mutation), but with a deep-cloned version of the original object (so no interference with other call-sites)
-  if (isGrafanaDevMode()) {
-    return getReadOnlyProxy(cloneDeep(value));
-  }
-
   // Default: we return a proxy of a deep-cloned version of the original object, which logs warnings when mutation is attempted
   return getMutationObserverProxy(cloneDeep(value), _log);
 }