feat(compiler): support for singleline, multiline & jsdoc comments #22715

ocombe · 2018-03-12T15:50:07Z

PR Type

What kind of change does this PR introduce?

[x] Feature

What is the current behavior?

There's no way to output comments with ngc

What is the new behavior?

You can create singleline, multiline and jsdoc comments.
I've reused some code of tsickle for the jsdoc comments, since it's not exported by tsickle, I just copy pasted it.

Does this PR introduce a breaking change?

[x] No

ocombe · 2018-03-12T15:51:49Z

packages/compiler/src/output/output_ast.ts

@@ -1467,3 +1484,165 @@ export function literal(
    value: any, type?: Type | null, sourceSpan?: ParseSourceSpan | null): LiteralExpr {
  return new LiteralExpr(value, type, sourceSpan);
 }
+


all of the code in this file below this line is copy pasted from tsickle

Are we going to add enough JsDoc comments to justify this code?

I don't know, I can make a simple implementation for now that doesn't check the tags at all and just formats the string, and we'll expand it later if we need to

The code has been updated to only keep the essentials

ocombe · 2018-03-12T15:54:13Z

packages/compiler/src/output/output_ast.ts

 export class CommentStmt extends Statement {
-  constructor(public comment: string, sourceSpan?: ParseSourceSpan|null) {
+  constructor(public comment: string, sourceSpan?: ParseSourceSpan|null, public multiline = false) {


should I put multiline before sourceSpan? I didn't want to break anything since it's exported

Please move it before span. This is exported but nothing in @angular/compiler nor @angular/compiler-cli are considered part of the public API are are not under the same restrictions as say @angular/core.

chuckjaz · 2018-03-12T16:27:09Z

packages/compiler-cli/src/transformers/node_emitter.ts

-    const statements: any[] = [].concat(
-        ...stmts.map(stmt => stmt.visitStatement(converter, null)).filter(stmt => stmt != null));
+    const statements: any[] =
+        [].concat(...stmts.map(stmt => stmt.visitStatement(converter, sourceFile))


Consider removing [].concat(... as it doesn't appear to be doing anything.

chuckjaz · 2018-03-12T16:31:24Z

packages/compiler/src/output/output_ast.ts

 export class CommentStmt extends Statement {
-  constructor(public comment: string, sourceSpan?: ParseSourceSpan|null) {
+  constructor(public comment: string, sourceSpan?: ParseSourceSpan|null, public multiline = false) {


Please move it before span. This is exported but nothing in @angular/compiler nor @angular/compiler-cli are considered part of the public API are are not under the same restrictions as say @angular/core.

chuckjaz · 2018-03-12T16:39:07Z

packages/compiler-cli/src/transformers/node_emitter.ts

-  visitCommentStmt(stmt: CommentStmt) { return null; }
+  visitCommentStmt(stmt: CommentStmt, context: any) {
+    let comment = stmt.comment;
+    const commentStmt = ts.createNotEmittedStatement(context);


This and JSDocCommentStmt substantially the same here and also in the AbstractEmitterVisitor which seems to argue for JSDoc being a field of CommentStmt instead of a separate node.

If we decide to keep the nodes separate then we need to create a helper function that both these methods call.

either one works for me, what do you think @alxhub ?

@ocombe As discussed on slack, keep the JSDocCommentStmt but create helper methods that contain the shared code in the handlers.

chuckjaz · 2018-03-12T16:40:29Z

packages/compiler/src/output/output_ast.ts

@@ -1467,3 +1484,165 @@ export function literal(
    value: any, type?: Type | null, sourceSpan?: ParseSourceSpan | null): LiteralExpr {
  return new LiteralExpr(value, type, sourceSpan);
 }
+


Are we going to add enough JsDoc comments to justify this code?

chuckjaz · 2018-03-12T18:20:11Z

packages/compiler-cli/src/transformers/node_emitter.ts

-  visitCommentStmt(stmt: CommentStmt) { return null; }
+  visitCommentStmt(stmt: CommentStmt, context: any) {
+    let comment = stmt.comment;
+    const commentStmt = ts.createNotEmittedStatement(context);


@ocombe As discussed on slack, keep the JSDocCommentStmt but create helper methods that contain the shared code in the handlers.

vicb · 2018-03-12T18:53:21Z

Please remove all references to ivy from the commit message and PR header (you can say that this feature will be used by Ivy but it is otherwise unrelated).

mary-poppins · 2018-03-12T18:56:30Z

You can preview 40e1ca6 at https://pr22715-40e1ca6.ngbuilds.io/.

vicb · 2018-03-12T18:57:53Z

packages/compiler-cli/src/transformers/node_emitter.ts

+
+  private createCommentStmt(
+      text: string,
+      kind: ts.SyntaxKind.SingleLineCommentTrivia|ts.SyntaxKind.MultiLineCommentTrivia,


would it make sense for kind to be singleLine: boolean instead and the ts impl details stay encapsulated inside this private method ?

yes good idea

vicb · 2018-03-12T18:59:02Z

packages/compiler-cli/src/transformers/node_emitter.ts

+  private createCommentStmt(
+      text: string,
+      kind: ts.SyntaxKind.SingleLineCommentTrivia|ts.SyntaxKind.MultiLineCommentTrivia,
+      context: any): ts.NotEmittedStatement {


"context" is meaningless here, please update the name and the type.

(you might also want to update the "context" name and type in visit(.*)CommentStmt()

vicb · 2018-03-12T19:00:30Z

packages/compiler-cli/src/transformers/node_emitter.ts

+      text: string,
+      kind: ts.SyntaxKind.SingleLineCommentTrivia|ts.SyntaxKind.MultiLineCommentTrivia,
+      context: any): ts.NotEmittedStatement {
+    let commentStmt = ts.createNotEmittedStatement(context);


vicb · 2018-03-12T19:03:43Z

packages/compiler-cli/test/transformers/node_emitter_spec.ts

+    it('should support multiline comments', () => {
+      expect(emitStmt(new o.CommentStmt('Multiline comment', true)))
+          .toBe('/* Multiline comment */');
+      expect(emitStmt(new o.CommentStmt(`Multiline\ncomment`, true), false))


q: what is the "false" here (ie why should the call be different depending on the comment content ?)

Because by default the emitStmt will remove newlines and such, it's easier to test like this when newlines are irrelevant, especially since TS will add spaces for indentation.
But in this case I wanted to make sure that it was adding new lines, that's why I changed it

Format.Raw and Format.Flat would be nicer IMO.
(Not asking to change this time, up to you)

vicb · 2018-03-12T19:04:44Z

packages/compiler-cli/test/transformers/node_emitter_spec.ts

+          .toBe(`/* Multiline\ncomment */`);
+    });
+
+    it('should support JSDoc comments', () => {


Why ? (do we need this ?)

to add i18n desc/meaning/id info for closure

vicb · 2018-03-12T19:05:44Z

packages/compiler-cli/test/transformers/node_emitter_spec.ts

+          .toBe(`/**\n * @desc description\n */`);
+      expect(emitStmt(
+                 new o.JSDocCommentStmt([
+                   {text: 'Intro comment'}, {tagName: o.JSDocTag.Desc, text: 'description'},


nit: adding a trailing comma would make the formatted output more readable with 1 obj per line

{...}, {...}, {...},

vicb · 2018-03-12T19:15:59Z

packages/compiler/src/output/output_ast.ts

+  constructor(public tags: Tag[] = [], sourceSpan?: ParseSourceSpan|null) {
+    super(null, sourceSpan);
+  }
+  isEquivalent(stmt: Statement): boolean { return stmt instanceof JSDocCommentStmt; }


Not sure about the usage of isEquivalent() but sounds like we should also make sure the tags are equivalent ?
The purpose of those tags is to add metadata.
@chuckjaz

The purpose of isEquivalent() is to detect changes that require a new .js file to be emitted in watch mode. Normally comments are ignored. As these comments are semantic we should re-emit the file if the comment changes so isEquivalent should ensure the tags are equivalent as well as the type.

vicb · 2018-03-12T19:17:35Z

packages/compiler/src/output/output_ast.ts

+  visitStatement(visitor: StatementVisitor, context: any): any {
+    return visitor.visitJSDocCommentStmt(this, context);
+  }
+  toString(includeStartEnd = false): string { return serialize(this.tags, includeStartEnd); }


toString() should not have a parameter, why is that used ?

something that is left from the tsickle code, removing it since we don't use it

vicb · 2018-03-12T20:42:15Z

packages/compiler-cli/src/transformers/node_emitter.ts

@@ -72,8 +72,8 @@ export function updateSourceFile(
      classes.map<[string, ClassStmt]>(classStatement => [classStatement.name, classStatement]));
  const classNames = new Set(classes.map(classStatement => classStatement.name));

-  const prefix =
-      <ts.Statement[]>prefixStatements.map(statement => statement.visitStatement(converter, null));
+  const prefix = <ts.Statement[]>prefixStatements.map(


Would const prefix: ts.Statement[] = ... works ?
Better readability IMO

vicb · 2018-03-12T20:49:51Z

packages/compiler/src/output/output_ast.ts

@@ -1467,3 +1482,55 @@ export function literal(
    value: any, type?: Type | null, sourceSpan?: ParseSourceSpan | null): LiteralExpr {
  return new LiteralExpr(value, type, sourceSpan);
 }
+
+/** The list of JSDoc tags that we currently support. Extend it if needed. */
+export enum JSDocTag {


Why do we need a enum here (instead of using string ?) at least Tag|string ?

that's what @alxhub asked me to use

@alxhub why ?

One reason I could imagine is to document the values... but I don't see comment here ?

@vicb my view is that since JSDoc tags are semantically meaningful, we should have some typechecking around which tags we support generating. Either an enum or a union type of 'tagname'|'othertag' would accomplish this, but I don't think we should accept any arbitrary string.

vicb · 2018-03-12T20:50:51Z

packages/compiler/src/output/output_ast.ts

+ * TypeScript has an API for JSDoc already, but it's not exposed.
+ * https://github.com/Microsoft/TypeScript/issues/7393
+ * For now we create types that are similar to theirs so that migrating
+ * to their API will be easier.  See e.g. ts.JSDocTag and ts.JSDocComment.


use backquote for code ref ie around "ts.JSDocTag"

vicb · 2018-03-12T20:51:29Z

packages/compiler/src/output/output_ast.ts

+ */
+export interface Tag {
+  /**
+   * tagName is e.g. "param" in an @param declaration.


quote @param, tagName and so on (especially important for @param)

vicb · 2018-03-12T20:54:00Z

packages/compiler/src/output/output_ast.ts

+    out += ` @${tag.tagName}`;
+  }
+  if (tag.text) {
+    out += ' ' + tag.text.replace(/@/g, '\\@');


Why escaping @ and not /*, */ ?

no idea, @evmar wrote this for tsickle

I don't have context on this code, but at least in tsickle the incoming comment text itself cannot contain /* or */ (because otherwise the code wouldn't have parsed), and the escaping of @ is so the Closure compiler doesn't parse it.

Thanks @evmar.
@ocombe add a test ?

vicb · 2018-03-12T20:56:48Z

packages/compiler/src/output/output_ast.ts

+}
+
+/**
+ * Serializes a Tag into a string usable in a comment.


quote Tag,

"a string usable in a comment." any string is usable in a comment

Do not use JSDoc for internal function/methods

"note the whitespace" well I notice several WS, do you mean the leading one ? Maybe @tagName text would be a better description (matching Tag props)

vicb · 2018-03-14T17:08:50Z

packages/compiler-cli/src/transformers/node_emitter.ts

-  visitCommentStmt(stmt: CommentStmt) { return null; }
+  visitCommentStmt(stmt: CommentStmt, sourceFile: ts.SourceFile) {
+    return this.createCommentStmt(
+        stmt.multiline ? ` ${stmt.comment} ` : ` ${stmt.comment}`, stmt.multiline, sourceFile);


nit: extract ```
stmt.multiline ? ${stmt.comment} : ` ${stmt.comment}`

to a local const. Whenever the evaluation order is not important it improves readability - plus you can add a meaningful name

vicb · 2018-03-14T17:11:44Z

packages/compiler/src/output/output_ast.ts

    super(null, sourceSpan);
  }
-  isEquivalent(stmt: Statement): boolean { return stmt instanceof CommentStmt; }
+  isEquivalent(stmt: Statement): boolean {
+    return stmt instanceof CommentStmt && this.comment === stmt.comment &&


I don't think this change is required only JsDocComments have a semantic value

vicb · 2018-03-14T17:14:09Z

packages/compiler/test/output/output_ast_spec.ts

@@ -21,5 +21,33 @@ import * as o from '../../src/output/output_ast';
        expect(o.collectExternalReferences([stmt])).toEqual([ref1, ref2]);
      });
    });
+
+    describe('comments', () => {
+      it('different CommentStmt should not be equivalent', () => {


why not ?
this should only be true for JSDoc

mary-poppins · 2018-03-14T21:37:03Z

You can preview 5320b98 at https://pr22715-5320b98.ngbuilds.io/.

vicb

Thanks !

vicb · 2018-03-14T22:07:19Z

tap/189092495 (thanks @alxhub)

…ngular#22715) PR Close angular#22715

angular-automatic-lock-bot · 2019-09-13T12:51:12Z

This issue has been automatically locked due to inactivity.
Please file a new issue if you are encountering a similar or related problem.

Read more about our automatic conversation locking policy.

_{This action has been performed automatically by a bot.}

ocombe added action: review The PR is still awaiting reviews from at least one requested reviewer target: major This PR is targeted for the next major release comp: ivy labels Mar 12, 2018

ocombe requested review from chuckjaz and alxhub March 12, 2018 15:50

googlebot added the cla: yes label Mar 12, 2018

ocombe commented Mar 12, 2018

View reviewed changes

ocombe force-pushed the feat/ivy/comments branch from 8e06776 to f459a23 Compare March 12, 2018 16:08

angular deleted a comment from mary-poppins Mar 12, 2018

chuckjaz suggested changes Mar 12, 2018

View reviewed changes

chuckjaz approved these changes Mar 12, 2018

View reviewed changes

vicb reviewed Mar 12, 2018

View reviewed changes

ocombe force-pushed the feat/ivy/comments branch from 40e1ca6 to e908aff Compare March 12, 2018 19:45

vicb reviewed Mar 12, 2018

View reviewed changes

angular deleted a comment from mary-poppins Mar 14, 2018

ocombe force-pushed the feat/ivy/comments branch from a759e53 to 7f4e112 Compare March 14, 2018 17:03

angular deleted a comment from mary-poppins Mar 14, 2018

ocombe force-pushed the feat/ivy/comments branch from 7f4e112 to 770cccd Compare March 14, 2018 17:06

vicb reviewed Mar 14, 2018

View reviewed changes

angular deleted a comment from mary-poppins Mar 14, 2018

feat(compiler): support for singleline, multiline & jsdoc comments

5320b98

ocombe force-pushed the feat/ivy/comments branch from 627482f to 5320b98 Compare March 14, 2018 21:27

angular deleted a comment from mary-poppins Mar 14, 2018

vicb approved these changes Mar 14, 2018

View reviewed changes

vicb added action: merge The PR is ready for merge by the caretaker and removed action: review The PR is still awaiting reviews from at least one requested reviewer labels Mar 14, 2018

mhevery closed this in 3b167be Mar 15, 2018

leo6104 pushed a commit to leo6104/angular that referenced this pull request Mar 25, 2018

feat(compiler): support for singleline, multiline & jsdoc comments (a…

6be8e95

…ngular#22715) PR Close angular#22715

ocombe deleted the feat/ivy/comments branch April 10, 2018 11:33

angular-automatic-lock-bot bot locked and limited conversation to collaborators Sep 13, 2019

feat(compiler): support for singleline, multiline & jsdoc comments #22715

feat(compiler): support for singleline, multiline & jsdoc comments #22715

Conversation

ocombe commented Mar 12, 2018 • edited Loading

PR Type

What is the current behavior?

What is the new behavior?

Does this PR introduce a breaking change?

ocombe Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb commented Mar 12, 2018

mary-poppins commented Mar 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ocombe Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

vicb Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vicb Mar 12, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mary-poppins commented Mar 14, 2018

vicb left a comment

Choose a reason for hiding this comment

vicb commented Mar 14, 2018

angular-automatic-lock-bot bot commented Sep 13, 2019

ocombe commented Mar 12, 2018 •

edited

Loading

ocombe Mar 12, 2018 •

edited

Loading

vicb Mar 12, 2018 •

edited

Loading

ocombe Mar 12, 2018 •

edited

Loading

vicb Mar 12, 2018 •

edited

Loading

vicb Mar 12, 2018 •

edited

Loading