Should be possible to force import of "unused" modules in case there are side-effects

[ThatRendle]

TypeScript Version:

1.8.10

Code

MyService.ts

// A self-contained demonstration of the problem follows...
import {ngModule} from './../module.ts';

export class MyService {
}

ngModule.service('myService', MyService); // <- REALLY IMPORTANT SIDE-EFFECT

MyComponent.ts

import {MyService} from './MyService.ts';

class MyComponent {
  constructor(private myService: MyService) { }
  // ...
}

Expected behavior: (I know this sort-of-tree-shaking is "by design", but by the principle of least astonishment, expected behavior is this)

MyComponent.js

var MyService_1 = require('./MyService');
var MyComponent = (function() {
  function MyComponent(myService) {
    this.myService = myService;
  };
  // ...
})();

Actual behavior: MyComponent.js

var MyComponent = (function() {
  function MyComponent(myService) {
    this.myService = myService;
  };
  // ...
})();

And then AngularJS is all "myServiceProvider not found" and crashy.

[RyanCavanaugh]

It's possible:

import './MyService`;

[ThatRendle]

I know I can do that, but I shouldn't have to say

import './MyService';
import {MyService} from './MyService';

all over the place. It's just untidy.

[ThatRendle]

:)

[zpdDG4gta8XKpMCd]

I am too dumb for getting why import {MyService} from './MyService'; would not work, please enlighten.

[mhegazy]

please see https://github.com/Microsoft/TypeScript/wiki/FAQ#why-are-imports-being-elided-in-my-emit

[ThatRendle]

@aleksey-bykov Basically, in Angular code, the class is instantiated by the dependency injection system, so you import MyService but then you don't new it or anything, you just use it as the type for the injected parameter. TypeScript sees that you don't actually use the class except as a type declaration, which has no meaning in the emitted JavaScript, so it sees no reason to include it in the emitted JavaScript. It makes perfect sense, except when the file you are importing from has side-effects; in this case, the registering of the service with an AngularJS module.

[kitsonk]

While I get understand the use case, I can't conceive of a clean syntax that would denote that the import should not be elided... Only except maybe:

import *, { MyService } from './MyService';

In that the compiler would understand that the anonymous * is denoting that the import is being doing for side-effects but not being named. Therefore:

import * from './MyService';
/* is equivalent to */
import './MyService';

The one "edge case" is that if the default export of the module being something that gets erased (is that even possible to export default something that is erasable)?

[aluanhaddad]

@markrendle for what it's worth, this is my pattern for registering services in angular 1.x:

my-service.ts

export class MyService { 
    static $inject = ['$q'];
    constructor(readonly $q: ng.IQService) {}
}

my-other-service.ts

import { MyService } from './my-service';

export class MyOtherService { 
    static $inject = ['MyService'];
    constructor(readonly myService: MyService) {}
}

app.ts

import { module } from 'angular';
import { MyService } from './my-service';
import { MyOtherService } from './my-other-service';

const app = module('app', [])
    .service({
        MyService,
        MyOtherService
    });

export default app;

main.ts

import { bootstrap } from 'angular';
import app from './app';

bootstrap(document.body, [app.name], { strictDi: true });

