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

tslint to eslint migration #87644

Merged
merged 53 commits into from Jan 3, 2020
Merged

tslint to eslint migration #87644

merged 53 commits into from Jan 3, 2020

Conversation

jrieken
Copy link
Member

@jrieken jrieken commented Dec 24, 2019

This PR explores to migrate our custom tslint rules to eslint rules and adopts eslint in general.

Rules that we have today and that need migration

  • duplicateImportsRule.ts (comes with eslint)
  • importPatternsRule.ts (this PR)
  • layeringRule.ts (9819da6)
  • noDomGlobalsRule.ts (moved to custom solution)
  • noNewBufferRule.ts (eslint rule, 291ee00)
  • noNlsInStandaloneEditorRule.ts (2ea9132)
  • noNodejsGlobalsRule.ts (moved to custom solution)
  • noStandaloneEditorRule.ts (b05b481)
  • noUnexternalizedStringsRule.ts (a61f38e)
  • translationRemindRule.ts (a3bd604)

Other work packages that are required to migrate away from eslint

  • investigate using https://eslint.org/docs/rules/no-restricted-imports (our needs are too special)
  • find estree-equivalent of import foo = require('...')
  • use ruledir-setting instead of plugin-approach?
  • migrate our existing tslint-config to eslint (using tslint-to-eslint-config result plus tweaks)
  • integrate into build
  • integrate into precommit hook
  • editor integration using our extension

@jrieken jrieken self-assigned this Dec 24, 2019
@jrieken jrieken added this to the January 2020 milestone Dec 24, 2019
@Stanzilla
Copy link

Just curious, what's the reason behind the move?

@mattacosta
Copy link
Contributor

@Stanzilla The TSLint project is winding down and will no longer be maintained in the future. See palantir/tslint#4534.

@bpasero
Copy link
Member

bpasero commented Dec 27, 2019

Pushed 7909462 to move the globals rules (node.js, DOM) into a custom solution that uses the TS API directly. I could not get that to work with a pure tsconfig.json solution, mainly because the options are not fine grained enough to exclude certain DOM types.

Btw there is a eslint rule for "new Buffer": https://github.com/eslint/eslint/blob/master/lib/rules/no-buffer-constructor.js

@kieferrm kieferrm mentioned this pull request Dec 29, 2019
40 tasks
@jrieken
Copy link
Member Author

jrieken commented Dec 30, 2019

@bpasero Won't this rule help you? https://eslint.org/docs/rules/no-restricted-globals.

@jrieken
Copy link
Member Author

jrieken commented Dec 31, 2019

current rule mapping/migration state (tslint <=> eslint)

👌 class-name <=> @typescript-eslint/class-name-casing
👌 curly <=> curly
👌 duplicate-imports <=> no-duplicate-imports
👌 import-patterns <=> code-import-patterns
👌 label-position <=> no-unused-labels (similar)
👌 layering <=> code-layering
👌 no-arg <=> no-caller
👌 no-construct <=> no-new-wrappers
👌 no-debugger <=> no-debugger
👌 no-duplicate-super <=> constructor-super
👌 no-duplicate-switch-case <=> no-duplicate-case
👌 no-eval <=> no-eval
👌 no-new-buffer <=> no-buffer-constructor
👌 no-nls-in-standalone-editor <=> code-no-nls-in-standalone-editor
👌 no-sparse-arrays <=> no-sparse-arrays
👌 no-standalone-editor <=> code-no-standalone-editor
👌 no-string-throw <=> no-throw-literal
👌 no-unexternalized-strings <=> code-no-unexternalized-strings
👌 no-unsafe-finally <=> no-unsafe-finally
👌 no-var-keyword <=> no-var
👌 semicolon <=> @typescript-eslint/semi
👌 translation-remind <=> code-translation-remind
👌 triple-equals <=> eqeqeq
👌 no-redundant-jsdoc <=> jsdoc/no-types (only TS-files, no JS)


  • ✋ 👌no-duplicate-variable <=> no-redeclare
    OFF, fails on enum Foo; namespace Foo;BUT only useful when using var-keyword - which we have disabled via no-var-keyword. let and const are checked by the compiler/runtime and therefore OK to be ignored.
  • ✋ 👌 no-unused-expression <=> no-unused-expressions
    Added custom fork which supports OptionalCallExpression(also see Optional Chaining and Nullish coalescing Operator arrived at Stage 4 eslint/eslint#12642)
  • ✋ 🙅‍♂ no-for-in-array <=> @typescript-eslint/no-for-in-array
    OFF, needs type info. Can be done with some additional effort but I'd say not worth the effort
  • ✋ 🙅‍♂ number-literal-format <=> N/A
    Rule does not exist in eslint, but would be easy to add a custom rule, check that numbers are 0.1 not .1 and 10 not 00010. I leave that for later...
  • ✋ 🙅‍♂ no-restricted-globals <=> no-restricted-globals
    Configured but not known as tslint-rule, so we never really had this rule and @bpasero also added a custom solution for this.

@@ -173,7 +173,7 @@ export class FileLoggerService extends Disposable implements ILoggerService {
getLogger(resource: URI): ILogger {
let logger = this.loggers.get(resource.toString());
if (!logger) {
logger = new BufferLogService, this.logService.getLevel();
logger = new BufferLogService(this.logService.getLevel());
Copy link
Member Author

Choose a reason for hiding this comment

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

fyi @sandy081

Copy link
Member

Choose a reason for hiding this comment

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

Good catch by the linter

@@ -180,7 +180,7 @@ export class TextFileSaveErrorHandler extends Disposable implements ISaveErrorHa
// Show message and keep function to hide in case the file gets saved/reverted
const actions: INotificationActions = { primary: primaryActions, secondary: secondaryActions };
const handle = this.notificationService.notify({ severity: Severity.Error, message, actions });
Event.once(handle.onDidClose)(() => { dispose(primaryActions), dispose(secondaryActions); });
Event.once(handle.onDidClose)(() => { dispose(primaryActions); dispose(secondaryActions); });
Copy link
Member Author

Choose a reason for hiding this comment

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

fyi @mjbvz

@jrieken jrieken added the engineering VS Code - Build / issue tracking / etc. label Jan 3, 2020
@jrieken jrieken merged commit 16a84b4 into master Jan 3, 2020
@jrieken jrieken deleted the joh/eslint-rules branch January 3, 2020 09:56
@bpasero
Copy link
Member

bpasero commented Jan 4, 2020

I think no-restricted-globals is still valid to have (and not covered by my solution), since it exists in ESLint, I can bring it back.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
engineering VS Code - Build / issue tracking / etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants