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

Imports left out of module dependencies when not directly assigned or assigned to. #2132

Closed
QueueHammer opened this issue Feb 24, 2015 · 4 comments
Labels
By Design Deprecated - use "Working as Intended" or "Design Limitation" instead Duplicate An existing issue was already created

Comments

@QueueHammer
Copy link

In the following case a module defined in TypeScript will not be included in the required modules of the module.

import something = require('Something');

angular.module('app')
.controller('somethingController', [
  'definedInSomething', function (definedInSomething: something.myInterfaceOrType) {
    definedInSomething; //Not defined, and not reached because 'definedInSomething' was not included
  }
]);

The module that is generated is:

define(["require", "exports"], function(require, exports) {

Instead of:

define(["require", "exports", 'Something'], function(require, exports, Something) {

Assigning something to another value or accessing the properties of something will cause it to be added to the module signature.

In any case the values defined in the import x = require('y') should be added to the module dependencies. The author has explicitly added the reference and as such has an expectation that it will be included in the dependencies of the current module.

@danquirk
Copy link
Member

This is by design, see #2038 and #596 for some discussion on the same sort of issue and available workarounds. #540 tracks specing this behavior.

@danquirk danquirk added By Design Deprecated - use "Working as Intended" or "Design Limitation" instead Duplicate An existing issue was already created labels Feb 25, 2015
@QueueHammer
Copy link
Author

@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.

@NoelAbrahams
Copy link

The problem is an imported module may only contain ambient declarations, in which case leaving the import behind could result in a runtime error, because the file itself may not exist at runtime. See #596 (comment)

This does preclude certain patterns, for example CommonJS extensions. But that may be the lesser of the two evils.

(BTW copying the same comment across issues is rather bad form.)

@QueueHammer
Copy link
Author

To your comment ambient declarations are addressed by ///<reference.... When not building AMD modules with TypeScript or using Visual Studio as your IDE, they are the way that "ambient" type information is linked between files. Causing import x = require('y') to not require y so that import can also perform the function of ///<reference... is bad design. I see it as a open bug.

(Hey @NoelAbrahams, I gave unique comments, articulated specifically for each thread. It doesn't change that import still has a bug)

@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 Duplicate An existing issue was already created
Projects
None yet
Development

No branches or pull requests

3 participants