JSX does not mark React import as used

[dallonf]

For input:

import * as React from 'react';
import * as ReactDOM from 'react-dom';
import * as config from 'config';

function render(id: string) {
  ReactDOM.render(
    <AppContextWrapper>
      <AssetPage id={id} />
    </AppContextWrapper>,
    document.getElementById('app')
  );
}

render(config.asset);

with the no-unused-variable rule enabled, throws the warning: unused variable: 'React'. Since JSX compiles to React.createElement, this is a false positive.

[dallonf]

Oops, just noticed #589 was a closed ticket. I'm not sure I like the suggested solutions though (either manually exclude React from this rule in every single JSX file, or make React a global variable)

[myitcv]

@dallonf - agree that turning off the rule for every time you import React is cumbersome and defeats the point of a linter.

The second suggestion however is not to make React a global variable; rather it is to use the global typings definition.

However, you need to be slightly careful. Because, and this is the case for us (I work with @priyatham-aps), we rely on the import of React in order to ensure modules are loaded when required (we use jspm and SystemJS). Hence our files now look like this:

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

// the above d.ts contains a reference to the global React definition

// this next import ensures that SystemJS loads the module if it hasn't already been loaded
import "react"; 

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

This satisfies the linter, however there is one dangerous side effect: there could be a .tsx file in which we forget to import "react". If we do, we won't catch it at compile time, but we might still get bitten at runtime (depending on the load order of files).

So I'm minded to agree that either:

• this rule should be modified to have special knowledge of React as an import
• a similar rule is created that has special knowledge of React as an import

@adidahiya - thoughts?

[adidahiya]

It seems like giving no-unused-variable special knowledge of React would get pretty messy -- what if you're using the compiler option --jsx=preserve? Then you can't assume that React is used unless TSLint also knows about your compiler rules.

Another solution would be to add support for "excludes" to no-unused-variable (similar to #522)? But it's unclear if there are truly valid use cases for this besides the React variable...

[myitcv]

what if you're using the compiler option --jsx=preserve

Hmm, I don't think it actually matters what mode we are in: React will always be used, and therefore a reference to it will be required (and the module will need to have been loaded, cf. SystemJS). If you have --jsx=preserve then the Babel (or other) transpilation will ultimately rewrite to React.XYZ albeit later in the build step.

I think we're actually on safe ground making an exception under the following conditions:

• we are linting a .tsx file
• the exception is exactly as follows:

import * as React from "react";

Thoughts?

[myitcv]

Sorry, I ignored your suggestion:

Another solution would be to add support for "excludes" to no-unused-variable (similar to #522)? But it's unclear if there are truly valid use cases for this besides the React variable...

Agreed, not keen on this - we only have a React use case right now, and this use-case is very tightly coupled to decisions that have been made to support .tsx in the TypeScript compiler.

[Pajn]

If there are jsx syntax in a tsx file React is used so the warning is erroneous. As the walker supports tsx it's already implicitly React specific so I would say the cleanest solution is to consider React used if the walker visits at least one jsx element.

[adidahiya]

If we go with that implementation ("if the no-unused-variable walker visits at least one JSX element, consider React to be a used value in that file") we would have to add the caveat in the docs that we now have a false negative in the case where you erroneously import React but are actually using another JSX-compatible framework (like Mithril).

... perhaps just to be safe we should add a "react" option to the no-unused-variable rule which enables this functionality.

[myitcv]

@adidahiya - great point, I hadn't considered other such frameworks at all.

... perhaps just to be safe we should add a "react" option to the no-unused-variable rule which enables this functionality.

And just to confirm, by "enables this functionality" are you specifically referring to these two conditions?

• we are linting a .tsx file
• the unused variable is defined exactly as follows:

import * as React from "react";

[adidahiya]

Yep, sorry if that wasn't clear -- "enable this functionality" means to fix the false positive we currently report where usage of JSX syntax (only possible in .tsx files) signifies an implicit usage of the React variable.

[myitcv]

Sorry, it's probably my misunderstanding!

I'm just looking to confirm that, under your proposal, with a config that contains:

  "no-unused-variable": [true, "react"],

the following will not report an error:

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

import * as React from "react";

let x = <span></span>;
console.log(x);

where it currently does:

app/example.tsx[3, 13]: unused variable: 'React'

Reason being the use-case I described earlier

[adidahiya]

@myitcv yep, we are in sync, you understand my proposal correctly

[adidahiya]

fixed as of v3.0.0-dev.2

[myitcv]

FYI - I'm seeing some issues with this rule in our code base... for some reason we're not getting 'visit' callbacks for every JSX element. I'm investigating.

[adidahiya]

@myitcv I think you need to also visit JsxSelfClosingElements. that's what I do in the whitespace rule to skip JSX. should be a simple fix

[adidahiya]

@myitcv fixed here: #776

[donaldpipowitch]

Should this work?

{
  "rules": {
    "no-unused-variable":  [
      true,
      "react"
    ]
  }
}

It still throws here:

import React from 'react';
import { render } from 'react-dom';
import { Router, Route, IndexRedirect, hashHistory } from 'react-router';
import Page from './page/component';
import GistsContainer from './gists/container';
import RepositoriesContainer from './repositories/container';

const DEFAULT_USER = 'octocat';

render(
  <Router history={hashHistory}>
    <Route path="/">
      <IndexRedirect to={`/users/${DEFAULT_USER}/gists`} />
      <Route path="users/:user" component={Page}>
        <Route path="gists" component={GistsContainer} />
        <Route path="repositories" component={RepositoriesContainer} />
      </Route>
    </Route>
  </Router>,
  document.getElementById('app')
);

I use "tslint": "^3.7.1",.

[jkillian]

@donaldpipowitch It should, but it's currently a bug: #893

[donaldpipowitch]

Okay, thank you. I'll follow the other bug.