Skip to content

[Discover] Enable consistent-type-imports eslint rule#212293

Merged
davismcphee merged 6 commits intoelastic:mainfrom
davismcphee:discover-consistent-type-imports
Mar 10, 2025
Merged

[Discover] Enable consistent-type-imports eslint rule#212293
davismcphee merged 6 commits intoelastic:mainfrom
davismcphee:discover-consistent-type-imports

Conversation

@davismcphee
Copy link
Contributor

@davismcphee davismcphee commented Feb 24, 2025 •
edited by kibanamachine
Loading

Summary

This PR enables the @typescript-eslint/consistent-type-imports eslint rule for the Discover and saved search plugins. The benefits are that it keeps things a bit cleaner, but more importantly ensures we aren't accidentally importing more than types when all we need is types, which can cause side effects.

I've added backport:prev-major and backport:prev-minor labels to the PR because I figure backporting this is safe and would reduce merge conflicts in future backports.

We should consider enabling this for all of our plugins and packages, although that list might harder to maintain. I'm also curious if anyone knows of other eslint rules we don't currently use that we'd benefit from enabling.

Checklist

  • Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
  • Documentation was added for features that require explanation or tutorials
  • Unit or functional tests were updated or added to match the most common scenarios
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
  • This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
  • Flaky Test Runner was used on any tests changed
  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

@davismcphee davismcphee added Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. t// backport:prev-minor labels Feb 24, 2025
@davismcphee davismcphee self-assigned this Feb 24, 2025
@davismcphee davismcphee marked this pull request as ready for review February 25, 2025 02:37
@davismcphee davismcphee requested review from a team as code owners February 25, 2025 02:37
@elasticmachine
Copy link
Contributor

elasticmachine commented Feb 25, 2025

Pinging @elastic/kibana-data-discovery (Team:DataDiscovery)

jughosta
jughosta approved these changes Feb 25, 2025
View reviewed changes
Copy link
Contributor

@jughosta jughosta left a comment

Choose a reason for hiding this comment

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

Nice idea!

We could also extend the list with our other plugins and packages like UnifiedHistogram and UnifiedDocViewer.

@davismcphee
Copy link
Contributor Author

davismcphee commented Feb 25, 2025

@jughosta Agreed! Now that I know I'm not the only one who thinks this is a good idea, I can definitely open another PR for our other plugins and packages 😁

yngrdyn
yngrdyn approved these changes Feb 28, 2025
View reviewed changes
Copy link
Contributor

@yngrdyn yngrdyn left a comment

Choose a reason for hiding this comment

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

Obs-ux-logs changes LGTM

@gsoldevila
Copy link
Contributor

gsoldevila commented Mar 4, 2025

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, }.

@davismcphee
Copy link
Contributor Author

davismcphee commented Mar 5, 2025

@gsoldevila That sounds great to me. In the meantime would it be ok for us to get ahead of things in our plugins and merge this PR?

Bamieh
Bamieh approved these changes Mar 7, 2025
View reviewed changes
Copy link
Contributor

@Bamieh Bamieh left a comment

Choose a reason for hiding this comment

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

Core changes LGTM (1 file change)

@davismcphee
Copy link
Contributor Author

davismcphee commented Mar 7, 2025

/ci

@elasticmachine
Copy link
Contributor

elasticmachine commented Mar 10, 2025

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
discover 921.6KB 921.6KB -4.0B

History

cc @davismcphee

@davismcphee davismcphee added backport:version Backport to applied version labels v9.1.0 v8.19.0 and removed backport:prev-major labels Mar 10, 2025
@davismcphee davismcphee merged commit c398102 into elastic:main Mar 10, 2025
18 checks passed
@davismcphee davismcphee deleted the discover-consistent-type-imports branch March 10, 2025 03:10
@kibanamachine
Copy link
Contributor

kibanamachine commented Mar 10, 2025

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/13756005544

@kibanamachine
Copy link
Contributor

kibanamachine commented Mar 10, 2025

💔 All backports failed

Status Branch Result
❌ 8.x Backport failed because of merge conflicts

