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

Update @typescript-eslint to ensure compatibility with TypeScript v3.9 #74091

Merged
merged 35 commits into from
Aug 5, 2020

Conversation

mshustov
Copy link
Contributor

@mshustov mshustov commented Aug 3, 2020

Summary

Eslint stopped working at #73924 so I had to start working on the fix.
This PR update @typescript-eslint version to make it compatible with TypeScript v3.9. Unfortunately, we still have to update @typescript-eslint to v4 (still in alpha) to ensure full compatibility with TypeScript v4, I'd rather make it in a separate PR.

@typescript-eslint 3.0 release notes: https://github.com/typescript-eslint/typescript-eslint/releases/tag/v3.0.0
This PR:

  • updates @typescript-eslint
  • removes outdated rules @typescript-eslint/camelcase @typescript-eslint/class-name-casing in favour of @typescript-eslint/naming-convention
  • fixes and mutes newly introduced naming style errors
  • adds eslint-comments plugin to detect unused eslint-disable, eslint-enable instructions
  • removes unused eslint-disable, eslint-enable instructions. most of them were added before plugins started rewriting eslint rules on per plugin basis
  • adds no-script-url and no-unsanitized/property rules for typescript files to fix no unused eslint-disable rule

Known problems

  • @typescript-eslint/naming-convention doesn't support multiple underscores as a valid prefix/suffix. I filtered out such problem places until the problem is fixed in [naming-convention] Allow multiple underscore typescript-eslint/typescript-eslint#1712
  • @typescript-eslint/naming-convention stated reporting CSS classes name format (for example, componentName__element--modifier, componentName__element__element) For now I muted such errors. We might need to create a naming convention to filter them out.
  • We have tons of rules declared for *.js files only and it's time we aligned them with *.ts files linting rules in a followup @elastic/kibana-operations I can create an issue, who is the right owner for it?
  • eslint task time on CI increased from 29m to 33m.

For maintainers

@mshustov mshustov added chore Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc release_note:skip Skip the PR/issue when compiling release notes v7.10.0 labels Aug 3, 2020
Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

Platform changes LGTM

adds eslint-comments plugin to detect unused eslint-disable, eslint-enable instructions

💯 love it

Copy link
Contributor

@alisonelizabeth alisonelizabeth left a comment

Choose a reason for hiding this comment

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

ES UI changes LGTM

Copy link
Member

@legrego legrego left a comment

Choose a reason for hiding this comment

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

Security changes LGTM

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

ML/Transform changes LGTM.

Copy link
Contributor

@timroes timroes left a comment

Choose a reason for hiding this comment

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

Reviewed code change of Kibana App only, LGTM

Copy link
Member

@kertal kertal left a comment

Choose a reason for hiding this comment

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

Code LGTM, KibanaApp code owner review, since there were just comments added and removed didn't test

Copy link
Member

@dgieselaar dgieselaar left a comment

Choose a reason for hiding this comment

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

LGTM, left suggestions for one or two possible improvements.

Copy link
Contributor

@shahzad31 shahzad31 left a comment

Choose a reason for hiding this comment

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

LGTM , uptime changes !!

Copy link
Contributor

@mikecote mikecote left a comment

Choose a reason for hiding this comment

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

Alerting code LGTM

Copy link
Contributor

@poffdeluxe poffdeluxe left a comment

Choose a reason for hiding this comment

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

Canvas changes look good to me

@kibanamachine
Copy link
Contributor

💚 Build Succeeded

Build metrics

async chunks size

id value diff baseline
crossClusterReplication 462.5KB -10.0B 462.5KB
indexManagement 1.5MB -2.0B 1.5MB
ingestPipelines 579.2KB +10.0B 579.2KB
maps 3.3MB -3.0B 3.3MB
securitySolution 7.3MB +8.0B 7.3MB
total - +3.0B -

page load bundle size

id value diff baseline
data 1.4MB -2.0B 1.4MB
upgradeAssistant 65.3KB +39.0B 65.2KB
total - +37.0B -

History

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

@mshustov mshustov merged commit 88c0631 into elastic:master Aug 5, 2020
@mshustov mshustov deleted the update-ts-lint branch August 5, 2020 15:32
mshustov added a commit to mshustov/kibana that referenced this pull request Aug 5, 2020
elastic#74091)

* bump @typescript-eslint deps

* update rules

* fix errors in pacakges

* fix src/

* fix x-pack

* fix test

* fix typings

* fix examples

* allow _ as prefix and suffix

* roll back prefix and suffix changes

* add eslint-plugin-eslint-comments

* report unused rules

* remove unused eslint comments from tests

* remove unused eslint comments 2nd pass

* remove unused eslint comments from src/

* remove unused comments in x-pack

* use no-script-url and no-unsanitized/property for ts files

* remove unused eslint comments

* eui/href-or-on-click removed when not complained

* no import/* rules for ts files

* cleanup

* remove the unused eslint-disable

* rollback unnecessary changes

* allow underscore prefix & sufix in type name

* update docs

* fix type error in enterprise search plugin mocks

* rename platform hack __coreProvider --> _coreProvider

* rollback space removal in src/core/public/legacy/legacy_service.test.ts

* fix naming convention in APM
# Conflicts:
#	src/legacy/ui/public/new_platform/new_platform.ts
#	x-pack/plugins/index_management/public/application/components/mappings_editor/mappings_editor.tsx
mshustov added a commit that referenced this pull request Aug 5, 2020
…pt v3.9 (#74091) (#74384)

* Update @typescript-eslint to ensure compatibility with TypeScript v3.9 (#74091)

* bump @typescript-eslint deps

* update rules

* fix errors in pacakges

* fix src/

* fix x-pack

* fix test

* fix typings

* fix examples

* allow _ as prefix and suffix

* roll back prefix and suffix changes

* add eslint-plugin-eslint-comments

* report unused rules

* remove unused eslint comments from tests

* remove unused eslint comments 2nd pass

* remove unused eslint comments from src/

* remove unused comments in x-pack

* use no-script-url and no-unsanitized/property for ts files

* remove unused eslint comments

* eui/href-or-on-click removed when not complained

* no import/* rules for ts files

* cleanup

* remove the unused eslint-disable

* rollback unnecessary changes

* allow underscore prefix & sufix in type name

* update docs

* fix type error in enterprise search plugin mocks

* rename platform hack __coreProvider --> _coreProvider

* rollback space removal in src/core/public/legacy/legacy_service.test.ts

* fix naming convention in APM
# Conflicts:
#	src/legacy/ui/public/new_platform/new_platform.ts
#	x-pack/plugins/index_management/public/application/components/mappings_editor/mappings_editor.tsx

* remove unnecessary comment
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backported chore 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:APM All issues that need APM UI Team support Team:Core Core services & architecture: plugins, logging, config, saved objects, http, ES client, i18n, etc Team:Fleet Team label for Observability Data Collection Fleet team v7.10.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.