fix!: deepMerge for arrays, shortcut keycodes returned as array

[maribethb]

The basics

• I validated my changes

The details

Resolves

Fixes #9008

Proposed Changes

Fixes the deepMerge function so that it works correctly for arrays. It still probably does not work correctly for other cases, so I also added a warning to the tsdoc.

Reason for Changes

This was causing the shortcut registry to return the keycodes as an object instead of as an array.

Test Coverage

Added tests for the deepMerge function and for the keycodes property of keyboard shortcuts.

BREAKING CHANGES

• If you are using the Blockly.utils.object.deepMerge function, it previously incorrectly converted some arrays to objects. You probably shouldn't be using this version anyway. If you need deep merge functionality we'd recommend you use a separate library that is fully functional and has solid tests.
• If you are getting shortcuts out of the keyboard registry and then re-registering them or reusing the keyCodes property, you can now do so without resorting to workarounds to convert the object back into an array. Your workaround may now fail to work as the underlying data is now correctly an array instead of an object, so remove your workaround.

[BenHenning]

Thanks @maribethb. Approving to unblock, but I have some thoughts and am happy to take another review pass.

[BenHenning]

From a typing perspective (and since this is an API breakage, anyway), would it be better to explicitly type this now?

I'm assuming this can be typed something like so:

export interface MergeableObject<T> {
  [key: string]: MergeableObject<T> | T[] | null;
}

export function deepMerge(target: MergeableObject<unknown>, source: MergeableObject<unknown>): MergeableObject<unknown> {
  for (const x in source) {
    if (source[x] !== null) {
      if (Array.isArray(source[x])) {
        if (!Array.isArray(target[x])) {
          throw new Error(`Expected source and target to both be arrays at key: ${x}.`);
        }
        target[x] = deepMergeArray((target[x] as unknown[] | null) || [], source[x] as unknown[]);
      } else if (typeof source[x] === 'object') {
        if (typeof target[x] !== 'object') {
          throw new Error(`Expected source and target to both be objects at key: ${x}.`);
        }
        target[x] = deepMerge((target[x] as MergeableObject<unknown> | null) || Object.create(null), source[x] as MergeableObject<unknown>);
      } else {
        target[x] = source[x];
      }
    }
  }
  return target;
}

function deepMergeArray(target: unknown[], source: unknown[]): unknown[] {
  for (const x in source) {
    if (source[x] !== null) {
      if (Array.isArray(source[x])) {
        if (!Array.isArray(target[x])) {
          throw new Error(`Expected source and target to both be arrays at key: ${x}.`);
        }
        target[x] = deepMergeArray((target[x] as unknown[] | null) || [], source[x] as unknown[]);
      } else if (typeof source[x] === 'object') {
        if (typeof target[x] !== 'object') {
          throw new Error(`Expected source and target to both be objects at key: ${x}.`);
        }
        target[x] = deepMerge((target[x] as MergeableObject<unknown> | null) || Object.create(null), source[x] as MergeableObject<unknown>);
      } else {
        target[x] = source[x];
      }
    }
  }
  return target;
}

I think there's a way to consolidate these but couldn't quite figure it out.

I'm assuming we probably don't want to put time into typing this, though it did occur to me when drafting the above that there could be some element type inconsistencies.

[maribethb]

I don't think it's worth delaying the release to figure this out. I'd actually just like to remove the function, as actual deep merging is more complicated than we're making it look. I don't think your proposed version works correctly because for example it doesn't work if source[x] is an array but target[x] is undefined. It's not something I want to totally revamp right now, especially since it's only used internally in 2 places, and I sincerely hope that despite us exporting it, nobody else is using it.

Last night I got as far as removing it from themes (which don't even need to be "deep merged" as we're only ever using it to merge one layer of objects that just have strings) but couldn't quickly get rid of it for the shortcut registry so settled for fixing it instead.

[BenHenning]

Do these tests include merging child objects and arrays with the same keys? The actual child merging bit seems missing here unless I'm missing something.

[maribethb]

No, but to be fair we literally never actually try to do that. We only use this function to create a deep copy of registered shortcuts (so we're only merging with Object.null) and and to copy one set of (non-nested) properties from one theme object to another.