This ensures the service import will not be elided. It also has the advantage of decoupling your service classes from angular (sure $inject is there but they don't depend on angular).

Edit: Updated example with explicit export from "app.ts"

[ThatRendle]

@aluanhaddad I see what you've done there, but I use ngAnnotate as part of my build process so I don't have to manually maintain $inject properties, and I don't think ngAnnotate would pick up that way of adding services... although I'll give it a try.

This would all go away if my boss would just let me spend 6-9 months rewriting the entire application in Angular 2...

[aluanhaddad]

@markrendle I don't use ngAnnotate so I wouldn't know if that would work but the only reason I write my "services" as classes registered with .service instead of factories registered with .factory is to take advantage of the static inject notation. That said, I only used the above style for my example because it is my preference.

My example was just meant to show that you don't have to register each service in the module where it is defined. I also think that there are compelling reasons to avoid doing so.

Regardless, using a tool like ngAnnotate shouldn't dictate the way in which you structure your code or the order in which you register your services.

[ThatRendle]

Regardless, using a tool like ngAnnotate shouldn't dictate the way in which you structure your code or the order in which you register your services.

@aluanhaddad But it does, because it makes life so much easier that I'm willing to put up with its demands.

[aluanhaddad]

@markrendle But if the import were not elided and the type were imported solely for use in type position in multiple dependent modules, you would end up registering the service multiple times. That just seems unsound.

Anyway, how about exporting a function which registers the service.

export class MyService { 
    static $inject = ['$q'];
    constructor(readonly $q: ng.IQService) {}
}

export default function register(ngModule: angular.IModule) {
    ngModule.service('myService', MyService);
}

[Cliveburr]

I'm going through this problem these days, exactly like @markrendle said. What do you think of adding a flag to force the import, I had thought of something subtle, like the simbol ! already used to define important things in several places .. something like that..

import {MyService} from '!./MyService.ts';

[RyanCavanaugh]

! is already treated specially as part of AMD plugin paths

[Cliveburr]

@RyanCavanaugh Good thinking!

[aluanhaddad]

@Cliveburr @RyanCavanaugh I don't think this use case is valid. In @markrendle's example, if the module import were not elided when the imported entity is used only in type position, the side effect of registering the service would occur an additional time for each such import. It is common for multiple components to depend on the same service.

[RyanCavanaugh]

Are people seriously writing modules with non-idempotent load-time side effects? That seems insane

[aluanhaddad]

Yes they are, and yes it is insane. It works for them precisely because TypeScript elides these imports. Here is a simple reproduction of the issue:

file0.ts

angular.module('app', [])
  .directive('myDirective', function() {
    return {
      scope: {}
    };
  });

file1.ts

angular.module('app')
  .directive('myDirective', function() {
    return {
      scope: {}
    };
  });

And here is a plinkr demonstrating the same: https://plnkr.co/edit/D53fDu1hwhvk5aKIsrJs Edit: fixed paste error

[aluanhaddad]

@RyanCavanaugh to clarify what this does: It registers two identically named directives, each of them specifying an isolate scope. Angular actually registers both directives and the result of writing

<my-directive></my-directive>

is that angular will explode with the error:

Multiple directives [myDirective (module: app), myDirective (module: app)] asking for new/isolated scope on: <my-directive>

This is very bad!

[Cliveburr]

@aluanhaddad Yes, that does happen, I had to do a little helper to prevent it, not the way you showed, but the loading of routes in a SPA navigation, pages where is visited several times in the same session, but however, I believe that this effect is a framework problem or the loader mechanism problem, not the language itself ..

[RyanCavanaugh]

:sob:

[aluanhaddad]

@Cliveburr Of course it is not a language problem. It is also not a loader problem or a framework problem. The problem is incorrect code. If you are having issues like this it is because you have a bug in your code. There are so many ways to avoid this particular problem. Here are two ways:

1. my-directive.ts

   export function myDirective() {
    return { .... };
   }

   app.ts

   import 'angular';
   import { myDirective } from './my-directive';
   
   angular
    .module('app', [])
    .directive('myDirective', myDirective);

2. my-directive.ts

   function myDirective() {
    return { .... };
   }
   export default function register(app : ng.IModule) {
    app.directive('myDirective', myDirective);
   }

   app.ts

   import 'angular';
   import registerMyDirective from './my-directive';
   
   const app = angular.module('app', []);
   
   registerMyDirective(app);

If you follow one of these patterns, you will not ever have this problem.

[ThatRendle]

@RyanCavanaugh wrote:

Are people seriously writing modules with non-idempotent load-time side effects? That seems insane

It's not insane, it's JavaScript. JavaScript modules have inline code that is executed when the module is loaded. It is the responsibility of the module loader to ensure that this only happens once. In my particular case, Webpack does a perfectly good job of this, but the same is true of RequireJS, Node/CommonJS, SystemJS, etc.

@aluanhaddad It is increasingly unclear to me what you think you are contributing to this issue. None of your comments have been constructive or helpful; the Plunkr you offered as a "repro" uses neither import statements nor module loading and is therefore entirely irrelevant in this context; the "workarounds" you have offered are either unusable in many contexts or horribly inelegant and fragile, and unintuitive to boot. Your assertion that "it only works precisely because TypeScript elides these imports" is flat-out wrong: I can and do repeatedly require() and import modules with side-effects throughout my application, and TypeScript does not elide the generated require statements, and it works because every JavaScript module loader in existence guarantees that the source file for a module will only be loaded and executed once.

Your obvious misinformation aside, may I propose that we take it as read that you are not encountering this problem yourself and therefore don't need the feature requested, and, in return, you accept that some people are and do, and that their experience is as valid and important as yours, and move on? Thank you.

[ThatRendle]

All I am asking for here is some kind of directive so I can say to TypeScript "hey, don't tree-shake this import, because I know my code-base better than you". Asserting that there are never going to be cases where tree-shaking should not occur is short-sighted and hubristic. Side-effects exist, are already well-handled in the JavaScript module ecosystem, and clearly in some cases are desirable, so TypeScript needs to recognise that.

Proposals

I can think of two directions from which to come at this. Firstly, from the side of the consumer of an import, such as this syntax (import-bang):

import! {MyService} from './myservice';

The second option is to declare within the module itself that it should always be imported, which would seem to make more sense, since it does not require special knowledge of module internals on the part of the consumer. Could be done with a /// comment or a free-standing string (like 'use strict';) within the module:

/// <ts-disable-elision>
export class Foo { }
angular.module('fubar').service('foo', Foo);

[kitsonk]

Why is non-compliant ES6 syntax better than the following again?

import './MyService';
import {MyService} from './MyService';

Ugly is clearly in the eye of the beholder.

[ThatRendle]

@kitsonk Multiple points:

1. Repetition.
2. Repetition.
3. Unclear at first sight why that is necessary at all, such that another developer might "helpfully" remove the "duplicate".
4. Places the burden of knowledge of internal workings of the module on the consumer rather than the author.
5. I'm not sure what "non-compliant ES6 syntax" means, nor what it has to do with a request for a TypeScript feature.
6. I should clarify that by "ugly" I meant "an ugly workaround" rather than "aesthetically displeasing".

[kitsonk]

5) I'm not sure what "non-compliant ES6 syntax" means, nor what it has to do with a request for a TypeScript feature.

