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

define parameter not generated for a referenced AND used external module class. #2038

Closed
rcollette opened this issue Feb 15, 2015 · 10 comments
Closed
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead

Comments

@rcollette
Copy link

I am using AngularJs and injecting a service, which is a TypeScript class in an external module, into a controller which is also a TypeScript class. Though I am specifically making a call to a service method, the TypeScript compiler will not include the external module in the define function unless I have a JavaScript call to moduleVariable;

There is no reason, if you are calling an instance method of an imported TS class, that the associated import statement should not be processed in the compiled code.

Service TS Definition

export interface IPurchaseOrderDto {
    id: number;
    dateCreated: Date;
    description: string;
}

export interface IPurchaseOrderDtoPageable extends IPageable<IPurchaseOrderDto> {
}

export class PurchaseOrdersService {
    private _serviceBase: string;
    private _purchaseOrdersResource: any;

    constructor(private $resource: ng.resource.IResourceService, private $location: ng.ILocationService) {
        // todo: The service base would be set from a global configuration/service/singleton normally.
        this._serviceBase = "http://private-a994d-testapi780.apiary-mock.com/";
        var serviceTemplate: string = this._serviceBase + "PurchaseOrders/:purchaseOrderId";
        this._purchaseOrdersResource = this.$resource<any>(serviceTemplate, { purchaseOrderId: "@id" });
    }

    public fetchAll(count?: number, pageNumber?: number): IPurchaseOrderDtoPageable {
        // we're doing get instead of query because we are returning the total page count in addition to the records
        // so the response is considered a single object.
        return this._purchaseOrdersResource.get({ count, pageNumber });
    }

    public fetch(id: number): IPurchaseOrderDto {
        return this._purchaseOrdersResource.get({ id: id });
    }
}
angular.module("app").service("purchaseOrderService", PurchaseOrdersService);  

Controller TS Definition

import poService = require("dataAccessServices/PurchaseOrder.service");
class PurchaseOrdersController implements IFeatureController {
    public title: string = "Purchase Orders";
    public purchaseOrders: poService.IPurchaseOrderDtoPageable;

    constructor($scope: any, private purchaseOrderService: poService.PurchaseOrdersService) {
        this.purchaseOrders = this.purchaseOrderService.fetchAll(10, 1); //Same result if not using "this"
    }
}
angular.module("app").controller("PurchaseOrdersController", PurchaseOrdersController);

Generated Controller JS (define is missing dataAccessServices/PurchaseOrder.service)

define(["require", "exports"], function (require, exports) {
    var PurchaseOrdersController = (function () {
        function PurchaseOrdersController($scope, purchaseOrderService) {
            this.purchaseOrderService = purchaseOrderService;
            this.title = "Purchase Orders";
            this.purchaseOrders = this.purchaseOrderService.fetchAll(10, 1);
        }
        PurchaseOrdersController.$inject = ["$scope", "purchaseOrderService"];
        return PurchaseOrdersController;
    })();
    angular.module("app").controller("PurchaseOrdersController", PurchaseOrdersController);
});

//# sourceMappingURL=../../modules/PurchaseOrder/PurchaseOrders.controller.js.map

"Fixed" Controller (TSLint says "expected assignment or function call")

import poService = require("dataAccessServices/PurchaseOrder.service");
poService; // necessary to cause define function to generated by TypeScript compiler
class PurchaseOrdersController implements IFeatureController {
    public title: string = "Purchase Orders";
    public purchaseOrders: poService.IPurchaseOrderDtoPageable;

    constructor($scope: any, private purchaseOrderService: poService.PurchaseOrdersService) {
        this.purchaseOrders = this.purchaseOrderService.fetchAll(10, 1);
    }
}
angular.module("app").controller("PurchaseOrdersController", PurchaseOrdersController);

"Fixed" Controller JS

define(["require", "exports", "dataAccessServices/PurchaseOrder.service"], function (require, exports, poService) {
    poService; // necessary to cause define parameter to be generated by TypeScript compiler
    var PurchaseOrdersController = (function () {
        function PurchaseOrdersController($scope, purchaseOrderService) {
            this.purchaseOrderService = purchaseOrderService;
            this.title = "Purchase Orders";
            this.purchaseOrders = this.purchaseOrderService.fetchAll(10, 1);
        }
        PurchaseOrdersController.$inject = ["$scope", "purchaseOrderService"];
        return PurchaseOrdersController;
    })();
    angular.module("app").controller("PurchaseOrdersController", PurchaseOrdersController);
});

//# sourceMappingURL=../../modules/PurchaseOrder/PurchaseOrders.controller.js.map
@danquirk
Copy link
Member

There is no reason, if you are calling an instance method of an imported TS class, that the associated import statement should not be processed in the compiled code.

