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

Removing unused imports "feature" #4717

Closed
nullptr128 opened this issue Sep 9, 2015 · 7 comments
Closed

Removing unused imports "feature" #4717

nullptr128 opened this issue Sep 9, 2015 · 7 comments
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead Canonical This issue contains a lengthy and complete description of a particular problem, solution, or design

Comments

@nullptr128
Copy link

Hello!

I recently stumbled upon a bit weird error which made me lost a bit of time. I am using TypeScript in client side SPA application with browserify (TypeScript module setting is "commonjs"). I found out that compiler has weird feature of silently removing "dead" imports that are not used within module code.

This should not work this way as it breaks code, because some libraries actually just extend other ones, with the example being Knockout Punches. This leads me to make a stupid workaround just to "cheat" TypeScript compiler. This behaviour shuold be documented and it should be configurable.

Code: http://pastebin.com/0dSyzieq

Removing fakeArray causes line 25 (ko.punches.enableAll()) to break in runtime because TS skips this import and knockout.punches library is not extending base Knockout at all.

@danquirk
Copy link
Member

danquirk commented Sep 9, 2015

Yes, this is a common source of confusion, but it is intentional.

From https://github.com/Microsoft/TypeScript-Handbook/blob/master/pages/Namespaces%20and%20Modules.md

The compiler detects whether each module is used in the emitted JavaScript. If a module identifier is only ever used in type annotations and never as an expression then no require call is emitted for that module. This culling of unused references is a good performance optimization, and also allows for optional loading of those modules.

As you've noticed, you need to use the imported module in a value position to trigger the emit of import statements. Alternatively, you can use the ///< amd-dependency > directive to manually specify imports that should be emitted. Last but not least, if you use ES6 module syntax this won't happen and imports will not be elided.

@danquirk danquirk closed this as completed Sep 9, 2015
@danquirk danquirk added the By Design Deprecated - use "Working as Intended" or "Design Limitation" instead label Sep 9, 2015
@nullptr128
Copy link
Author

Okay thank you. I wish it would be optional feature to switch off in configuration, because this is more often than not going to make problems. A lot of library use "extend some type" pattern, like KnockoutJS or jQuery.

@danquirk
Copy link
Member

danquirk commented Sep 9, 2015

Didn't realize my comment markdown got messed so just wanted to note I edited it to be clearer about using to force the import emit on a per file basis if that's more convenient for you than dummy variables.

@nullptr128
Copy link
Author

Also I am not using AMD modules (I am using browserify) so I will have to stick with dummy var. Also, I believe that unused imports behaviour is a task for IDE, not compiler itself. Thanks for your time anyway! :)

@mhegazy
Copy link
Contributor

mhegazy commented Sep 10, 2015

to ensure your import is emitted in the output, use import "module"; syntax. this form of imports will not be elided.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 10, 2015

so you can do something like:

import "myModule"; // ensure side effects are maintained regardless of the use of the module

import { myInterface } from "myModule"; // used only in type position, and will be elided along with  the import
var v: myInterface;

@bbottema
Copy link

I took this approach and turned it into a generic solution using the preprocessor loader:

{
    "line": false,
    "file": true,
    "callbacks": {
    "fileName": "all",
    "scope": "line",
    "callback": "(function fixTs(line, fileName, lineNumber) { return line.replace(/^(import.*(require\\(.*?\\)))/g, '$2;$1'); })"
    }]
}

As a proof of concept, this regex version is very limited it only support the following format:

import ClassA = require('ClassA');
// becomes
require('ClassA');import ClassA = require('ClassA');

But it works for me. Similarly, I'm adding the require shim:

{
    "fileName": "all",
    "scope": "source",
    "callback": "(function fixTs(source, fileName) { return 'declare var require: any;' + source; })"
}

I made a sample project with this solution.

@microsoft microsoft locked and limited conversation to collaborators Jun 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead Canonical This issue contains a lengthy and complete description of a particular problem, solution, or design
Projects
None yet
Development

No branches or pull requests

4 participants