From TypeScript's design goals:

6) Align with current and future ECMAScript proposals. 8) Avoid adding expression-level syntax.

So TypeScript tries to align to current and future ES Syntax, so it is material to any feature request.

The syntax:

import! {MyService} from './myservice';

Would have to be rewritten in all instances to generate a compatible ES Module. The elided import can just be erased, with no re-writes and without introducing expression level syntax.

Doesn't this proposal also suffer from your item 4) which the burden of knowledge is on the consumer?

While the triple slash comment does not add any expression level of syntax that is syntactically meaningful to ES.

Again "ugly" is still in the eye of the beholder. I don't think either of your solutions really improve about item 3) of your list. I don't think import! is clear at all...

I personally am not uncomfortable with the current solution, but if you wanted to really propose introducing some sort syntax to express this, why not propose something that doesn't use cryptic syntax:

import always {MyService} from './myservice';
/* or */
always import {MyService} from './myservice';

I suspect the former would be better (since the ES import syntax is largely backwards anyways).

[ThatRendle]

Placing some form of directive in the module to be forced to load addresses points 1 through 4 (and probably 5). The change happens in a single file, instead of wherever that file is referenced, the import statements in the consuming files are ES6 standard, and the /// directive approach will, as you say, be ignored as a comment by ES6. Furthermore, if an update is made to the module that no longer requires it to be force-loaded, only the module needs to change.

So, the second suggested approach obviously makes more sense.

Can I formally propose some triple-slash-based directive along the lines of:

Proposed Syntax

/// <ts-always-load>
export class StockService { }
angular.module('shop').service('stock', StockService);

Behaviour: When the TypeScript compiler encounters this directive in an imported module file, it will always generate the equivalent require, or leave the import intact, even if the imported members are not used in the currently-compiling file.

Disclaimer: I am not, as you can clearly see, a language designer, so I have no idea how to turn this into a proper formal proposal. Sorry.

[kitsonk]

How do you plan to handle a situation where the developer is actually only importing an interface/class for its shape but expects/wants any side effects to not occur? Not every module that can have side effects is some sort of form of DI. For example, let's say I have a shim library that is exposing an API surface, but I want/need to intelligently load that code in a different way, and not eliding the module causes issues or inefficiencies at run-time?

Also, what effect does this proposal have on the emit of definition files (.d.ts) and how does that behave?

[ThatRendle]

Why would a shim library, or a module that is to be loaded in a non-standard way, have the always-import directive? Can you give a realistic example?

I can't imagine any effects on definition file emission, or changes that would be required in that area.

[kitsonk]

Why would a shim library, or a module that is to be loaded in a non-standard way, have the always-import directive? Can you give a realistic example?

Let's say I have a Promise shim. I want to import the API surface:

import Promise from 'my-promise-shim';

function logPromise(p: Promise<string>) {
    p.then((value) => console.log(value);
}

But at build time, I may or may not want to elide my-promise-shim if I am targeting and environment that supports them natively, but it is likely the shim then would be included in the output, because it will be rewritten as an import and then not elided in the final build, even though I don't want it there.

There are issues already open for conditional compilation and it almost seems like solving this potentially would fall under that, as I think it is hard to properly express a "you must always include this", because that is sometimes not that black or white.

I can't imagine any effects on definition file emission, or changes that would be required in that area.

What happens if I am consuming a built package of JavaScript code that contains modules with side effects and in TypeScript land it is described using a definition file? How would TypeScript know to elide or not elide? Especially when not using UMD definition files, but a global definition with namespaces.

[ThatRendle]

I'm still not clear on why the Promise shim, given its optional nature, would have the always-import directive.

TypeScript doesn't currently elide requires from definition files if the imported symbol is used other than for type declaration, otherwise it does. I am not proposing that the always-import directive be added to .d.ts files.

[clement911]

I've been tripped by this issue as well when using RxJS.

import { Observable } from 'rxjs';
...//omitted code
 get<TResult>(url: string, params: Param[])
    {
        let urlParams = new URLSearchParams();
        this.serializeParamsForQuery(urlParams, params);
        return this.http.get(url, { search: urlParams })
            .map(r => r.json() as TResult);
    }

I couldn't figure out why it didn't work, even though I use the map function. Using import 'rxjs'; fixed the issue but this doesn't feel right. Typescript is trying to be clever but shoots itself in the foot. IMO optimizations like this should only be implemented if they work 100% of the time, otherwise the compile should err on the side of caution and be conservative.

TypeScript design goals:

Emit clean, idiomatic, recognizable JavaScript code. Preserve runtime behavior of all JavaScript code.

[DominicBoettger]

Something new about this functionality?

[aluanhaddad]

@clement911 no it's not being clever, it's removing TypeScript artifacts from the source. Your snippet doesn't even use what you imported. However, if you had used it, RxJS is structured in a very confusing manner such that the map function is not a member of a Observable.prototype. You need to add an additional import

import 'rxjs/add/operator/map';

and even if you use Observable as a value, as in Observable.of(1).map(f), in which case TypeScript does retain the import because, if you import {Observable} from 'rxjs/Observable' map will still throw an error. This is a compile time error in recent versions but the fix is not obvious. However if TypeScript retained the import used only in type position it would not be correct. It would not be removing TypeScript related artifacts from the source.

[aluanhaddad]

Note to the side effecting import above actually mutates Observable.prototype. It's a weird pattern but it's good that it happens explicitly as otherwise using it simply for type annotations would change the emit which would in turn cause imports in other modules to break because the import would be evaluated and cached before its prototype were mutated.

[Cezus]

I read all the replies and I still hope this is not an already given answer to the original question:

What you can do is put the code in MyService.ts in a namespace and create a interface for MyService: IMyService. The only code you 'export' in the MyService.ts is the interface:

import {ngModule} from './../module.ts';

namespace myAwsomeNamespace {
    export interface IMyService{
        myVariable: string;
    }

    class MyService implements myAwsomeNamespace.IMyService {
        public myVariable: string;

        constructor() {
            this.myVariable = 'hello world!'
        }
    }

    ngModule.service('myService', MyService); 
}

import './MyService.ts';

class MyComponent {
  constructor(private myService: myAwsomeNamespace.IMyService) { }
  // ...
}

this let you import/load 'side-effects' like you would nomally do and make the injected service type-safe. The other benefit is this example is that the MyService class is not put on the JavaScript Window object.

Look at the output of the TypeScript transpiler:

define(["require", "exports", "./../module.ts"], function (require, exports, module_ts_1) {
    "use strict";
    var myAwsomeNamespace;
    (function (myAwsomeNamespace) {
        var MyService = (function () {
            function MyService() {
                this.myVariable = 'hello world!';
            }
            return MyService;
        }());
        module_ts_1.ngModule.service('myService', MyService);
    })(myAwsomeNamespace || (myAwsomeNamespace = {}));
});

[ThatRendle]

RxJS is a weird example in that what you're importing is a set of "extension methods" to Observable, i.e. they have no standalone representation like classes or functions.

In my most recent Angular 1.6 code I switched to a different pattern based in John Papa's and Todd Motto's styleguides, which I'll lay out here for posterity.

Each component and related assets is contained in its own folder, along with a module file. A Component class looks something like:

import { IHttpService } from 'angular';
export class LoginFormCmp {
   // @ngInject
  constructor (private $http: IHttpService) { }
  ...
  public static ngName = 'loginForm';
  public static ngComponent = {
    bindings: { ... },
    controller: LoginFormCmp,
    template: require('./loginForm.html')
  }
}

The module file then imports the component class and uses the static members, so the import is not elided:

import * as angular from 'angular';
import { LoginFormCmp } from './loginForm.component.ts';

const ngModule = angular.module('app.loginForm', []);
const ngName = ngModule.name;
ngModule.component(LoginFormCmp.ngName, LoginFormCmp.ngComponent);
export default ngName;

Obviously a service looks something like

export class DogmataService {
  // ...
  public static ngName = 'dogmata';
}

It's actually a nice way of organising code generally, as it encourages more separation and isolation, and smaller components.

But there should still be a way of stopping imports from being elided when there are desired side-effects, because even if it is not recommended practice, it happens all over the place, and things should be built for the world we have, not the one we may wish for.

[Cezus]

In my most recent Angular 1.6 code I switched to a different pattern based in John Papa's and Todd Motto's styleguides, which I'll lay out here for posterity.

Thank you, I didn't know the styleguide from Todd Motto. The way that John Papa explaines the registering of angular modules, services, directives etc. is a bit different.

Where Todd is registering all the angular things beforehand, John Papa registers the angular things as you define them: (link)

/* recommended */

// dashboard.js
angular
    .module('app')
    .controller('DashboardController', DashboardController);

function DashboardController() { }

But there should still be a way of stopping imports from being elided when there are desired side-effects,

When we were developing in JS instead of TS we used the styleguide from John Papa. Since a few months we are developing in TS.

Why I placed the quotes around 'side-effect' is because defining a service and then register it with Angular is in my opinion not really a side-effect. The service is not doing anything until I call it from where it is injected (unless developed otherwise).

What I like about the 'side-effect' method (on-demand referencing) is that you only reference a file when you need it. Like you do with usings in C# or Java. On the other hand, the application structure needs to compensate more in what the on-demand referencing lacks..

[TheWizz]

I've been bitten by this numerous times. To me, it seems it should be the imported module that dictates whether it should always be loaded (for side effects), not the importing module (which has less knowledge about the innards of the imported module). Is there really any case where it makes sense for the importing module to control this behaviour?

-JM

[aiboost]

Hi, all, I vote for this feature.

My proposal is to add some flag "noOptimizeImports" to compiler options (tsconfig.conf, etc.) and not to add new syntax for language.

This is very important to modules which are plugins to something else. For example code:

import {Product} from "./product";

export class MyPlugin {
 ...
}

Product.addPlugin(new MyPlugin());

currently will not be included to the product if I write:

import {Product} from "./product";
import {MyPlugin} from "./myplugin";

console.log("MyPlugin version = " + MyPlugin.version);
Product.run();

I think TypeScript must follow what developer says and do not do none-obvious things.

[DominicBoettger]

@TheWizz Yes, there is a case. If you use a DI system like @molecuel/di :-). I am tagging classes with @injectable to mark them. Just a import is needed to load them into the DI.

I had the Problem that it did not work cause i only had imports for some classes in the main file without using them there. I created a little workaround and do a di.bootstrap() and add the imports as parameters.

[zlitz-cw]

I am running into the same issue in a very valid context. When using decorators on a getter like this:

import { ADecorator } from './a-module';
import { SomeClass } from './some-module';

class MyClass {
  @ADecorator()
  public set myProp(value: SomeClass) {
    //do stuff
  } 
}

Here the TypeScript compiler outputs something like this (when using commonjs modules):

__decorate([
    amodule_1.ADecorator (),
    __metadata("design:type", some_module_1.SomeClass),
    __metadata("design:paramtypes", [some_module_1.SomeClass])
], MyClass .prototype, "myProp", null);

The problem is that since I didn't use the "SomeClass" type (other than for type info) the module does not get imported and the code breaks at runtime.

[vladima]

that last thing looks like a #12888 which should already be fixed in nightly build. Can you please try typescript@next to see if you still see this problem?

[zlitz-cw]

Using version 2.3.0-dev.20170221 did not seem to solve the problem. But my build process/tooling is kind of complex so I'll have to play with it and see what's happening.

For now I am solving the problem by just adding an erroneous usage of the offending classes.

[jods4]

@TheWizz

To me, it seems it should be the imported module that dictates whether it should always be loaded (for side effects), not the importing module (which has less knowledge about the innards of the imported module). Is there really any case where it makes sense for the importing module to control this behaviour?

