Skip to content

fixes #1908 enhancement request #1909

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

Merged
merged 6 commits into from
Feb 12, 2015
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -44,3 +44,4 @@ scripts/ior.js
scripts/*.js.map
coverage/
internal/
**/.DS_Store
22 changes: 21 additions & 1 deletion src/compiler/emitter.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4057,11 +4057,25 @@ module ts {
}
});
}

function sortAMDModules(amdModules: {name: string; path: string}[]) {
// AMD modules with declared variable names go first
return amdModules.sort((moduleA, moduleB) => {
if (moduleA.name === moduleB.name) {
return 0;
} else if (!moduleA.name) {
return 1;
} else {
return -1;
}
});
}

function emitAMDModule(node: SourceFile, startIndex: number) {
var imports = getExternalImportDeclarations(node);
writeLine();
write("define(");
sortAMDModules(node.amdDependencies);
if (node.amdModuleName) {
write("\"" + node.amdModuleName + "\", ");
}
Expand All @@ -4071,7 +4085,7 @@ module ts {
emitLiteral(<LiteralExpression>getExternalModuleImportDeclarationExpression(imp));
});
forEach(node.amdDependencies, amdDependency => {
var text = "\"" + amdDependency + "\"";
var text = "\"" + amdDependency.path + "\"";
write(", ");
write(text);
});
Expand All @@ -4080,6 +4094,12 @@ module ts {
write(", ");
emit(imp.name);
});
forEach(node.amdDependencies, amdDependency => {
Copy link
Contributor

Choose a reason for hiding this comment

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

what if you have multiple dependencies, ones with names and ones without. this will get the order our of sync, since name is optional and path is not. e.g.:

///<amd-dependency path='module1' />
///<amd-dependency path='module2' name='m2'/>

will emit:

define(["require", "exports", "module1", "module2"], function (require, exports, m2) { }

which is not what you want. so either you reorder them, so that ones with names come first, which we do not do usually as we like to keep emitted code as close as possible to generated code. or you give the other ones unused random names consider using createTempVariable utility in the emitter.

Also worth adding a new test for this case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Giving random names would add unnecessary garbage to emitted code. Since import order does not matter for AMD modules, reordering would make most sense.

Changing order is a bit intrusive to original code, but amd-dependency comment is not a language feature, just a utility. Its also not documented in official materials, so I think violating some project principles is justifiable.

if (amdDependency.name) {
write(", ");
write(amdDependency.name);
}
});
write(") {");
increaseIndent();
emitCaptureThisForNodeIfNecessary(node);
Expand Down
13 changes: 10 additions & 3 deletions src/compiler/parser.ts
Original file line number Diff line number Diff line change
Expand Up @@ -4749,7 +4749,7 @@ module ts {
function processReferenceComments(sourceFile: SourceFile): void {
var triviaScanner = createScanner(sourceFile.languageVersion, /*skipTrivia*/false, sourceText);
var referencedFiles: FileReference[] = [];
var amdDependencies: string[] = [];
var amdDependencies: {path: string; name: string}[] = [];
var amdModuleName: string;

// Keep scanning all the leading trivia in the file until we get to something that
Expand Down Expand Up @@ -4789,10 +4789,17 @@ module ts {
amdModuleName = amdModuleNameMatchResult[2];
}

var amdDependencyRegEx = /^\/\/\/\s*<amd-dependency\s+path\s*=\s*('|")(.+?)\1/gim;
var amdDependencyRegEx = /^\/\/\/\s*<amd-dependency\s/gim;
var pathRegex = /\spath\s*=\s*('|")(.+?)\1/gim;
var nameRegex = /\sname\s*=\s*('|")(.+?)\1/gim;
var amdDependencyMatchResult = amdDependencyRegEx.exec(comment);
if (amdDependencyMatchResult) {
amdDependencies.push(amdDependencyMatchResult[2]);
var pathMatchResult = pathRegex.exec(comment);
var nameMatchResult = nameRegex.exec(comment);
if (pathMatchResult) {
var amdDependency = {path: pathMatchResult[2], name: nameMatchResult ? nameMatchResult[2] : undefined };
amdDependencies.push(amdDependency);
}
}
}
}
Expand Down
2 changes: 1 addition & 1 deletion src/compiler/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -887,7 +887,7 @@ module ts {
fileName: string;
text: string;

amdDependencies: string[];
amdDependencies: {path: string; name: string}[];
amdModuleName: string;
referencedFiles: FileReference[];

Expand Down
2 changes: 1 addition & 1 deletion src/services/services.ts
Original file line number Diff line number Diff line change
Expand Up @@ -725,7 +725,7 @@ module ts {
public statements: NodeArray<Statement>;
public endOfFileToken: Node;

public amdDependencies: string[];
public amdDependencies: {name: string; path: string}[];
public amdModuleName: string;
public referencedFiles: FileReference[];

Expand Down
10 changes: 10 additions & 0 deletions tests/baselines/reference/amdDependencyCommentName1.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
tests/cases/compiler/amdDependencyCommentName1.ts(3,21): error TS2307: Cannot find external module 'm2'.


==== tests/cases/compiler/amdDependencyCommentName1.ts (1 errors) ====
///<amd-dependency path='bar' name='b'/>

import m1 = require("m2")
~~~~
!!! error TS2307: Cannot find external module 'm2'.
m1.f();
10 changes: 10 additions & 0 deletions tests/baselines/reference/amdDependencyCommentName1.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
//// [amdDependencyCommentName1.ts]
///<amd-dependency path='bar' name='b'/>

import m1 = require("m2")
m1.f();

//// [amdDependencyCommentName1.js]
///<amd-dependency path='bar' name='b'/>
var m1 = require("m2");
m1.f();
10 changes: 10 additions & 0 deletions tests/baselines/reference/amdDependencyCommentName2.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
tests/cases/compiler/amdDependencyCommentName2.ts(3,21): error TS2307: Cannot find external module 'm2'.


==== tests/cases/compiler/amdDependencyCommentName2.ts (1 errors) ====
///<amd-dependency path='bar' name='b'/>

import m1 = require("m2")
~~~~
!!! error TS2307: Cannot find external module 'm2'.
m1.f();
11 changes: 11 additions & 0 deletions tests/baselines/reference/amdDependencyCommentName2.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
//// [amdDependencyCommentName2.ts]
///<amd-dependency path='bar' name='b'/>

import m1 = require("m2")
m1.f();

//// [amdDependencyCommentName2.js]
///<amd-dependency path='bar' name='b'/>
define(["require", "exports", "m2", "bar"], function (require, exports, m1, b) {
m1.f();
});
12 changes: 12 additions & 0 deletions tests/baselines/reference/amdDependencyCommentName3.errors.txt
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
tests/cases/compiler/amdDependencyCommentName3.ts(5,21): error TS2307: Cannot find external module 'm2'.


==== tests/cases/compiler/amdDependencyCommentName3.ts (1 errors) ====
///<amd-dependency path='bar' name='b'/>
///<amd-dependency path='foo'/>
///<amd-dependency path='goo' name='c'/>

import m1 = require("m2")
~~~~
!!! error TS2307: Cannot find external module 'm2'.
m1.f();
15 changes: 15 additions & 0 deletions tests/baselines/reference/amdDependencyCommentName3.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
//// [amdDependencyCommentName3.ts]
///<amd-dependency path='bar' name='b'/>
///<amd-dependency path='foo'/>
///<amd-dependency path='goo' name='c'/>

import m1 = require("m2")
m1.f();

//// [amdDependencyCommentName3.js]
///<amd-dependency path='bar' name='b'/>
///<amd-dependency path='foo'/>
///<amd-dependency path='goo' name='c'/>
define(["require", "exports", "m2", "bar", "goo", "foo"], function (require, exports, m1, b, c) {
m1.f();
});
5 changes: 5 additions & 0 deletions tests/cases/compiler/amdDependencyCommentName1.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
//@module: commonjs
///<amd-dependency path='bar' name='b'/>

import m1 = require("m2")
m1.f();
5 changes: 5 additions & 0 deletions tests/cases/compiler/amdDependencyCommentName2.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
//@module: amd
///<amd-dependency path='bar' name='b'/>

import m1 = require("m2")
m1.f();
7 changes: 7 additions & 0 deletions tests/cases/compiler/amdDependencyCommentName3.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
//@module: amd
///<amd-dependency path='bar' name='b'/>
///<amd-dependency path='foo'/>
///<amd-dependency path='goo' name='c'/>

import m1 = require("m2")
m1.f();