-
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
[Expressions] Remove the any
type usages
#113477
Conversation
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.
LGTM, code review only!
event: (event: any) => void; | ||
hasCompatibleActions?: (event: any) => Promise<boolean>; | ||
getRenderMode: () => RenderMode; | ||
done(): void; |
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.
arrow functions are preferred by our style guide if i remember correctly.
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.
I think it is not. Otherwise, that should be fixed there. Starting from TS 2.6, it has stricter type checks for the functions. When we define those methods as properties, which is unsound already, we are not able to extend or implement that interface with a subtype.
The following example will throw a compilation error:
interface A {
foo: (bar: unknown) => number;
}
const a: A = {
foo: (bar: number) => {
return bar;
},
};
class B implements A {
foo(bar: number) {
return bar;
}
}
But this one will not:
interface A {
foo(bar: unknown): number;
}
const a: A = {
foo: (bar: number) => {
return bar;
},
};
class B implements A {
foo(bar: number) {
return bar;
}
}
At first, I have fixed that for the methods with the any. But then I have corrected them all for consistency.
src/plugins/expressions/common/expression_types/expression_type.ts
Outdated
Show resolved
Hide resolved
@@ -110,7 +110,7 @@ export const mapColumn: ExpressionFunctionDefinition< | |||
map((rows) => { | |||
let type: DatatableColumnType = 'null'; | |||
for (const row of rows) { | |||
const rowType = getType(row[id]); | |||
const rowType = getType(row[id]) as DatatableColumnType; |
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.
should we update getType function to return correct type ?
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 string
is a supertype of the DatatableColumnType
. That's fine to cast it here.
src/plugins/expressions/common/expression_functions/specs/var_set.ts
Outdated
Show resolved
Hide resolved
245aa0c
to
e33b674
Compare
e33b674
to
a54736d
Compare
💚 Build SucceededMetrics [docs]Public APIs missing comments
Any counts in public APIs
Page load bundle
History
To update your PR or re-run it, just comment with: |
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.
code LGTM
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.
Presentation changes look good 👍
* Update ESLint config to disallow usage of the any type * Remove the any type usages from the expressions plugin * Update plugins using expressions according to the updated public API
* Update ESLint config to disallow usage of the any type * Remove the any type usages from the expressions plugin * Update plugins using expressions according to the updated public API
Summary
Removed most of the
any
type usages from the expressions plugin.Checklist
For maintainers