-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
chore: updates to fix ts errors in v4.9.5
upgrade
#176114
Conversation
…into upgrade-typescript-v4-9-5
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.
First batch of fixes, pretty confident in 😅
Merged in 0c669ff
...ns/chart_expressions/expression_legacy_metric/public/__stories__/metric_renderer.stories.tsx
Show resolved
Hide resolved
src/plugins/chart_expressions/expression_legacy_metric/public/components/with_auto_scale.tsx
Show resolved
Hide resolved
src/plugins/vis_types/timeseries/public/convert_to_lens/lib/convert/percentile.ts
Show resolved
Hide resolved
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.
Here is my attempt to fix x-pack/plugins/lens/public/datasources/form_based/operations/definitions/formula/formula.test.tsx
.
See a20a96c.
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.
These types are a little crazy but they are mocks trying to conform to multiple type definitions in a single function.
I reworked the types to pass a generic type based on the input
type, with it defaulting to the field
type.
This works for almost all cases, haven't checked other files 🤞🏼, except for used with the label: 'moving_average'
, see next comment 👇🏼 .
@@ -35,6 +35,7 @@ export interface ValueFormatConfig { | |||
// Formatting can optionally be added to any column | |||
export interface FormattedIndexPatternColumn extends BaseIndexPatternColumn { | |||
params?: { | |||
window?: number; | |||
format?: ValueFormatConfig; | |||
}; | |||
} |
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.
When defining the moving average definition like here...
Lines 58 to 74 in a20a96c
moving_average: createOperationDefinitionMock('moving_average', { | |
input: 'fullReference', | |
operationParams: [{ name: 'window', type: 'number', required: true }], | |
filterable: true, | |
getErrorMessage: jest.fn(() => ['mock error']), | |
buildColumn: ({ referenceIds }, columnsParams) => ({ | |
label: 'moving_average', | |
dataType: 'number', | |
operationType: 'moving_average', | |
isBucketed: false, | |
scale: 'ratio', | |
timeScale: undefined, | |
params: { window: 5 }, | |
references: referenceIds, | |
filter: getFilter(undefined, columnsParams), | |
}), | |
}), |
The type of params: { window: 5 }
is pointing to this type here thus why adding the window
prop fixes the error.
But I think window
type should be pulling from MovingAverageIndexPatternColumn
instead which is here...
Lines 45 to 51 in a20a96c
export type MovingAverageIndexPatternColumn = FormattedIndexPatternColumn & | |
ReferenceBasedIndexPatternColumn & { | |
operationType: typeof MOVING_AVERAGE_ID; | |
params: { | |
window: number; | |
}; | |
}; |
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.
The other two below are related and a bit more complex to solve.
x-pack/plugins/lens/public/editor_frame_service/editor_frame/workspace_panel/workspace_panel.tsx
x-pack/plugins/lens/public/embeddable/expression_wrapper.tsx
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.
These errors in both these files are due to the use of onData$
. I believe the problem is that onData$
is declared as a generic function with 2 parameters, see below.
kibana/src/plugins/expressions/public/react_expression_renderer/use_expression_renderer.ts
Lines 24 to 39 in a20a96c
export interface ExpressionRendererParams extends IExpressionLoaderParams { | |
debounce?: number; | |
expression: string | ExpressionAstExpression; | |
hasCustomErrorRenderer?: boolean; | |
onData$?<TData, TInspectorAdapters>( | |
data: TData, | |
adapters?: TInspectorAdapters, | |
partial?: boolean | |
): void; | |
onEvent?(event: ExpressionRendererEvent): void; | |
onRender$?(item: number): void; | |
/** | |
* An observable which can be used to re-run the expression without destroying the component | |
*/ | |
reload$?: Observable<unknown>; | |
} |
Then when used like in ReactExpressionRendererType
, it locks in the generic types for both TData
and TInspectorAdapters
, see here...
kibana/src/plugins/expressions/public/react_expression_renderer/react_expression_renderer.tsx
Lines 30 to 31 in a20a96c
export type ReactExpressionRendererType = React.ComponentType<ReactExpressionRendererProps>; | |
export type ExpressionRendererComponent = React.FC<ReactExpressionRendererProps>; |
But in both usages we set these types to a different type and typescript throws an error similar to...
Type 'TInspectorAdapters' is not assignable to type 'Partial<DefaultInspectorAdapters>'
Closing in favor of mistic#18 for simpler fixes. |
Summary
Fixes errors type errors from #175178 removing
@ts-expect-error
comments.Fixed in 89cf00e
See #176114 (review)
Attempted fix in a20a96c
See #176114 (review)
Still to be fixed
See #176114 (review)