It was my gut feeling as well: modules should declare if they have side-effects or not, not consumers. And then I met someone who was importing types from a module because he needed the types... but did not want to initialize the module yet. The details why are a little bit convoluted but it comes down to having to load another module with side-effects first (yay for side-effects all over the place).

And now I'm confused as to what is the best solution.

[aluanhaddad]

The best solution is just to write

import 'module-specifier';
import {SomeType} from 'module-specifier';

It is extremely clear as to intent, is an existing ECMAScript import form (one specifically created to import modules for their side-effects) and your loader will understand it immediately. I do not see why the extra line is so unappealing to many.

[TheWizz]

Yes, that's what I do now. It just feels "backwards" that tne IMPORTING module should know it needs to do this in order to use another module. The IMPORTED module should have a better idea about whether it has side effects or not, and should be able to "do the right thing" without being told by a client. I understand there may be some edge cases where this doesn't hold. But in all practivcal cases where I've stumbled over this, it has been entirely clear to me that if I import something frmo a module I *definitely" want that module to be brought in at runtime (and not just "peeked" at at compile time).

-JM

[aiboost]

@aluanhaddad,

This is not only "one more line" you need to write. This is a pitfall you can fall into: TypeScript was designed to avoid mistakes in compile-time, but with current behavior these situations go in run-time. Sometimes go and sometimes don't. Sometimes you need such line, sometimes not. Do you think this is fine?

[jrabbe]

I do acknowledge the issue that you might want to import a module only for the types, and not the side effects. I feel that most cases where you want to import a module for the types, however, you would also want the side effects. And selfishly, especially with AngularJS, those side effects are often necessary to the running of the application.

In our scenario specifically, we are using Webpack together with AngularJS to lazy load controllers, and other injectables (components, directives, filters, and services). If TypeScript elides certain imports our whole dependency tree falls apart. In any case I would want babel to do the tree shaking, not TypeScript.

This makes me raise my hand in supports for @dmiko's proposal to have a noOptimizeImports compiler option that would prevent TypeScript from doing tree-shaking.

Alternatively, either the proposal to allow a module to be annotated that it has side effects as suggested by @markrendle, or the import always 'MyModule'; syntax would work as well.

The suggestions that we either double the imports for modules with side effects, or "use" the imported item, will both cause problems. Both look like mistakes that some good samaritan will undo, resulting in bugs. That much I can say for certain.