Skip to content

Output declarations should include module name #17379

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

Closed
madelson opened this issue Jul 24, 2017 · 13 comments
Closed

Output declarations should include module name #17379

madelson opened this issue Jul 24, 2017 · 13 comments
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug

Comments

@madelson
Copy link

TypeScript Version: Microsoft.TypeScript.MSBuild 2.4.1

Code

/// <amd-module name="foo" />
export class Bar { ... }

Expected behavior:
I would expect my explicit module name to be preserved in the output .d.ts file:

declare module "foo" {
    export class Bar { ... }
}

Actual behavior:
The explictly-specified module name seems to be ignored completely:

export declare class Bar { ... }
@mhegazy
Copy link
Contributor

mhegazy commented Aug 23, 2017

module names will not be changed in the declaration output unless --outFile is used. otherwise the compiler prefers keeping modules within their scope and not lifting them tot he global scope.

@mhegazy mhegazy added the Working as Intended The behavior described is the intended behavior; this is not a bug label Aug 23, 2017
@madelson
Copy link
Author

@mhegazy can you explain why custom module names should be discarded absent --outFile? As a consumer trying to use custom module names, I don't understand why these concepts are connected at all.

It seems to me that the output declaration file should exactly mirror the "shape" of the TypeScript source file. With the current behavior they don't quite match up.

Even if emitting custom modules names is not the default, it would be great if it could be an option.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 23, 2017

how are you packaging these modules to your users?

@madelson
Copy link
Author

@mhegazy we are currently using NuGet packages on an internal NuGet feed. We want to migrate to NPM packages on an internal NPM feed, but we aren't there yet. Either way we need to custom module name to be preserved in transit because the consuming application will be referencing these modules as remotely-hosted AMD modules resolved and loaded via RequireJS at runtime.

@mhegazy
Copy link
Contributor

mhegazy commented Aug 23, 2017

and --outFile is not an option?

@madelson
Copy link
Author

I don't think outFile would work. We need the compiled JS to stay in separate files since we leverage the capability to load just the modules required by the page (as well as dynamically loading additional modules at runtime)

@mhegazy
Copy link
Contributor

mhegazy commented Aug 24, 2017

so if the modules are left in their own files, i would recommend keeping the .d.ts in the same structure.

the main difference between a declare module "foo" {..} and a file named foo.d.ts is the scoping. the earlier is in the global scope, while the later is in its own scope. having declarations in the global scope can cause conflicts if more than one version of the code is used. Modules by definition allow for multiple versions, since they are isolated. This is why the compiler does not generate global declarations unless it has no other way around it (i.e. --outFile scenarios).

@aluanhaddad
Copy link
Contributor

As a consumer of an AMD module I would expect the import name and the file name to match unless explicitly documented otherwise. If they do not match then, unless you add additional configuration to your AMD loader then it will not be able to locate them. If you want to use a different module name than the file name you should consider using paths configuration in tsconfig.json which would be used to mirror the loader configuration.

Also named defines should be avoided for the same reason that TypeScript avoids emitting declare module "foo" {} stated above and generally under the same circumstances.

@madelson
Copy link
Author

madelson commented Aug 26, 2017

@mhegazy @aluanhaddad thank you for your continued engagement here.

A few thoughts:

  • We are aware of the paths option in tsconfig.json: this is the workaround we are currently using. However, it is annoying from a .d.ts distribution perspective because it means consumers must update their tsconfig.json files in addition to pulling down our .d.ts package (right now since we're using NuGet we're getting around this with an install.ps1 script that does this update for them, but this feels hacky).

  • I'm not sure I agree with the statement that the import and file name should always match when explicitly named imports are used. The reason that we use explicit naming is to divorce modules from physical file locations (in our case, apps end up loading modules that are hosted by other apps to create "mashup" style pages). It seems inconsistent to me that TypeScript offers explicit naming (which I agree should be optional and is a somewhat niche use-case), but then chooses to throw those names away under some conditions.

@mhegazy
Copy link
Contributor

mhegazy commented Sep 11, 2017

Automatically closing this issue for housekeeping purposes. The issue labels indicate that it is unactionable at the moment or has already been addressed.

