Organize imports removes react imports in js projects that don't configure "jsx"

[mjbvz]

From microsoft/vscode#47287

TypeScript Version: 2.9.0-dev.20180409

Search Terms:

• organize imports
• jsx

Code
For a simple js project that does not configure "jsx:

jsconfig.json:

{
    "compilerOptions": {
        "module": "commonjs",
        "target": "es2016",
        "experimentalDecorators": true
    },
    "exclude": [
        "node_modules",
        "**/node_modules/*"
    ]
}

index.js:

import * as React from 'react'

const Button = () => <button />

2. Run organize imports

Problem:
Typescript (correctly from its perspective) removes the React import as React is not referenced anywhere. However this is very confusing for users.

Proposal:
Some ideas:

• Special case React imports for files that use jsx even when you don't have "jsx" configured

• Default "jsx" on for jsconfig projects (the jsconfigs that vscode generates for users already do this). Allow the config to turn if off it a user really needs to

• Show an editor suggestion on jsx in js files that are part of a jsconfig project that does not configure "jsx"

[amcasey]

I believe we already have heuristics for this. Sounds like you're just asking us to drop the check for the compiler option?

https://github.com/Microsoft/TypeScript/blob/1df79970144489924daa2fa1ef056d847488a8a5/src/services/organizeImports.ts#L95

[amcasey]

Confirmed that the import is correctly retained if jsx is set in the compiler options.

[amcasey]

@mhegazy The perf cost of removing the check looks negligible but I don't know enough about JSX to understand the correctness implications.

[amcasey]

I assume the worst case is that we might never remove such imports, which seems acceptable.

[mhegazy]

I think removing the check for the compiler option is the safer choice. we have a problem that it will be tagged as unused and now shown as grey.. but i guess that is a different bug.

[amcasey]

It is rather annoying that we can gray it out accurately but not organize/remove it accurately.

[amcasey]

@mhegazy As I recall, we did it that way because we didn't want to trigger a full bind for the refactoring?

[amcasey]

@mhegazy I've created a PR to have organizeImports stop deleting it. What do you want to do about not removing grayed out imports? File a Future bug?

[mhegazy]

Yes please.

[amcasey]

As we discussed offline, we can improve the heuristic now so there's no reason to file a separate bug.