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

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

Closed
wants to merge 1 commit into from

Conversation

@ocombe
Copy link
Contributor

commented Mar 12, 2018

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

This comment has been minimized.

Copy link
@ocombe

ocombe Mar 12, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@chuckjaz

chuckjaz Mar 12, 2018

Member

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

This comment has been minimized.

Copy link
@ocombe

ocombe Mar 12, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@ocombe

ocombe Mar 12, 2018

Author Contributor

The code has been updated to only keep the essentials

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

This comment has been minimized.

Copy link
@ocombe

ocombe Mar 12, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@chuckjaz

chuckjaz Mar 12, 2018

Member

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.

@ocombe ocombe force-pushed the ocombe:feat/ivy/comments branch from 8e06776 to f459a23 Mar 12, 2018

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

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))

This comment has been minimized.

Copy link
@chuckjaz

chuckjaz Mar 12, 2018

Member

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

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

This comment has been minimized.

Copy link
@chuckjaz

chuckjaz Mar 12, 2018

Member

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.

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

This comment has been minimized.

Copy link
@chuckjaz

chuckjaz Mar 12, 2018

Member

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.

This comment has been minimized.

Copy link
@ocombe

ocombe Mar 12, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@chuckjaz

chuckjaz Mar 12, 2018

Member

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

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

This comment has been minimized.

Copy link
@chuckjaz

chuckjaz Mar 12, 2018

Member

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

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

This comment has been minimized.

Copy link
@chuckjaz

chuckjaz Mar 12, 2018

Member

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

@vicb

This comment has been minimized.

Copy link
Contributor

commented Mar 12, 2018

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

This comment has been minimized.

Copy link

commented Mar 12, 2018


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

This comment has been minimized.

Copy link
@vicb

vicb Mar 12, 2018

Contributor

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

This comment has been minimized.

Copy link
@ocombe

ocombe Mar 12, 2018

Author Contributor

yes good idea

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

This comment has been minimized.

Copy link
@vicb

vicb Mar 12, 2018

Contributor

"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()

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

This comment has been minimized.

Copy link
@vicb

vicb Mar 12, 2018

Contributor

const

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))

This comment has been minimized.

Copy link
@vicb

vicb Mar 12, 2018

Contributor

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

This comment has been minimized.

Copy link
@ocombe

ocombe Mar 12, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@vicb

vicb Mar 12, 2018

Contributor

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

This comment has been minimized.

Copy link
@alxhub

alxhub Mar 14, 2018

Contributor

Agreed.

.toBe(`/* Multiline\ncomment */`);
});

it('should support JSDoc comments', () => {

This comment has been minimized.

Copy link
@vicb

vicb Mar 12, 2018

Contributor

Why ? (do we need this ?)

This comment has been minimized.

Copy link
@ocombe

ocombe Mar 12, 2018

Author Contributor

to add i18n desc/meaning/id info for closure

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

This comment has been minimized.

Copy link
@vicb

vicb Mar 12, 2018

Contributor

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

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

This comment has been minimized.

Copy link
@ocombe

ocombe Mar 12, 2018

Author Contributor

oh nice!

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

This comment has been minimized.

Copy link
@vicb

vicb Mar 12, 2018

Contributor

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

This comment has been minimized.

Copy link
@chuckjaz

chuckjaz Mar 14, 2018

Member

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.

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

This comment has been minimized.

Copy link
@vicb

vicb Mar 12, 2018

Contributor

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

This comment has been minimized.

Copy link
@ocombe

ocombe Mar 12, 2018

Author Contributor

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

@ocombe ocombe force-pushed the ocombe:feat/ivy/comments branch from 40e1ca6 to e908aff Mar 12, 2018

@@ -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(

This comment has been minimized.

Copy link
@vicb

vicb Mar 12, 2018

Contributor

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

@@ -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 {

This comment has been minimized.

Copy link
@vicb

vicb Mar 12, 2018

Contributor

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

This comment has been minimized.

Copy link
@ocombe

ocombe Mar 12, 2018

Author Contributor

that's what @alxhub asked me to use

This comment has been minimized.

Copy link
@vicb

vicb Mar 13, 2018

Contributor

@alxhub why ?

This comment has been minimized.

Copy link
@vicb

vicb Mar 13, 2018

Contributor

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

This comment has been minimized.

Copy link
@alxhub

alxhub Mar 14, 2018

Contributor

@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.

* 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.

This comment has been minimized.

Copy link
@vicb

vicb Mar 12, 2018

Contributor

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

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

This comment has been minimized.

Copy link
@vicb

vicb Mar 12, 2018

Contributor

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

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

This comment has been minimized.

Copy link
@vicb

vicb Mar 12, 2018

Contributor

Why escaping @ and not /*, */ ?

This comment has been minimized.

Copy link
@ocombe

ocombe Mar 12, 2018

Author Contributor

no idea, @evmar wrote this for tsickle

This comment has been minimized.

Copy link
@evmar

evmar Mar 12, 2018

Contributor

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.

This comment has been minimized.

Copy link
@vicb

vicb Mar 13, 2018

Contributor

Thanks @evmar.
@ocombe add a test ?

}

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

This comment has been minimized.

Copy link
@vicb

vicb Mar 12, 2018

Contributor
  • 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)

@ocombe ocombe force-pushed the ocombe:feat/ivy/comments branch from 9fbf395 to a759e53 Mar 14, 2018

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

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

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

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

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

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

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

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

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

@ocombe ocombe force-pushed the ocombe:feat/ivy/comments branch from a759e53 to 7f4e112 Mar 14, 2018

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

@ocombe ocombe force-pushed the ocombe:feat/ivy/comments branch from 7f4e112 to 770cccd Mar 14, 2018

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

This comment has been minimized.

Copy link
@vicb

vicb Mar 14, 2018

Contributor

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
super(null, sourceSpan);
}
isEquivalent(stmt: Statement): boolean { return stmt instanceof CommentStmt; }
isEquivalent(stmt: Statement): boolean {
return stmt instanceof CommentStmt && this.comment === stmt.comment &&

This comment has been minimized.

Copy link
@vicb

vicb Mar 14, 2018

Contributor

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

@@ -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', () => {

This comment has been minimized.

Copy link
@vicb

vicb Mar 14, 2018

Contributor

why not ?
this should only be true for JSDoc

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

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

@ocombe ocombe force-pushed the ocombe:feat/ivy/comments branch from 627482f to 5320b98 Mar 14, 2018

@mary-poppins

This comment has been minimized.

Copy link

commented Mar 14, 2018

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

@vicb

vicb approved these changes Mar 14, 2018

Copy link
Contributor

left a comment

Thanks !

@vicb

This comment has been minimized.

Copy link
Contributor

commented Mar 14, 2018

tap/189092495 (thanks @alxhub)

@mhevery mhevery closed this in 3b167be Mar 15, 2018

@ocombe ocombe deleted the ocombe:feat/ivy/comments branch Apr 10, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.