Improve SavedObjectAttributes type

[rudolf]

SavedObjectClient methods are currently typed with a generic that must extend SavedObjectAttributes:

// SavedObjectsClient#get() method signature
async get<T extends SavedObjectAttributes = any>(
  type: string,
  id: string,
  options: SavedObjectsBaseOptions = {}
): Promise<SavedObject<T>>

SavedObjectAttributes type

export type SavedObjectAttribute =
  | string
  | number
  | boolean
  | null
  | undefined
  | SavedObjectAttributes
  | SavedObjectAttributes[];

export interface SavedObjectAttributes {
  [key: string]: SavedObjectAttribute | SavedObjectAttribute[];
}

Because of this consumers of the API are forced to provide a generic parameter that extends SavedObjectAttributes or the any type. Extending from the SavedObjectAttributes type relaxes types to unknown/unspecified keys because of the index signature:

interface CanvasRenderedPage extends SavedObjectAttributes {
  elements: string[];
  groups: boolean;
}

const validCanvasRenderedPage: CanvasRenderedPage = {
  unknownKey: "you didn't want this key to be allowed",
  elements: ['renderedElementsArray'],
  groups: true,
}

validCanvasRenderedPage.hello = 'I should get a type warning here';

Although the SavedObjectAttributes type is useful for guaranteeing that a saved object is serializable, the risk of subtle business logic bugs introduced by the weaker type safety is arguable higher than that a SavedObject cannot be serialized.

To improve the typings we should:

1. Don't force SavedObjectsClient method generics to extend from SavedObjectAttributes but rather change the method signatures to match:

   // SavedObjectsClient#get() method signature
   async get<T = unknown>( // Previous default was any, so might require a lot of changes in consuming code
     type: string,
     id: string,
     options: SavedObjectsBaseOptions = {}
   ): Promise<SavedObject<T>>

2. If we're concerned about serializability we could implement something like the following for methods which accept arguments:

   export interface SavedObjectsClient {
   public async create<T>(
       type: string,
       attributes: SavedObjectSerializable<T>,
       options: SavedObjectsCreateOptions = { overwrite: false, references: [] }
     ): Promise<SavedObject<T>>;
   }
   
   export type SavedObjectSerializable<T> = {
     [K in keyof T]: T[K] extends SavedObjectAttribute | SavedObjectAttribute[]
       ? T[K]
       : T[K] extends Function | Date | symbol
       ? never
       : SavedObjectSerializable<T[K]>;
   };

   Unfortunately it doesn't create very descriptive type errors:

   Type '() => void' is not assignable to type 'never'.ts(2322)
   types.ts(73, 36): The expected type comes from property 'a' which is declared here on type 'SavedObjectSerializable<{ a: () => void; }>'

[elasticmachine]

Pinging @elastic/kibana-platform (Team:Platform)

[mshustov]

I think it still should be Record<string, unknown> since we need to enforce the Record interface. Currently, it's a blocker for #57774 due to microsoft/TypeScript#15300

[rudolf]

since we need to enforce the Record interface.

Do you want to use a Record to enforce that saved object attributes are always serializable?

[mshustov]

Do you want to use a Record to enforce that saved object attributes are always serializable?

no, my bad. It loosens the type because of the index signature. We can enforce Record interface when microsoft/TypeScript#15300 lands in TS