The problem here is that you aren't calling an instance method of an imported instance. From the type system's perspective all you have done is used the type of an imported class. The compiler has an optimization to not emit imports for modules that are only used for type information (since then they're unneeded at runtime and would only cause a performance impact). Obviously here that's biting you (you are not the first ex #596). Unfortunately the compiler is unaware of the Angular magic that causes this module to actually get loaded because of its use in this parameter position. Your options are the workaround you found or to use the amd-dependency directive noted in #596 which would avoid the TSLint errors.

@danquirk danquirk added the By Design Deprecated - use "Working as Intended" or "Design Limitation" instead label Feb 16, 2015
@rcollette
Copy link
Author

@danquirk - I have declared a parameter of an imported type and then USED the parameter of that type with the instance method call this.purchaseOrderService.fetchAll(10, 1);.

I can see optimizing away type information that is not necessary at run time but in this case, clearly the module needs to be imported for it to function.

Are you absolutely sure it is not possible for a compiler to determine if a call to an instance method of an imported type has been made and if so, load the module?

Why so quick to close the issue without discussion?

@mhegazy
Copy link
Contributor

mhegazy commented Feb 16, 2015

A solution for this problem is the new ES6-style import "mod"; this should be coming in shortlly (#1983)

@rcollette
Copy link
Author

@mhegazy - Thank you for replying with a solution. I think I documented pretty well a valid scenario where a define() call must occur. Given that type checking is already occurring on my code, it wouldn't seem unreasonable, that it would impose extra compile time overhead, for the compiler to track instance calls and appropriately generate define() calls.

I look forward to seeing this feature added.

@mhegazy
Copy link
Contributor

mhegazy commented Feb 16, 2015

I do not see how the compiler would do that. remember types are not nominal, they are structural in TypeScript. So we have no way of knowing that this specific type is something that requires the module to be loaded, or it is just a structural type that you gave it a name and put it in that module because you do not want to redefine it.

The flip side is if we add an import to one of these type-only modules we will be requiring a file that does not exist at run time.

@rcollette
Copy link
Author

@mhegazy - I appreciate you time on this. It sounds like you are saying that the module that I am referencing could/might only contain a type definition, similar to a .d.ts file, and therefore it assumes that it should not create the appropriate define.

Where this is difficult to follow, is the fact that I am calling a method on a type, which not only has a type definition (structure), but there is actual code in that module that the compiler must actually process/build and output.

In my layman mind, if the compiler actually builds and outputs type A in external module X then the compiler should be tracking that type A is not just a structure but an actual class. If Type B uses an instance of Type A in external module X, then the fact that code was generated and written for X would serve as the indicator that external module X must have a define created for it when imported.

More confusing is the fact that calling importVar;, with no reference to anything inside the module, would cause the define to be created. It would seem that the optimization should be applied in that case.

Perhaps I am expecting too much in trying to compare C# compilation to JavaScript compilation. There's probably a good reason why the compiler does not make the same assumptions that I do, I just don't know what it is.

This is probably a good topic for a blog article.

@RyanCavanaugh
Copy link
Member

The compiler has only one rule for determining whether or not to emit a module import: was the imported name used in a value position. This encodes the assumption that module imports don't have side effects. Anything in the type system has no effect on that.

We have an important scenario here where you get the type information from an external module but don't actually import it 100% of the time, which is what it "looks like" to the compiler.

@QueueHammer
Copy link

@danquirk It's an arbitrary and dangerous thing to have the TypeScript engine take on the responsibility of optimizing the import statements. There are many patterns that TypeScript does not yet capture from JavaScript. One example is how it does not address that this could have a type #229 .

Import is used to load dependencies before the current code is loaded and may not be referenced directly. Angular dependencies are one example, while jQuery plugins are another. Any library that extends a base object and is not referenced directly are affected by "feature". By deciding to arbitrarly not included an import based on local static analysis you are imposing a C style compiler pattern over the explicit intention of the developer who wrote the import statement. The expectation of writing import is that it will be included as a dependency and available to the local scope of the module. Any other action is a side effect to the natural expectation of choosing to write import inside a module.

It is very possible that in this case the TypeScript compiler could do less, and accomplish more.

@danquirk
Copy link
Member

It is not arbitrary. It's entirely reasonable for someone to import a module only for type information and not want to bear the cost of loading code that won't be used, or the module may only be type information and there actually is no code to load at all (and attempting to load it will fail at runtime, as @mhegazy described).

Yes the compiler could choose to always emit imports and this case would work correctly. Other cases would then work incorrectly. There are always trade offs. We want this scenario to work and we do/will have multiple ways to do that now (amd-dependency, ES6 style import "mod").

@QueueHammer
Copy link

It's arbitrary because there is a way to load a module for its type information and have it added to the scope of TypeScript "intellisense" /// <reference path="justInterfaces"/>. Here the contextual information of the file is loaded but the actual file is not. Not requiring someone to "bear the cost" of loading it at runtime.

In my example the expectation is that the file will be loaded, as the file has compilable contents, hence the need for them as a dependency. This is the existing usage, pattern, and expectation of the CommonJS and AMD (Require.js) implementation in node and client side JavaScript libraries.

Your premise is that the compiler needs to optimize because someone could be linking to something that has no output. This is exclusively a TypeScript behavior, and linking to files for contextual information is already addressed in the spec (see ///<reference...) so there is no need for import to try to accommodate this as well. The pattern worked correctly in every case that has come before TypeScript. To modify an established expectation built around an existing pattern when a language specific work around already exists is reckless.

The issue in your last paragraph only occurs because import in TypeScript tries to load dependencies and do the same thing as ///<reference.... Being in the context of the reasons previously stated I added it here as a bug and not a new feature request. Again if the goal of the project is to add to JavaScript then it is a good idea to address the existing body of practices and patterns before "improving" on them.

(original bug #2132)

@microsoft microsoft locked and limited conversation to collaborators Jun 18, 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
Projects
None yet
Development

No branches or pull requests

5 participants