You might need to backport the following PRs to 8.x:
- [Discover] Replace DiscoverInternalStateContainer with Redux based InternalStateStore (#208784)

Manual backport

To create the backport manually run:

node scripts/backport --pr 212293

Questions ?

Please refer to the Backport tool documentation

This was referenced Mar 10, 2025
Merged
Merged
Merged
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 12, 2025
@kibanamachine
Copy link
Contributor

kibanamachine commented Mar 12, 2025

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 212293 locally

@kibanamachine kibanamachine mentioned this pull request Mar 12, 2025
Merged
5 tasks
@kibanamachine
Copy link
Contributor

kibanamachine commented Mar 13, 2025

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 212293 locally

1 similar comment
@kibanamachine
Copy link
Contributor

kibanamachine commented Mar 14, 2025

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create automatically backports add a backport:* label or prevent reminders by adding the backport:skip label.
You can also create backports manually by running node scripts/backport --pr 212293 locally

@davismcphee davismcphee mentioned this pull request Mar 14, 2025
Merged
@davismcphee
Copy link
Contributor Author

davismcphee commented Mar 14, 2025

💚 All backports created successfully

Status Branch Result
✅ 8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

davismcphee added a commit to davismcphee/kibana that referenced this pull request Mar 14, 2025
@davismcphee
[Discover] Enable consistent-type-imports eslint rule (elastic#212293)
1b7c850 
## Summary

This PR enables the `@typescript-eslint/consistent-type-imports` eslint
rule for the Discover and saved search plugins. The benefits are that it
keeps things a bit cleaner, but more importantly ensures we aren't
accidentally importing more than types when all we need is types, which
can cause side effects.

I've added `backport:prev-major` and `backport:prev-minor` labels to the
PR because I figure backporting this is safe and would reduce merge
conflicts in future backports.

We should consider enabling this for all of our plugins and packages,
although that list might harder to maintain. I'm also curious if anyone
knows of other eslint rules we don't currently use that we'd benefit
from enabling.

### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
(cherry picked from commit c398102)

# Conflicts:
#	src/platform/plugins/shared/discover/public/application/context/context_app.tsx
#	src/platform/plugins/shared/discover/public/application/context/context_app_content.test.tsx
#	src/platform/plugins/shared/discover/public/application/context/context_app_content.tsx
#	src/platform/plugins/shared/discover/public/application/doc/components/doc.test.tsx
#	src/platform/plugins/shared/discover/public/application/main/components/layout/discover_layout.tsx
#	src/platform/plugins/shared/discover/public/application/main/components/loading_spinner/loading_spinner.tsx
#	src/platform/plugins/shared/discover/public/application/main/components/top_nav/on_save_search.tsx
#	src/platform/plugins/shared/discover/public/application/main/data_fetching/update_search_source.ts
#	src/platform/plugins/shared/discover/public/application/main/discover_main_route.test.tsx
#	src/platform/plugins/shared/discover/public/application/main/discover_main_route.tsx
#	src/platform/plugins/shared/discover/public/application/main/state_management/discover_state.ts
#	src/platform/plugins/shared/discover/public/application/main/state_management/utils/get_state_defaults.ts
#	src/platform/plugins/shared/discover/public/components/data_types/traces/summary_column.tsx
#	src/platform/plugins/shared/discover/public/context_awareness/profile_providers/observability/traces_data_source_profile/accessors/get_doc_viewer.tsx
#	src/platform/plugins/shared/discover/public/context_awareness/profile_providers/observability/traces_data_source_profile/profile.test.ts
#	src/platform/plugins/shared/discover/public/context_awareness/profile_providers/observability/traces_data_source_profile/profile.ts
#	src/platform/plugins/shared/discover/public/context_awareness/profile_providers/security/accessors/get_cell_renderer_accessor.test.tsx
#	src/platform/plugins/shared/discover/public/context_awareness/profile_providers/security/accessors/get_cell_renderer_accessor.tsx
#	src/platform/plugins/shared/discover/public/context_awareness/profile_providers/security/security_root_profile/profile.tsx
#	src/platform/plugins/shared/discover/public/embeddable/components/search_embeddable_grid_component.tsx
#	src/platform/plugins/shared/discover/public/plugin.tsx
#	src/platform/plugins/shared/discover/server/locator/columns_from_locator.ts
davismcphee added a commit that referenced this pull request Mar 14, 2025
@davismcphee @kibanamachine
[8.x] [Discover] Enable consistent-type-imports eslint rule (#212293)…
a7d0bd8 
… (#214641)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Discover] Enable `consistent-type-imports` eslint rule
(#212293)](#212293)

<!--- Backport version: 9.6.6 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sorenlouv/backport)

<!--BACKPORT [{"author":{"name":"Davis
McPhee","email":"davis.mcphee@elastic.co"},"sourceCommit":{"committedDate":"2025-03-10T03:09:57Z","message":"[Discover]
Enable `consistent-type-imports` eslint rule (#212293)\n\n##
Summary\n\nThis PR enables the
`@typescript-eslint/consistent-type-imports` eslint\nrule for the
Discover and saved search plugins. The benefits are that it\nkeeps
things a bit cleaner, but more importantly ensures we
aren't\naccidentally importing more than types when all we need is
types, which\ncan cause side effects.\n\nI've added
`backport:prev-major` and `backport:prev-minor` labels to the\nPR
because I figure backporting this is safe and would reduce
merge\nconflicts in future backports.\n\nWe should consider enabling
this for all of our plugins and packages,\nalthough that list might
harder to maintain. I'm also curious if anyone\nknows of other eslint
rules we don't currently use that we'd benefit\nfrom enabling.\n\n###
Checklist\n\n- [ ] Any text added follows [EUI's
writing\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\nsentence case text and includes
[i18n\nsupport](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)\n-
[
]\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\nwas
added for features that require explanation or tutorials\n- [ ] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common scenarios\n- [ ] If a plugin
configuration key changed, check if it needs to be\nallowlisted in the
cloud and added to the
[docker\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\n-
[ ] This was checked for breaking HTTP API changes, and any
breaking\nchanges have been approved by the breaking-change committee.
The\n`release_note:breaking` label should be applied in these
situations.\n- [ ] [Flaky
Test\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\nused on any tests changed\n- [ ] The PR description includes the
appropriate Release Notes section,\nand the correct `release_note:*`
label is applied per
the\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\n\n---------\n\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"c398102c79a48d7f2c26c7f0b73bd16ee9ce9f7c","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["Feature:Discover","release_note:skip","backport
missing","Team:DataDiscovery","backport:version","v9.1.0","v8.19.0"],"title":"[Discover]
Enable `consistent-type-imports` eslint
rule","number":212293,"url":"https://github.com/elastic/kibana/pull/212293","mergeCommit":{"message":"[Discover]
Enable `consistent-type-imports` eslint rule (#212293)\n\n##
Summary\n\nThis PR enables the
`@typescript-eslint/consistent-type-imports` eslint\nrule for the
Discover and saved search plugins. The benefits are that it\nkeeps
things a bit cleaner, but more importantly ensures we
aren't\naccidentally importing more than types when all we need is
types, which\ncan cause side effects.\n\nI've added
`backport:prev-major` and `backport:prev-minor` labels to the\nPR
because I figure backporting this is safe and would reduce
merge\nconflicts in future backports.\n\nWe should consider enabling
this for all of our plugins and packages,\nalthough that list might
harder to maintain. I'm also curious if anyone\nknows of other eslint
rules we don't currently use that we'd benefit\nfrom enabling.\n\n###
Checklist\n\n- [ ] Any text added follows [EUI's
writing\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\nsentence case text and includes
[i18n\nsupport](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)\n-
[
]\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\nwas
added for features that require explanation or tutorials\n- [ ] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common scenarios\n- [ ] If a plugin
configuration key changed, check if it needs to be\nallowlisted in the
cloud and added to the
[docker\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\n-
[ ] This was checked for breaking HTTP API changes, and any
breaking\nchanges have been approved by the breaking-change committee.
The\n`release_note:breaking` label should be applied in these
situations.\n- [ ] [Flaky
Test\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\nused on any tests changed\n- [ ] The PR description includes the
appropriate Release Notes section,\nand the correct `release_note:*`
label is applied per
the\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\n\n---------\n\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"c398102c79a48d7f2c26c7f0b73bd16ee9ce9f7c"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/212293","number":212293,"mergeCommit":{"message":"[Discover]
Enable `consistent-type-imports` eslint rule (#212293)\n\n##
Summary\n\nThis PR enables the
`@typescript-eslint/consistent-type-imports` eslint\nrule for the
Discover and saved search plugins. The benefits are that it\nkeeps
things a bit cleaner, but more importantly ensures we
aren't\naccidentally importing more than types when all we need is
types, which\ncan cause side effects.\n\nI've added
`backport:prev-major` and `backport:prev-minor` labels to the\nPR
because I figure backporting this is safe and would reduce
merge\nconflicts in future backports.\n\nWe should consider enabling
this for all of our plugins and packages,\nalthough that list might
harder to maintain. I'm also curious if anyone\nknows of other eslint
rules we don't currently use that we'd benefit\nfrom enabling.\n\n###
Checklist\n\n- [ ] Any text added follows [EUI's
writing\nguidelines](https://elastic.github.io/eui/#/guidelines/writing),
uses\nsentence case text and includes
[i18n\nsupport](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)\n-
[
]\n[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)\nwas
added for features that require explanation or tutorials\n- [ ] [Unit or
functional\ntests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)\nwere
updated or added to match the most common scenarios\n- [ ] If a plugin
configuration key changed, check if it needs to be\nallowlisted in the
cloud and added to the
[docker\nlist](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)\n-
[ ] This was checked for breaking HTTP API changes, and any
breaking\nchanges have been approved by the breaking-change committee.
The\n`release_note:breaking` label should be applied in these
situations.\n- [ ] [Flaky
Test\nRunner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1)
was\nused on any tests changed\n- [ ] The PR description includes the
appropriate Release Notes section,\nand the correct `release_note:*`
label is applied per
the\n[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)\n\n---------\n\nCo-authored-by:
kibanamachine
<42973632+kibanamachine@users.noreply.github.com>","sha":"c398102c79a48d7f2c26c7f0b73bd16ee9ce9f7c"}},{"branch":"8.x","label":"v8.19.0","branchLabelMappingKey":"^v8.19.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Mar 14, 2025
CAWilson94 pushed a commit to CAWilson94/kibana that referenced this pull request Mar 22, 2025
@davismcphee @kibanamachine @CAWilson94
[Discover] Enable consistent-type-imports eslint rule (elastic#212293)
7f003db 
## Summary

This PR enables the `@typescript-eslint/consistent-type-imports` eslint
rule for the Discover and saved search plugins. The benefits are that it
keeps things a bit cleaner, but more importantly ensures we aren't
accidentally importing more than types when all we need is types, which
can cause side effects.

I've added `backport:prev-major` and `backport:prev-minor` labels to the
PR because I figure backporting this is safe and would reduce merge
conflicts in future backports.

We should consider enabling this for all of our plugins and packages,
although that list might harder to maintain. I'm also curious if anyone
knows of other eslint rules we don't currently use that we'd benefit
from enabling.

### Checklist

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

---------

Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yngrdyn yngrdyn yngrdyn approved these changes

@jughosta jughosta jughosta approved these changes

@jbudz jbudz jbudz approved these changes

@Bamieh Bamieh Bamieh approved these changes

Assignees

@davismcphee davismcphee

Labels

backport:version Backport to applied version labels Feature:Discover Discover Application release_note:skip Skip the PR/issue when compiling release notes Team:DataDiscovery Discover, search (data plugin and KQL), data views, saved searches. For ES|QL, use Team:ES|QL. t// v8.19.0 v9.1.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

8 participants

@davismcphee @elasticmachine @gsoldevila @kibanamachine @yngrdyn @jughosta @jbudz @Bamieh