@mhegazy mhegazy closed this as completed Sep 11, 2017
@aluanhaddad
Copy link
Contributor

I'm not sure I agree with the statement that the import and file name should always match when explicitly named imports are used. The reason that we use explicit naming is to divorce modules from physical file locations (in our case, apps end up loading modules that are hosted by other apps to create "mashup" style pages).

In general, I understand your desire to abstract the module specifier used to import a module from the file name or URL. However, JavaScript module systems typically consider the file name to be the specifier (although additional resolution logic is added to at least some degree by all practical mechanisms). Aliasing, or abstracted module naming, via mechanisms such as paths and as provided by various package managers and loaders is actually quite important.
For example being able to write

import $ = require('jquery');
$('div').text('from $');

and have it resolve to ../bower_components/jquery/jquery.js is an example of this and is obviously valuable. RequireJS provides for this by allowing the short name to be mapped to the full location. If you want to take the code above and load it with Webpack or JSPM, you won't need to make any changes to the code itself, rather just to the bundler or loader configuration.

It seems inconsistent to me that TypeScript offers explicit naming (which I agree should be optional and is a somewhat niche use-case), but then chooses to throw those names away under some conditions.

While I am not entirely sure what you mean by "throw away", as stated above, having

/// <amd-module name="foo" />
export class Bar { ... }

emit a declaration file

declare module "foo" {
    export class Bar { ... }
}

pollutes the global namespace and therefore does not scale. This breaks one of the key use cases for aliasing, multi-versioning, something which TypeScript should have better support for, not worse. Mashups are a very good reason to avoid ambient declarations because two occurrences of declare module "foo" in the same application will either conflict or allow erroneous code.
declare module "foo" {export = "bar";} in a mashup would be rather like having define("foo", () => "bar");. Having the alias "foo" affect other packages would be a bad thing.

This does create problems when exposing an API for consumption or when using package mangers or loaders that do not follow NodeJS conventions. Explicit paths configuration is necessary, but very hard to share. One approach you might consider would be to place the paths mapping in dedicated configuration file, say tsconfig.paths.json, in order to leverage configuration inheritance to share it/ship it more easily.

@madelson
Copy link
Author

madelson commented Sep 14, 2017

@aluanhaddad it seems like the primary concern here is pollution of the global namespace. I see that, but I guess I would assert that people who are using custom module names are doing so in a way that avoids naming collisions (e. g. similar to how languages like C# use qualified names rather than file paths to avoid conflicts).

In that sense, were we to care about multi-versioning we'd probably do so via different custom module names (although in our case we've never had this need).

I agree that paths is the right current workaround (it's what we are using currently); I just wish it "just worked" instead.

I'm curious whether you think other users of custom module names would want this behavior or whether they would prefer the current behavior. I don't know all the use-cases of custom module names, but I'd assume that those who are using them want to avoid thinking about file paths as much as possible.

@aluanhaddad
Copy link
Contributor

aluanhaddad commented Sep 14, 2017

EDIT: It seems I am mistaken. Given how /// <amd-module name="bar"/> works currently, this should probably work as you wish.
See #18454

I maintain that this is a very poor practice.

@madelson the problem with choosing your own custom module names is that third party code such as libraries needs to be able to be authored without knowledge of how you plan to resolve the conflict.

Given

// bar.ts
/// <amd-module name="foo" />
export class Bar { ... }

generating the following would hurt portability

// bar.js
define((require, exports) => exports.Bar = class Bar {});
// bar.d.ts
declare module "foo" {export class Bar {}}

As opposed to

// bar.js
define((require, exports) => exports.Bar = class Bar {});
// main.js
require.config({
  "paths": {
    "foo": "bar"
  }
});
// tsconfig.json
{
  "compilerOptions": {
    "paths": {
      "foo": ["bar"]
    }
  }
}

I would prefer the latter as it keeps options open and keeps modules concerned with their functionality, as opposed to by which specifier they will be imported.

If you are doing it for more than a handful of modules, you are already maintaining a lot of boilerplate in the triple-slash-comments in the first place. Also, it makes sense to put these settings at project level or package level.

@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Working as Intended The behavior described is the intended behavior; this is not a bug
Projects
None yet
Development

No branches or pull requests

3 participants