diff --git a/.changeset/warm-llamas-bake.md b/.changeset/warm-llamas-bake.md new file mode 100644 index 0000000000..3da4a9b94d --- /dev/null +++ b/.changeset/warm-llamas-bake.md @@ -0,0 +1,5 @@ +--- +'@backstage/config-loader': patch +--- + +Do not unnecessarily notify subscribers when no-op updates to config happen diff --git a/packages/config-loader/src/sources/ObservableConfigProxy.test.ts b/packages/config-loader/src/sources/ObservableConfigProxy.test.ts index d1fccf3e80..f072536489 100644 --- a/packages/config-loader/src/sources/ObservableConfigProxy.test.ts +++ b/packages/config-loader/src/sources/ObservableConfigProxy.test.ts @@ -25,21 +25,17 @@ describe('ObservableConfigProxy', () => { const sub = config.subscribe(fn); expect(config.getOptionalNumber('x')).toBe(undefined); - config.setConfig(new ConfigReader({})); - expect(fn).toHaveBeenCalledTimes(1); - expect(config.getOptionalNumber('x')).toBe(undefined); - config.setConfig(new ConfigReader({ x: 1 })); - expect(fn).toHaveBeenCalledTimes(2); + expect(fn).toHaveBeenCalledTimes(1); expect(config.getOptionalNumber('x')).toBe(1); config.setConfig(new ConfigReader({ x: 3 })); - expect(fn).toHaveBeenCalledTimes(3); + expect(fn).toHaveBeenCalledTimes(2); sub.unsubscribe(); expect(config.getOptionalNumber('x')).toBe(3); config.setConfig(new ConfigReader({ x: 5 })); - expect(fn).toHaveBeenCalledTimes(3); + expect(fn).toHaveBeenCalledTimes(2); expect(config.getOptionalNumber('x')).toBe(5); }); @@ -58,18 +54,10 @@ describe('ObservableConfigProxy', () => { expect(config2.getOptionalNumber('x')).toBe(undefined); expect(config3.getOptionalNumber('x')).toBe(undefined); - config1.setConfig(new ConfigReader({})); + config1.setConfig(new ConfigReader({ x: 1, a: { x: 2, b: { x: 3 } } })); expect(fn1).toHaveBeenCalledTimes(1); expect(fn2).toHaveBeenCalledTimes(1); expect(fn3).toHaveBeenCalledTimes(1); - expect(config1.getOptionalNumber('x')).toBe(undefined); - expect(config2.getOptionalNumber('x')).toBe(undefined); - expect(config3.getOptionalNumber('x')).toBe(undefined); - - config1.setConfig(new ConfigReader({ x: 1, a: { x: 2, b: { x: 3 } } })); - expect(fn1).toHaveBeenCalledTimes(2); - expect(fn2).toHaveBeenCalledTimes(2); - expect(fn3).toHaveBeenCalledTimes(2); expect(config1.getNumber('x')).toBe(1); expect(config2.getNumber('x')).toBe(2); expect(config3.getNumber('x')).toBe(3); @@ -79,9 +67,9 @@ describe('ObservableConfigProxy', () => { sub3.unsubscribe(); config1.setConfig(new ConfigReader({ x: 4, a: { x: 5, b: { x: 6 } } })); - expect(fn1).toHaveBeenCalledTimes(2); - expect(fn2).toHaveBeenCalledTimes(2); - expect(fn3).toHaveBeenCalledTimes(2); + expect(fn1).toHaveBeenCalledTimes(1); + expect(fn2).toHaveBeenCalledTimes(1); + expect(fn3).toHaveBeenCalledTimes(1); expect(config1.getNumber('x')).toBe(4); expect(config2.getNumber('x')).toBe(5); expect(config3.getNumber('x')).toBe(6); @@ -140,4 +128,43 @@ describe('ObservableConfigProxy', () => { 'Only the root config can be closed', ); }); + + it('should only notify subscribers when the config data actually changes (ignoring key order)', () => { + const config = ObservableConfigProxy.create(new AbortController()); + const fn = jest.fn(); + config.subscribe(fn); + + expect(config.getOptionalNumber('a')).toBe(undefined); + + config.setConfig(new ConfigReader({})); + expect(fn).toHaveBeenCalledTimes(0); + expect(config.getOptionalNumber('a')).toBe(undefined); + + config.setConfig( + new ConfigReader({ + a: 1, + b: 1, + }), + ); + expect(fn).toHaveBeenCalledTimes(1); + expect(config.getOptionalNumber('a')).toBe(1); + + config.setConfig( + new ConfigReader({ + b: 1, + a: 1, + }), + ); + expect(fn).toHaveBeenCalledTimes(1); + expect(config.getOptionalNumber('a')).toBe(1); + + config.setConfig( + new ConfigReader({ + b: 1, + a: 2, + }), + ); + expect(fn).toHaveBeenCalledTimes(2); + expect(config.getOptionalNumber('a')).toBe(2); + }); }); diff --git a/packages/config-loader/src/sources/ObservableConfigProxy.ts b/packages/config-loader/src/sources/ObservableConfigProxy.ts index 59be836160..3d2084beac 100644 --- a/packages/config-loader/src/sources/ObservableConfigProxy.ts +++ b/packages/config-loader/src/sources/ObservableConfigProxy.ts @@ -16,6 +16,7 @@ import { Config, ConfigReader } from '@backstage/config'; import { JsonValue } from '@backstage/types'; +import isEqual from 'lodash/isEqual'; export class ObservableConfigProxy implements Config { private config: Config = new ConfigReader({}); @@ -40,12 +41,22 @@ export class ObservableConfigProxy implements Config { if (this.parent) { throw new Error('immutable'); } + + // We only notify subscribers if the data contents of the config actually + // changed. If they didn't, there's no point in callers trying to re-read + // them. However we still want to replace the local config object, since its + // runtime implementation could be entirely different. + const changed = !isEqual(this.config.get(), config.get()); + this.config = config; - for (const subscriber of this.subscribers) { - try { - subscriber(); - } catch (error) { - console.error(`Config subscriber threw error, ${error}`); + + if (changed) { + for (const subscriber of this.subscribers) { + try { + subscriber(); + } catch (error) { + console.error(`Config subscriber threw error, ${error}`); + } } } }