Skip to content
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.

Sign up for GitHub

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

Jump to bottom

[ResponseOps] consistent-type-imports linting rule for RO packages/plugins - PR1 #212348

Open
wants to merge 13 commits into
base: main
Choose a base branch
from
Open

[ResponseOps] consistent-type-imports linting rule for RO packages/plugins - PR1 #212348

wants to merge 13 commits into from

Conversation

georgianaonoleata1904
Copy link
Contributor

@georgianaonoleata1904 georgianaonoleata1904 commented Feb 25, 2025 •
edited
Loading

  • Enabled @typescript-eslint/consistent-type-imports eslint rule for ResponseOps packages and plugins:

    • this rule ensures that imports used only for type declarations are consistently written using import type syntax

  • fixed eslint errors for:

    • src/platform/packages/shared/response-ops
    • x-pack/platform/plugins/shared/alerting

@georgianaonoleata1904 georgianaonoleata1904 self-assigned this Mar 4, 2025
@georgianaonoleata1904 georgianaonoleata1904 added Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) backport:version Backport to applied version labels v9.1.0 v8.19.0 release_note:skip Skip the PR/issue when compiling release notes labels Mar 4, 2025
@georgianaonoleata1904 georgianaonoleata1904 marked this pull request as ready for review March 4, 2025 09:28
@georgianaonoleata1904 georgianaonoleata1904 requested review from a team as code owners March 4, 2025 09:28
@elasticmachine
Copy link
Contributor

Pinging @elastic/response-ops (Team:ResponseOps)

@georgianaonoleata1904 georgianaonoleata1904 changed the title WIP - [ResponseOps] consistent-type-imports linting rule [ResponseOps] consistent-type-imports linting rule for RO packages/plugins - PR1 Mar 4, 2025
@georgianaonoleata1904
Copy link
Contributor Author

The is part 1/4. The following PRs have related changes:
#212366
#212498
#212508

adcoelho
adcoelho approved these changes Mar 4, 2025
View reviewed changes
Copy link
Contributor

@adcoelho adcoelho left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nice rule to enable, I had no idea we did this in so many places. 👍

@gsoldevila
Copy link
Contributor

Related to #212221

@gsoldevila
Copy link
Contributor

gsoldevila commented Mar 4, 2025 •
edited
Loading

I think it is worth addressing this globally for the whole repo, I'll create an issue for it.

If we really want to add this ESLint constraint, best strategy, as Matt suggests, would be a divide & conquer approach:

  • Make the ESLint rules a warning by default.
  • Create a list of all Kibana modules (~800 atm), grouped by owning team.
  • Add this list with each line commented out, as an override in .eslintrc.json, turning the warning into an error.
  • Create as many PRs as owning teams, and in each PR uncomment the corresponding modules in the eslint overrides.
  • Address issues with the import/no-duplicates rule, e.g. import type { …, type XXX, }.

umbopepato
umbopepato approved these changes Mar 5, 2025
View reviewed changes
Copy link
Member

@umbopepato umbopepato left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Super happy that we're enabling this rule, it'll help a lot. Thank you Georgiana! 🤗
LGTM, just left a comment where a type import is missing

src/platform/packages/shared/response-ops/rule_form/src/types.ts
import { FieldsMetadataPublicStart } from '@kbn/fields-metadata-plugin/public';
import {
import type { ActionConnector, ActionTypeRegistryContract } from '@kbn/alerts-ui-shared';
import type {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the FieldsMetadataPublicStart type import was lost here 🙂

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, it was lost at merge. Fixed it, thanks! :)

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

✅ unchanged

History

cc @georgianaonoleata1904

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@adcoelho adcoelho adcoelho approved these changes

@jbudz jbudz jbudz approved these changes

@umbopepato umbopepato umbopepato approved these changes

Assignees

@georgianaonoleata1904 georgianaonoleata1904

Labels
backport:version Backport to applied version labels release_note:skip Skip the PR/issue when compiling release notes Team:ResponseOps Label for the ResponseOps team (formerly the Cases and Alerting teams) v8.19.0 v9.1.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@georgianaonoleata1904 @elasticmachine @gsoldevila @adcoelho @jbudz @umbopepato @kibanamachine