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

Adding support for named AMD modules. #1158

Merged
merged 3 commits into from Nov 18, 2014
Merged

Conversation

gisenberg
Copy link
Contributor

This pull request adds support for named AMD modules and addresses the pain point of using TypeScript-generated AMD modules when anonymous modules and out-of-the-box bundlers (like r.js) are not an option.

This feature was previously discussed at https://typescript.codeplex.com/discussions/451454 and https://typescript.codeplex.com/workitem/285 where it was resolved as external/won't fix, but this feature was a big enough pain point in our recent adoption of TypeScript to warrant further investigation.

@sophiajt
Copy link
Contributor

Thanks for the PR. We chatted a bit on Friday about it, and should be able to take a look shortly (a few of us have been out on a business trip this past week).

Thanks for being patient.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 16, 2014

I do not like the use of comments to direct compilation, but i do not have a better solution either. implementation wise, I have no comments.

We will need a signed Contributor License Agreement (CLA) before proceeding. Download the agreement (Microsoft Contribution License Agreement.docx or Microsoft Contribution License Agreement.pdf), sign, scan, and email it back to cla@microsoft.com.

@gisenberg
Copy link
Contributor Author

Thanks for reviewing! A signed CLA was sent over on 11/13.

My initial implementation hooked into export assignments (export = Foo : "moduleName"), but that was inconsistent with colon usage elsewhere in the language and had implications for folks using TSLint. I switched it over to a reference comment since that usage pattern was already being used for amd-dependencies and seemed to rock the boat the least.

@osbornm
Copy link

osbornm commented Nov 16, 2014

This does match the existing /// <amd_dependecy path='...'/> pattern

@@ -4237,6 +4239,12 @@ module ts {
}
}
else {
var amdModuleNameRegEx = /^\/\/\/\s*<amd-module\s+name\s*=\s*('|")(.+?)\1/gim;
var amdModuleNameMatchResult = amdModuleNameRegEx.exec(comment);
if(amdModuleNameMatchResult) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Report error for duplicate entry if !amdModuleName?
Add test case for same

@gisenberg
Copy link
Contributor Author

Thanks for the feedback! I've updated the parser to treat multiple AMD module name declarations as an error condition and added a compiler test for that scenario.

@mhegazy
Copy link
Contributor

mhegazy commented Nov 18, 2014

thanks!

mhegazy added a commit that referenced this pull request Nov 18, 2014
Adding support for named AMD modules.
@mhegazy mhegazy merged commit 22e2bde into microsoft:master Nov 18, 2014
@osbornm
Copy link

osbornm commented Nov 18, 2014

tumblr_m2ybpf8hd11qihztbo1_400

@gisenberg
Copy link
Contributor Author

Thanks!

@ca0v
Copy link

ca0v commented Dec 8, 2014

It strikes me that this would make a great compiler option as the first step in creating an AMD build. When tsc -out is specified with -m amd we automatically give names to these modules. The names are based on the location in the file system. I think --sourceRoot could identify an optional root folder is need be (not sure if that matters).

@gisenberg
Copy link
Contributor Author

We also considered the compiler switch option, but updating the VS tooling wasn't something that could be addressed with a pull request.

@slaneyrw
Copy link

Just tried out this feature for the first time and found that the explicit name suppied in the "amd-module" directive was not then subsequently used when importing into another module. but kept the value used in the require statement.

/* app\classes\MyClass.ts */
/// <amd-module name="MyClass" />
class MyClass {
   ...
}

/* app\classes\MyClass.js */
define("MyClass", ["require", "exports"], function(require, exports) {
    ...
}

/* app\MyApp.ts */
import MyClass = require("classes/MyClass");
class MyApp {
    ...
}

/* app\MyApp.js */
define(["require", "exports", "classes/MyClass"], function(require, exports, MyClass) {
    ...
}

In this example I would have expected MyApp.js to use "MyClass" not "classes/MyClass" for the dependency name.

Is this expected ? If so is there any plans to "close the loop" ?

@mhegazy
Copy link
Contributor

mhegazy commented Mar 15, 2015

The /// is just used when emitting the file. it is more of an emitter directive if anything. We will need some clear language syntax that denoted a module name when it is different from a file name. possibly related to #17. Currently the assumption is that module names and file names are in sync, which does not really work with bundling scenarios, which i am amusing is what you are trying to do. We are looking into making module resolution work better with common AMD/CommonJS/SystemJs patterns (take a look at #2338).

@luboid
Copy link

luboid commented May 4, 2015

Could this be realized

When in the module exists amd-module then need to be created .d.ts file, which will be located in well known place named_modules in the root directory of the project for example, then easy can be created reference to them from TypeScript and loading of JS files will be left to whichever module loader. If module name is "something1/somthig2/somthig3" then directory structure will be :

project
named_modules
something1
somthig2
somthig3.d.ts

only one catch here is, module do not need contain references to others with ./ or ../ - all references need to be typings

@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
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants