From 470f3c1578d71bf2405b6155db9dc5983b31f2bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Hugo=20H=C3=A4ggmark?= Date: Mon, 30 Jun 2025 05:54:12 +0200 Subject: [PATCH] Plugin Extensions: Mutable props for extension components (#107290) * Plugin Extensions: Mutable props for extension components * chore: updates after PR feedback --- .../extensions/usePluginComponent.test.tsx | 17 +++--- .../extensions/usePluginComponents.test.tsx | 10 ++-- .../plugins/extensions/utils.test.tsx | 53 +++++++------------ .../app/features/plugins/extensions/utils.tsx | 5 -- 4 files changed, 33 insertions(+), 52 deletions(-) diff --git a/public/app/features/plugins/extensions/usePluginComponent.test.tsx b/public/app/features/plugins/extensions/usePluginComponent.test.tsx index 51583987271..2e8e92f7321 100644 --- a/public/app/features/plugins/extensions/usePluginComponent.test.tsx +++ b/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 && ); expect(rendered.getByText('Foo')).toBeVisible(); - // Should throw an error if it mutates the props - jest.spyOn(console, 'error').mockImplementation(() => {}); - expect(() => render(Component && )).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 && )).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 && ); 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 && )).not.toThrow(); // Should log a warning diff --git a/public/app/features/plugins/extensions/usePluginComponents.test.tsx b/public/app/features/plugins/extensions/usePluginComponents.test.tsx index 7f306ff3335..c2ab7ec6845 100644 --- a/public/app/features/plugins/extensions/usePluginComponents.test.tsx +++ b/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(); 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()).toThrow(TypeError); + // Should also render the component if it wants to change the props + expect(() => render()).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'); diff --git a/public/app/features/plugins/extensions/utils.test.tsx b/public/app/features/plugins/extensions/utils.test.tsx index b2cf153cb7d..e1edf9bd3c8 100644 --- a/public/app/features/plugins/extensions/utils.test.tsx +++ b/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); + }).not.toThrow(); - // 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', - }, - }, - }, - }; + expect(log.warning).toHaveBeenCalledWith(`Attempted to mutate object property "a"`, { + stack: expect.any(String), + }); - Object.freeze(obj2); - Object.freeze(obj2.a); - Object.freeze(obj2.a.b); - - const copy2 = readOnlyCopy(obj2); - - expect(() => { - copy2.a.b.c.d = 'testing'; - }).toThrow("'set' on proxy: trap returned falsish for property 'd'"); - - 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(); - }); - }).rejects.toThrow(`'set' on proxy: trap returned falsish for property 'c'`); + render(); - // Logs an error - expect(console.error).toHaveBeenCalledWith(expect.any(String)); + expect(await screen.findByText('Hello Grafana!')).toBeVisible(); - // Not able to mutate the props in development mode + // 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 dev mode either expect(props.a.b.c).toBe('Grafana'); }); diff --git a/public/app/features/plugins/extensions/utils.tsx b/public/app/features/plugins/extensions/utils.tsx index e60cff38877..72f17ee6974 100644 --- a/public/app/features/plugins/extensions/utils.tsx +++ b/public/app/features/plugins/extensions/utils.tsx @@ -295,11 +295,6 @@ export function readOnlyCopy(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); }