palantir / tslint

:vertical_traffic_light: An extensible linter for the TypeScript language
http://palantir.github.io/tslint/
Apache License 2.0
5.91k stars 887 forks source link

Support for .tsx files #490

Closed pepaar closed 9 years ago

pepaar commented 9 years ago

Typescipt now support JSX and it introduced .tsx extension for files containing JSX syntax. https://github.com/Microsoft/TypeScript/issues/3203

Do you plan to support .tsx files?

adidahiya commented 9 years ago

JSX support is coming in TS 1.6, and we plan to support it by the time that releases. Right now tslint depends on typescript v1.5.0-beta, the latest npm release.

We do have intentions of more closely following the latest TS syntax & language services API on our master branch (see #474), so look out for that soon!

pepaar commented 9 years ago

Thanks. Just from curiosity, how big is the change to make tslint work for tsx files but without any JSX linting support (let's suppose i will disable linting for JSX part of file)? Or to rephrase, I would like to tslint file regardless its extension.

adidahiya commented 9 years ago

I think it's fairly trivial. tslint rules only walk certain nodes in the syntax tree, so when the TS language services get updated to include JSX syntax nodes, tslint will simply ignore them.

ashwinr commented 9 years ago

@pepaar you can also consider using block comments to disable tslint for all the JSX code blocks until we get first-class support (which should be pretty soon)

pepaar commented 9 years ago

Here is my example:

I have GroupPerson.ts file with:

export default class GroupPerson {

    /** Id */
    id: string; 

    /** DisplayName */
    displayName: string;

}

Then if i run: tslint -f App\Models\Groups\GroupPerson.ts everything works fine as expected.

If the file with same content has different extension, e.g. GroupPerson.tsx or GroupPerson.abc and I run tslint -f App\Models\Groups\GroupPerson.tsx I get following error:

C:\Users\pepaar\AppData\Roaming\npm\node_modules\tslint\bin\tslint-cli.js:41676
            this.limit = this.sourceFile.getFullWidth();
                                        ^
TypeError: Cannot read property 'getFullWidth' of undefined
    at EnableDisableRulesWalker.RuleWalker (C:\Users\pepaar\AppData\Roaming\npm\node_modules\tslint\bin\tslint-cli.js:41676:41)
    at EnableDisableRulesWalker.SkippableTokenAwareRuleWalker (C:\Users\pepaar\AppData\Roaming\npm\node_modules\tslint\bin\tslint-cli.js:42219:20)
    at new EnableDisableRulesWalker (C:\Users\pepaar\AppData\Roaming\npm\node_modules\tslint\bin\tslint-cli.js:42340:20)
    at Linter.lint (C:\Users\pepaar\AppData\Roaming\npm\node_modules\tslint\bin\tslint-cli.js:42609:31)
    at processFile (C:\Users\pepaar\AppData\Roaming\npm\node_modules\tslint\bin\tslint-cli.js:42768:29)
    at Object.<anonymous> (C:\Users\pepaar\AppData\Roaming\npm\node_modules\tslint\bin\tslint-cli.js:42777:5)
    at Module._compile (module.js:460:26)
    at Object.Module._extensions..js (module.js:478:10)
    at Module.load (module.js:355:32)
    at Function.Module._load (module.js:310:12)

Do you know what's the issue here?

adidahiya commented 9 years ago

@pepaar Importing .tsx files is not going to work with any current version of tslint. The compiler is bundled with the tslint binary, so you're locked to that compiler version.

You could build a version of tslint yourself that is able to import .tsx files by adding the compiler option allowNonTsExtensions: true in createCompilerOptions() in utils.ts. However, I just tried this with v1.5.3 of the compiler and many linting rules are still broken (some report false positive lint failures, some throw outright exceptions) because the new syntax tree APIs are not included.

Your best bet is to wait until we are tracking the TypeScript master branch in our repo (#474).

adidahiya commented 9 years ago

Just released v2.5.0-dev.1 with the dist-tag next on npm. Try it out for TSX support!

myitcv commented 9 years ago

@adidahiya - this is great thanks. Consider the following:

/// <reference path="../../tsd.d.ts" />

import * as React from "react";

interface IProps {
   foo: string;
}

class MyComponent extends React.Component<IProps, {}> {
   render(): JSX.Element {
      return <span>{ this.props.foo }</span>;
   }
}

export function BuildMyComponent(): JSX.Element {
   let x: string = "test";
   return <MyComponent foo={ x } />;
}

I get the following whitespace warning:

test.tsx[17, 26]: missing whitespace

Line 17 is the return statement in the exported function.

My gut tells me that foo={ x } is more 'correct' in JSX terms than foo = { x } even though the two are semantically equivalent in TSX terms.

Thoughts?

adidahiya commented 9 years ago

That looks like a bug. I tried to make the whitespace rule skip jsx elements for this exact reason. Looks like I missed some cases.

pepaar commented 9 years ago

Great, nice work!

I found issue with noUnusedVariableRule:

My app.tsx file:

/// <reference path="react.d.ts" />
/// <reference path="react-jsx.d.ts" />

import React = require("react");

interface IProps {
    content: string;
}

class MyComponent extends React.Component<IProps, {}> {
    render() {
        this.alertMe();
        return 
            <div>
                {this.props.content}
                <button onClick={() => this.alertMe()}>AlertMe</button>
            </div>;
    }

    private alertMe() {
        let yo = this.props.content + " YO";
        alert(yo);
    }
}

React.render(<MyComponent content="Hello World"/>, document.body);

Tslint error:

C:\Users\pepaar\AppData\Roaming\npm\node_modules\tslint\build\rules\noUnusedVari
ableRule.js:161
        if (highlights[0].highlightSpans.length <= 1) {
                      ^
TypeError: Cannot read property '0' of undefined
    at NoUnusedVariablesWalker.validateReferencesForVariable (C:\Users\pepaar\Ap
pData\Roaming\npm\node_modules\tslint\build\rules\noUnusedVariableRule.js:161:23
)
    at NoUnusedVariablesWalker.visitMethodDeclaration (C:\Users\pepaar\AppData\R
oaming\npm\node_modules\tslint\build\rules\noUnusedVariableRule.js:154:22)
    at NoUnusedVariablesWalker.SyntaxWalker.visitNode (C:\Users\pepaar\AppData\R
oaming\npm\node_modules\tslint\bin\tslint-cli.js:48349:26)
    at C:\Users\pepaar\AppData\Roaming\npm\node_modules\tslint\bin\tslint-cli.js
:48436:67
    at visitEachNode (C:\Users\pepaar\AppData\Roaming\npm\node_modules\tslint\bi
n\tslint-cli.js:7113:30)
    at Object.forEachChild (C:\Users\pepaar\AppData\Roaming\npm\node_modules\tsl
int\bin\tslint-cli.js:7336:21)
    at NoUnusedVariablesWalker.SyntaxWalker.walkChildren (C:\Users\pepaar\AppDat
a\Roaming\npm\node_modules\tslint\bin\tslint-cli.js:48436:16)
    at NoUnusedVariablesWalker.SyntaxWalker.visitClassDeclaration (C:\Users\pepa
ar\AppData\Roaming\npm\node_modules\tslint\bin\tslint-cli.js:48071:18)
    at NoUnusedVariablesWalker.SyntaxWalker.visitNode (C:\Users\pepaar\AppData\R
oaming\npm\node_modules\tslint\bin\tslint-cli.js:48265:26)
    at C:\Users\pepaar\AppData\Roaming\npm\node_modules\tslint\bin\tslint-cli.js
:48436:67

And root cause is at the line 160 in noUnusedVariableRule.js which returns undefined for next statement:

var highlights = this.languageService.getDocumentHighlights("file.ts", position, ["file.ts"]);

Also interesting thing is if you change the line in app.tsx from:

<button onClick={() => this.alertMe()}>AlertMe</button>

to

<button onClick={this.alertMe}>AlertMe</button>

The error is not present anymore.

Also, error is not present if I remove "private" from alertMe() definition.

adidahiya commented 9 years ago

@myitcv filed #559

@pepaar filed #558

the issues you're seeing are probably real bugs, so please file them separately. thanks!