-
Notifications
You must be signed in to change notification settings - Fork 8.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add generic typings for SavedObjectMigrationFn #63943
Changes from 4 commits
b769c2a
b97d4c1
4374ec8
2504a55
a0ad3fb
2614f61
05912ff
8a03805
fa962a7
bb25186
9e8ebf7
cb373e9
4799896
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,7 +26,7 @@ import { SavedObjectsMigrationLogger } from './core/migration_logger'; | |
* | ||
* @example | ||
* ```typescript | ||
* const migrateProperty: SavedObjectMigrationFn = (doc, { log }) => { | ||
* const migrateProperty: SavedObjectMigrationFn<MyUnmigratedAttributes, MyMigratedAttributes> = (doc, { log }) => { | ||
* if(doc.attributes.someProp === null) { | ||
* log.warn('Skipping migration'); | ||
* } else { | ||
|
@@ -39,10 +39,10 @@ import { SavedObjectsMigrationLogger } from './core/migration_logger'; | |
* | ||
* @public | ||
*/ | ||
export type SavedObjectMigrationFn = ( | ||
doc: SavedObjectUnsanitizedDoc, | ||
export type SavedObjectMigrationFn<InputAttributes = unknown, MigratedAttributes = unknown> = ( | ||
doc: SavedObjectUnsanitizedDoc<InputAttributes>, | ||
context: SavedObjectMigrationContext | ||
) => SavedObjectUnsanitizedDoc; | ||
) => SavedObjectUnsanitizedDoc<MigratedAttributes>; | ||
Comment on lines
+56
to
+59
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, my only interrogation around that is that atm migrations are all mutating and returning the input I.E export const migrateToKibana660: SavedObjectMigrationFn<any, any> = doc => {
if (!doc.attributes.hasOwnProperty('disabledFeatures')) {
doc.attributes.disabledFeatures = [];
}
return doc;
}; Meaning that type InputAttrs = {
}
type OutputAttrs = {
disabledFeatures: SomeType[];
}
export const migrateToKibana660: SavedObjectMigrationFn<InputAttrs, OutputAttrs> = doc => {
if (!doc.attributes.hasOwnProperty('disabledFeatures')) {
doc.attributes.disabledFeatures = [];
}
return doc;
}; Would fail to compile with something like Which is why I'm wondering if we should add the output properties on the input export type SavedObjectMigrationFn<InputAttributes = unknown, MigratedAttributes = unknown> = (
doc: SavedObjectUnsanitizedDoc<InputAttributes & MigratedAttributes>,
context: SavedObjectMigrationContext
) => SavedObjectUnsanitizedDoc<MigratedAttributes>; Ideally I would have been using Partial, The other option being to decide that migration function should properly spread / construct the resulting migrated object without returning directly the input There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Given the high impact of even a small We can leave existing migrations with There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As a slightly more type-safe solution than There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. SGTM, I'll keep the definition as it is right now then. |
||
|
||
/** | ||
* Migration context provided when invoking a {@link SavedObjectMigrationFn | migration handler} | ||
|
Original file line number | Diff line number | Diff line change | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -14,7 +14,8 @@ interface XYLayerPre77 { | |||||||||||||
accessors: string[]; | ||||||||||||||
} | ||||||||||||||
|
||||||||||||||
export const migrations: Record<string, SavedObjectMigrationFn> = { | ||||||||||||||
// eslint-disable-next-line @typescript-eslint/no-explicit-any | ||||||||||||||
export const migrations: Record<string, SavedObjectMigrationFn<any, any>> = { | ||||||||||||||
Comment on lines
-134
to
+137
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Caused by Lines 736 to 741 in 40f8222
|
||||||||||||||
'7.7.0': doc => { | ||||||||||||||
const newDoc = cloneDeep(doc); | ||||||||||||||
if (newDoc.attributes?.visualizationType === 'lnsXY') { | ||||||||||||||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should update the example to show how to construct a copy so that the output is typed different to the input. Since this is only for type safety we don't need to make a deep copy, so it might be worth adding a comment to the example to highlight this.