Fix latest tslint errors by saschanaz · Pull Request #13006 · microsoft/TypeScript

saschanaz · Dec 18, 2016

Except this type of errors:

export function getNavigationTree(sourceFile: SourceFile): NavigationTree {
    curSourceFile = sourceFile;
    const result = convertToTree(rootNavigationBarNode(sourceFile));
    curSourceFile = undefined;
    return result;
}
let curSourceFile: SourceFile;
// Identifier 'curSourceFile' is never reassigned; use 'const' instead of 'let'.

let response: protocol.Response;
try {
    ({ response } = this.executeCommand(msg));
}
//  Identifier 'response' is never reassigned; use 'const' instead of 'let'.

I think they are tslint bugs rather than actual code problem. (palantir/tslint#1898)

BTW I feel fairly painful to create a new constant for every for-loop, I hope there is a better way to keep high performance...

saschanaz · Dec 18, 2016

A benchmark shows for-of loop is much slower on Chrome (and thus node.js), does this mean we shouldn't use for-of?

ghost · Dec 18, 2016

for-of gets emitted as a for loop, since we use an ES5 target. (So maybe some of those manually-written for loops could be upgraded.)
Also, this would be a good time to delete our own preferConstRule.ts since tslint has that now.

saschanaz · Dec 19, 2016

Already done the upgrade for obvious cases but many of them are using indices so I couldn't do it for them.

… on #13006 to upgrade.

mhegazy

I would inline the collection.length check instead of adding a new variable for it. There is not much perf difference on modern engines anyways.

mhegazy · Dec 19, 2016

            const result: Signature[] = [];
-            for (let i = 0, len = symbol.declarations.length; i < len; i++) {
+            const len = symbol.declarations.length;
+            for (let i = 0; i < len; i++) {


switch to for..of?

Inner lines of code use i for some purpose. It seems I can still switch to for-of in this case but should I do it?

mhegazy · Dec 19, 2016

-                for (let i = isJSConstructSignature ? 1 : 0, n = declaration.parameters.length; i < n; i++) {
+                const n = declaration.parameters.length;
+                for (let i = isJSConstructSignature ? 1 : 0; i < n; i++) {
                    const param = declaration.parameters[i];


for (let i = isJSConstructSignature ? 1 : 0; i < declaration.parameters.length; i++) {

mhegazy · Dec 19, 2016

@saschanaz i have merged a change to move tslint back to 4.0.0-dev.3 (#13028). can you update it with your PR to next again.

… lintfix

saschanaz · Dec 19, 2016

It still won't pass because of some bugs, which should be fixed on tslint side.

mhegazy · Dec 19, 2016

It still won't pass because of some bugs, which should be fixed on tslint side.

do you have a list of the issues?

ghost · Dec 19, 2016

@mhegazy See first post and palantir/tslint#1898. I actually fixed one of them yesterday. The other one is also a legitimate issue, but could be fixed on our end by moving the declaration ( let curSourceFile: SourceFile;) to before the function.

saschanaz · Dec 19, 2016

@Andy-MS I'm still seeing destructuring issue on Travis, should I do something to get the merged fix?

ghost · Dec 19, 2016

tslint hasn't published a new version yet, so there's not much to do but wait.

vladima · Dec 19, 2016

I'd prefer not to change our code to workaround bugs in the toolchain hence palantir/tslint#1908

mhegazy · Dec 21, 2016

@nchen63 any news about palantir/tslint#1908?

nchen63 · Dec 23, 2016

it's merged and included in a new release, v4.2.0

saschanaz · Dec 23, 2016

It seems tslint@next is still giving tslint@4.1.0-dev.0.

nchen63 · Dec 23, 2016

should be fixed now

saschanaz added 3 commits December 10, 2016 17:53

Merge remote-tracking branch 'refs/remotes/Microsoft/master'

3202d59

Merge branch 'master' of https://github.com/Microsoft/TypeScript

603ba89

fix linting errors

27a60e4

msftclas added the cla-already-signed label Dec 18, 2016

This was referenced Dec 18, 2016

Format false branch of ternary operation #5905

Closed

tslint@next does not recognize some reassignation palantir/tslint#1898

Closed

remove preferConstRule.ts

27ed5b8

HerringtonDarkholme mentioned this pull request Dec 19, 2016

Fix #1809, introduce non primitive object type #12501

Merged

saschanaz closed this Dec 19, 2016

saschanaz reopened this Dec 19, 2016

msftclas added the cla-already-signed label Dec 19, 2016

ghost pushed a commit that referenced this pull request Dec 19, 2016

Fix tslint version. 4.1.0-dev.0 introduced some bugs, so we must wait…

ef14c08

… on #13006 to upgrade.

ghost mentioned this pull request Dec 19, 2016

Fix tslint version. 4.1.0-dev.0 introduced some bugs, so we must wait on #13006 to upgrade. #13025

Closed

ghost pushed a commit that referenced this pull request Dec 19, 2016

Fix tslint version. 4.1.0-dev.0 introduced some bugs, so we must wait…

6a578f8

… on #13006 to upgrade.

mhegazy reviewed Dec 19, 2016

View reviewed changes

saschanaz added 2 commits December 20, 2016 03:12

inline length

2a941a7

needless newline

1f79741

mhegazy approved these changes Dec 19, 2016

View reviewed changes

saschanaz added 2 commits December 20, 2016 03:46

Merge branch 'master' of https://github.com/Microsoft/TypeScript into…

1a6e43d

… lintfix

next again

67e1fd8

saschanaz closed this Dec 23, 2016

saschanaz reopened this Dec 23, 2016

msftclas added the cla-already-signed label Dec 23, 2016

saschanaz closed this Dec 23, 2016

saschanaz reopened this Dec 23, 2016

msftclas added the cla-already-signed label Dec 23, 2016

saschanaz closed this Dec 23, 2016

saschanaz reopened this Dec 23, 2016

msftclas added the cla-already-signed label Dec 23, 2016

saschanaz closed this Dec 23, 2016

saschanaz reopened this Dec 23, 2016

msftclas added the cla-already-signed label Dec 23, 2016

mhegazy merged commit e4b81d0 into microsoft:master Dec 26, 2016

microsoft locked and limited conversation to collaborators Jun 19, 2018

Search code, repositories, users, issues, pull requests...

Conversation

saschanaz commented Dec 18, 2016 • edited by DanielRosenwasser Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saschanaz commented Dec 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Dec 18, 2016

Uh oh!

saschanaz commented Dec 19, 2016

Uh oh!

mhegazy left a comment

Choose a reason for hiding this comment

Uh oh!

mhegazy Dec 19, 2016

Choose a reason for hiding this comment

Uh oh!

saschanaz Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mhegazy Dec 19, 2016

Choose a reason for hiding this comment

Uh oh!

mhegazy commented Dec 19, 2016

Uh oh!

saschanaz commented Dec 19, 2016

Uh oh!

mhegazy commented Dec 19, 2016

Uh oh!

ghost commented Dec 19, 2016 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

saschanaz commented Dec 19, 2016

Uh oh!

ghost commented Dec 19, 2016

Uh oh!

vladima commented Dec 19, 2016

Uh oh!

mhegazy commented Dec 21, 2016

Uh oh!

nchen63 commented Dec 23, 2016

Uh oh!

saschanaz commented Dec 23, 2016

Uh oh!

nchen63 commented Dec 23, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

saschanaz commented Dec 18, 2016 •

edited by DanielRosenwasser

Loading

saschanaz commented Dec 18, 2016 •

edited

Loading

saschanaz Dec 19, 2016 •

edited

Loading

ghost commented Dec 19, 2016 •

edited by ghost

Loading