fixes #1908 enhancement request #1909
Conversation
|
Hi @v3nom, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution! TTYL, MSBOT; |
|
@v3nom, Thanks for signing the contribution license agreement so quickly! Actual humans will now validate the agreement and then evaluate the PR. |
|
Hmz, weird, some files are now changed to 755. You might want to look at this: link |
| @@ -4680,7 +4680,7 @@ module ts { | |||
| function processReferenceComments(sourceFile: SourceFile): void { | |||
| var triviaScanner = createScanner(languageVersion, /*skipTrivia*/false, sourceText); | |||
| var referencedFiles: FileReference[] = []; | |||
| var amdDependencies: string[] = []; | |||
| var amdDependencies: {path:string; name:string}[] = []; | |||
There was a problem hiding this comment.
Minor: Please put space between :and string
|
Please add more tests |
| var amdDependencyRegEx = /^\/\/\/\s*<amd-dependency\s+path\s*=\s*('|")(.+?)\1/gim; | ||
| var amdDependencyRegEx = /^\/\/\/\s*<amd-dependency/gim; | ||
| var pathRegex = /path\s*=\s*('|")(.+?)\1/gim | ||
| var nameRegex = /name\s*=\s*('|")(.+?)\1/gim |
|
For the tests, you can look at tests\cases\compiler\amdDependencyComment1.ts as a reference. |
|
Thanks for comments. I have updated code to match code style. Added /s to regexp and added test cases following existing amdDependencyComment(1/2).ts example |
| @@ -3925,6 +3925,12 @@ module ts { | |||
| write(", "); | |||
| emit(imp.name); | |||
| }); | |||
| forEach(node.amdDependencies, amdDependency => { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| var amdDependency = {path: pathMatchResult[2], name: nameMatchResult ? nameMatchResult[2] : undefined }; | ||
| // AMD dependencies with names have to go first in define header | ||
| if (nameMatchResult) { | ||
| amdDependencies.unshift(amdDependency); |
There was a problem hiding this comment.
why not just sort the list in the emitter? i do not think we will have that many AMD imports that a sort would have a noticeable perf impact. this way the parser always have the truth, and the emitter can chose to do some extra work to ensure a valid transformation.
| function sortAMDModules(amdModules: {name: string; path: string}[]) { | ||
| // AMD modules with declared variable names goes first | ||
| return amdModules.sort((moduleA, moduleB) => { | ||
| if (moduleA.name == moduleB.name) { |
There was a problem hiding this comment.
nit: "===" instead of "=="
|
Thanks @v3nom. I think we are good to go. just a couple of comments, and if you can merge master into your branch as well. thanks again. |
|
thanks! |
Extended amd-dependency comment with name attribute, which is used to generate more human-readable JavaScript AMD output. It also simplifies JavaScript AMD module use in TS code. #1908