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.

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

[eslint] add rule to prevent export* in plugin index files #109357

Merged
merged 29 commits into from
Sep 2, 2021

Conversation

spalger
Copy link
Contributor

@spalger spalger commented Aug 19, 2021

Reviewers!

These changes are just adding /* eslint-disable */ comments to tons of plugins so don't worry about reviewing. I'll be merging without reviews. Just please check the issues created for your team and in: https://github.com/elastic/kibana/projects/76


Implements the ESLint rule requested by #57370

Unfortunately, auto-fixing these violations perfectly isn't easy (we tried, but it created a number of problems) so rather than spend a ton of time trying to get the auto-fix perfect I suggest that we disable all the current violations and create issues for each team to remove the eslint-disable comments. Removing the comment will auto-fix, but since it's imperfect folks will need to review the changes.

The list of plugins with violations by team:

  • kibana-app-services: (10) bfetch, data, embeddable, expressions, fieldFormats, inspector, kibanaReact, kibanaUtils, uiActions, uiActionsEnhanced
  • kibana-app: (10) charts, kibanaLegacy, urlForwarding, visDefaultEditor, visTypeTable, visTypeVislib, visTypeXy, visualizations, discoverEnhanced, lens
  • kibana-presentation: (8) expressionError, expressionImage, expressionMetric, expressionRepeatImage, expressionRevealImage, expressionShape, presentationUtil, dashboardEnhanced
  • kibana-alerting-services: alerting, ruleRegistry, stackAlerts, triggersActionsUi
  • kibana-stack-management: devTools, indexManagement, snapshotRestore
  • ml-ui: dataVisualizer, fileUpload, ml
  • security-solution: metricsEntities, securitySolution, timelines
  • kibana-gis: mapsEms, maps
  • security-threat-hunting: cases
  • fleet: fleet
  • kibana-core: licensing
  • security-detections-response: lists
  • observability-ui: observability
  • security-asset-management: osquery
  • kibana-reporting-services: reporting

@kobelb
Copy link
Contributor

kobelb commented Aug 20, 2021

👏 👏 👏 👏 👏 👏 👏 👏 - nicely done

@spalger spalger force-pushed the implement/eslint-rule-no-export-all branch 4 times, most recently from 0b3e6f6 to d796ec2 Compare August 23, 2021 17:13
@spalger spalger force-pushed the implement/eslint-rule-no-export-all branch from 175bbf2 to 1d6958e Compare August 23, 2021 20:17
@spalger spalger requested review from a team as code owners September 1, 2021 21:02
@spalger spalger added release_note:skip Skip the PR/issue when compiling release notes Team:Operations Team label for Operations Team v7.16.0 v8.0.0 labels Sep 1, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-operations (Team:Operations)

@spalger spalger enabled auto-merge (squash) September 1, 2021 21:05
@spalger spalger disabled auto-merge September 1, 2021 21:05
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Thanks Spencer. Looks like there are a couple false positives, though. I only reviewed Stack Management's plugins.

src/plugins/dev_tools/public/index.ts Show resolved Hide resolved
@botelastic botelastic bot added Feature:Embedding Embedding content via iFrame Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) Team:Fleet Team label for Observability Data Collection Fleet team labels Sep 1, 2021
@elasticmachine
Copy link
Contributor

Pinging @elastic/fleet (Team:Fleet)

@spalger
Copy link
Contributor Author

spalger commented Sep 1, 2021

@elasticmachine merge upstream

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@spalger spalger merged commit fecdba7 into elastic:master Sep 2, 2021
@spalger spalger deleted the implement/eslint-rule-no-export-all branch September 2, 2021 01:05
@spalger spalger added the auto-backport Deprecated - use backport:version if exact versions are needed label Sep 2, 2021
kibanamachine added a commit to kibanamachine/kibana that referenced this pull request Sep 2, 2021
…09357)

* [eslint] add rule to prevent export* in plugin index files

* deduplicate export names for types/instances with the same name

* attempt to auto-fix duplicate exports too

* capture exported enums too

* enforce no_export_all for core too

* disable rule by default, allow opting-in for help fixing

* update tests

* reduce yarn.lock duplication

* add rule but no fixes

* disable all existing violations

* update api docs with new line numbers

* revert unnecessary changes to yarn.lock which only had drawbacks

* remove unnecessary eslint-disable

* rework codegen to split type exports and use babel to generate valid code

* check for "export types" deeply

* improve test by using fixtures

* add comments to some helper functions

* disable fix for namespace exports including types

* label all eslint-disable comments with related team-specific issue

* ensure that child exports of `export type` are always tracked as types

Co-authored-by: spalger <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>
@kibanamachine
Copy link
Contributor

💚 Backport successful

Status Branch Result
7.x

This backport PR will be merged automatically after passing CI.

kibanamachine added a commit that referenced this pull request Sep 2, 2021
…110928)

* [eslint] add rule to prevent export* in plugin index files

* deduplicate export names for types/instances with the same name

* attempt to auto-fix duplicate exports too

* capture exported enums too

* enforce no_export_all for core too

* disable rule by default, allow opting-in for help fixing

* update tests

* reduce yarn.lock duplication

* add rule but no fixes

* disable all existing violations

* update api docs with new line numbers

* revert unnecessary changes to yarn.lock which only had drawbacks

* remove unnecessary eslint-disable

* rework codegen to split type exports and use babel to generate valid code

* check for "export types" deeply

* improve test by using fixtures

* add comments to some helper functions

* disable fix for namespace exports including types

* label all eslint-disable comments with related team-specific issue

* ensure that child exports of `export type` are always tracked as types

Co-authored-by: spalger <[email protected]>
Co-authored-by: Kibana Machine <[email protected]>

Co-authored-by: Spencer <[email protected]>
Co-authored-by: spalger <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
auto-backport Deprecated - use backport:version if exact versions are needed Feature:Embedding Embedding content via iFrame Feature:ExpressionLanguage Interpreter expression language (aka canvas pipeline) release_note:skip Skip the PR/issue when compiling release notes Team:Fleet Team label for Observability Data Collection Fleet team Team:Operations Team label for Operations Team v7.16.0 v8.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants