mirror of
https://github.com/cpvalente/ontime.git
synced 2026-08-28 10:29:11 +00:00
Simplify entry clone back to a spread, cover it with two generic tests
The per-type clone functions from the previous commit listed every field of every entry type, which was a lot of code for a change meant to be a drop-in swap. They are replaced by the spread they were written to guard, keeping both review fixes and the exhaustiveness guard. The explicit literals were also weaker than they looked. They forced a new field to be named, but nothing stopped it being named as a plain alias, which is the actual bug we care about. The guarantee now comes from a test that walks a clone against its source and fails on any shared nested object or array. That covers a field added later without the clone enumerating fields, and it names the path it found: entry.triggers.0 is shared with the source Paired with a field-for-field comparison against structuredClone, the two tests pin both halves of the contract - same value, no shared references - and both were confirmed to fail when the corresponding fix is reverted. Dropping the literals also removes the `parent: undefined` divergence they introduced on entries that omit the key, since a spread copies exactly the keys that are present. Net: the diff for this PR goes from 261 to 159 added lines, with rundown.utils.ts down from 132 to 52.
This commit is contained in:
@@ -1,5 +1,6 @@
|
||||
import {
|
||||
EndAction,
|
||||
OntimeEntry,
|
||||
OntimeEvent,
|
||||
OntimeGroup,
|
||||
OntimeMilestone,
|
||||
@@ -732,69 +733,46 @@ describe('cloneEntryData()', () => {
|
||||
automationId: 'automation-1',
|
||||
};
|
||||
|
||||
const event = createEvent(
|
||||
{ title: 'An event', custom: { sponsor: 'a value' }, triggers: [trigger] },
|
||||
'cue-1',
|
||||
) as OntimeEvent;
|
||||
// the real factories, so these are complete entries exactly as the rundown holds them
|
||||
const group = createGroup({ id: 'group-1', entries: ['a', 'b'], custom: { sponsor: 'a value' } });
|
||||
const milestone = createMilestone({ id: 'milestone-1', custom: { sponsor: 'a value' } });
|
||||
const delay = createDelay({ id: 'delay-1', duration: 10 });
|
||||
const entries: [string, OntimeEntry][] = [
|
||||
['event', createEvent({ custom: { sponsor: 'a value' }, triggers: [trigger] }, 'cue-1') as OntimeEvent],
|
||||
['group', createGroup({ id: 'group-1', entries: ['a', 'b'], custom: { sponsor: 'a value' } })],
|
||||
['milestone', createMilestone({ id: 'milestone-1', custom: { sponsor: 'a value' } })],
|
||||
['delay', createDelay({ id: 'delay-1', duration: 10 })],
|
||||
];
|
||||
|
||||
/**
|
||||
* The clone exists to replace structuredClone, so it has to agree with it exactly:
|
||||
* this catches a field being dropped, added or normalised (eg. undefined -> []).
|
||||
* Fails if any nested object or array in the clone is the same reference as the source,
|
||||
* so a field added later that needs a copy of its own is caught here without the clone
|
||||
* having to enumerate fields.
|
||||
*/
|
||||
it.each([
|
||||
['event', event],
|
||||
['group', group],
|
||||
['milestone', milestone],
|
||||
['delay', delay],
|
||||
])('produces the same value as structuredClone for a %s', (_label, entry) => {
|
||||
function expectNoSharedReferences(clone: unknown, source: unknown, path: string) {
|
||||
if (typeof source !== 'object' || source === null) return;
|
||||
expect(clone, `${path} is shared with the source`).not.toBe(source);
|
||||
const cloneRecord = clone as Record<string, unknown>;
|
||||
const sourceRecord = source as Record<string, unknown>;
|
||||
for (const key of Object.keys(sourceRecord)) {
|
||||
expectNoSharedReferences(cloneRecord[key], sourceRecord[key], `${path}.${key}`);
|
||||
}
|
||||
}
|
||||
|
||||
/** the two halves of the structuredClone contract: same value, no shared references */
|
||||
it.each(entries)('clones a %s to the same value structuredClone would produce', (_type, entry) => {
|
||||
expect(cloneEntryData(entry)).toStrictEqual(structuredClone(entry));
|
||||
});
|
||||
|
||||
it('gives the event a custom object that is not shared with the source', () => {
|
||||
const clone = cloneEntryData(event);
|
||||
clone.custom.sponsor = 'changed';
|
||||
expect(event.custom.sponsor).toBe('a value');
|
||||
});
|
||||
|
||||
it('gives the event a triggers array, and each trigger, that is not shared with the source', () => {
|
||||
const clone = cloneEntryData(event);
|
||||
expect(clone.triggers).not.toBe(event.triggers);
|
||||
expect(clone.triggers[0]).not.toBe(event.triggers[0]);
|
||||
|
||||
clone.triggers[0].title = 'changed';
|
||||
clone.triggers.push({ ...trigger, id: 'trigger-2' });
|
||||
|
||||
expect(event.triggers[0].title).toBe('Go on air');
|
||||
expect(event.triggers).toHaveLength(1);
|
||||
});
|
||||
|
||||
it('gives the group an entries array that is not shared with the source', () => {
|
||||
const clone = cloneEntryData(group);
|
||||
clone.entries.push('c');
|
||||
expect(group.entries).toStrictEqual(['a', 'b']);
|
||||
});
|
||||
|
||||
it('gives the milestone a custom object that is not shared with the source', () => {
|
||||
const clone = cloneEntryData(milestone);
|
||||
clone.custom.sponsor = 'changed';
|
||||
expect(milestone.custom.sponsor).toBe('a value');
|
||||
it.each(entries)('shares no nested object or array with the source %s', (_type, entry) => {
|
||||
expectNoSharedReferences(cloneEntryData(entry), entry, 'entry');
|
||||
});
|
||||
|
||||
/**
|
||||
* Regression: normalising an absent value to [] makes deepEqual report a change on every
|
||||
* comparison, which would have the runtime re-broadcast and re-save the restore point on
|
||||
* every tick. See PR #2178.
|
||||
* Regression: normalising an absent container to an empty one makes deepEqual report a
|
||||
* change on every comparison, which would have the runtime re-broadcast and re-save the
|
||||
* restore point on every tick. See PR #2178.
|
||||
*/
|
||||
it('leaves an absent triggers or entries value absent rather than normalising it to []', () => {
|
||||
const partialEvent = makeOntimeEvent({ id: 'partial-event' });
|
||||
const partialGroup = makeOntimeGroup({ id: 'partial-group', entries: undefined });
|
||||
|
||||
expect(cloneEntryData(partialEvent).triggers).toBeUndefined();
|
||||
expect(cloneEntryData(partialGroup).entries).toBeUndefined();
|
||||
it('leaves an absent triggers or entries value absent', () => {
|
||||
expect(cloneEntryData(makeOntimeEvent({ id: 'partial' })).triggers).toBeUndefined();
|
||||
expect(cloneEntryData(makeOntimeGroup({ id: 'partial', entries: undefined })).entries).toBeUndefined();
|
||||
});
|
||||
|
||||
it('throws on an entry type it does not know how to clone', () => {
|
||||
|
||||
@@ -499,106 +499,26 @@ export function cloneSimpleRundownEntry(entry: OntimeEntry, newId: EntryId): Ont
|
||||
throw new Error(`Unsupported entry type for cloning: ${entry}`);
|
||||
}
|
||||
|
||||
/**
|
||||
* Fast, shape-aware clones of a rundown entry, one per entry type.
|
||||
*
|
||||
* Every field is listed explicitly rather than spread - the same pattern as the
|
||||
* `create*Patch` functions above - so adding a field to an entry type fails to compile here
|
||||
* until it is handled, and whoever adds it has to decide whether it needs a copy of its own.
|
||||
* The nested containers (`custom`, `triggers`, `entries`) get a fresh copy so the result can
|
||||
* be mutated freely without touching the source, which is what makes these a drop-in
|
||||
* replacement for `structuredClone` at a fraction of the cost.
|
||||
*/
|
||||
function cloneEventData(entry: OntimeEvent): OntimeEvent {
|
||||
return {
|
||||
id: entry.id,
|
||||
type: entry.type,
|
||||
flag: entry.flag,
|
||||
cue: entry.cue,
|
||||
title: entry.title,
|
||||
note: entry.note,
|
||||
endAction: entry.endAction,
|
||||
timerType: entry.timerType,
|
||||
countToEnd: entry.countToEnd,
|
||||
linkStart: entry.linkStart,
|
||||
timeStrategy: entry.timeStrategy,
|
||||
timeStart: entry.timeStart,
|
||||
timeEnd: entry.timeEnd,
|
||||
duration: entry.duration,
|
||||
skip: entry.skip,
|
||||
colour: entry.colour,
|
||||
timeWarning: entry.timeWarning,
|
||||
timeDanger: entry.timeDanger,
|
||||
parent: entry.parent,
|
||||
revision: entry.revision,
|
||||
delay: entry.delay,
|
||||
dayOffset: entry.dayOffset,
|
||||
gap: entry.gap,
|
||||
custom: { ...entry.custom },
|
||||
// the array and each trigger in it, otherwise a mutable transaction could edit a trigger
|
||||
// of the cached rundown before commit.
|
||||
// partially formed entries appear in fixtures: keep an absent value absent rather than
|
||||
// normalising it to [], which would make deepEqual report a change on every comparison
|
||||
triggers: entry.triggers?.map((trigger) => ({ ...trigger })) as OntimeEvent['triggers'],
|
||||
};
|
||||
}
|
||||
|
||||
function cloneGroupData(entry: OntimeGroup): OntimeGroup {
|
||||
return {
|
||||
id: entry.id,
|
||||
type: entry.type,
|
||||
title: entry.title,
|
||||
note: entry.note,
|
||||
targetDuration: entry.targetDuration,
|
||||
colour: entry.colour,
|
||||
revision: entry.revision,
|
||||
timeStart: entry.timeStart,
|
||||
timeEnd: entry.timeEnd,
|
||||
duration: entry.duration,
|
||||
isFirstLinked: entry.isFirstLinked,
|
||||
custom: { ...entry.custom },
|
||||
// ids are strings, so copying the array is enough
|
||||
entries: entry.entries?.slice() as OntimeGroup['entries'],
|
||||
};
|
||||
}
|
||||
|
||||
function cloneMilestoneData(entry: OntimeMilestone): OntimeMilestone {
|
||||
return {
|
||||
id: entry.id,
|
||||
type: entry.type,
|
||||
cue: entry.cue,
|
||||
title: entry.title,
|
||||
note: entry.note,
|
||||
colour: entry.colour,
|
||||
parent: entry.parent,
|
||||
revision: entry.revision,
|
||||
custom: { ...entry.custom },
|
||||
};
|
||||
}
|
||||
|
||||
function cloneDelayData(entry: OntimeDelay): OntimeDelay {
|
||||
return {
|
||||
id: entry.id,
|
||||
type: entry.type,
|
||||
duration: entry.duration,
|
||||
parent: entry.parent,
|
||||
};
|
||||
}
|
||||
|
||||
/**
|
||||
* Fast, shape-aware clone of a single entry, preserving its identity (id, revision, etc).
|
||||
* Drop-in replacement for `structuredClone(entry)`.
|
||||
* Drop-in replacement for `structuredClone(entry)`, at a fraction of the cost: the spread
|
||||
* already copies every primitive field by value, so only the nested containers need work.
|
||||
* `custom` and `entries` hold primitives, so copying the container is enough. `triggers`
|
||||
* holds objects, which are copied too - otherwise a mutable transaction could edit a trigger
|
||||
* of the cached rundown before commit.
|
||||
* An absent container is left absent rather than normalised to an empty one, so that a clone
|
||||
* still compares equal to the entry it came from.
|
||||
*/
|
||||
export function cloneEntryData<T extends OntimeEntry>(entry: T): T {
|
||||
switch (entry.type) {
|
||||
case SupportedEntry.Event:
|
||||
return cloneEventData(entry) as T;
|
||||
return { ...entry, custom: { ...entry.custom }, triggers: entry.triggers?.map((t) => ({ ...t })) } as T;
|
||||
case SupportedEntry.Group:
|
||||
return cloneGroupData(entry) as T;
|
||||
return { ...entry, custom: { ...entry.custom }, entries: entry.entries?.slice() } as T;
|
||||
case SupportedEntry.Milestone:
|
||||
return cloneMilestoneData(entry) as T;
|
||||
return { ...entry, custom: { ...entry.custom } } as T;
|
||||
case SupportedEntry.Delay:
|
||||
return cloneDelayData(entry) as T;
|
||||
return { ...entry } as T;
|
||||
default: {
|
||||
// exhaustiveness guard: a new member of `SupportedEntry` is named in the error here
|
||||
const unhandled: never = entry;
|
||||
|
||||
Reference in New Issue
Block a user