GeezerVison

Paging for review: Mohamed Hegazy (@mhegazy), Anders Hejlsberg (@ahejlsberg), Yui (@yuit), Daniel Rosenwasser (@DanielRosenwasser), Ryan Cavanaugh (@RyanCavanaugh), Vladimir Matveev (@vladima)

You can get around this by using Reviewable.

Review status: 0 of 8 files reviewed at latest revision, 5 unresolved discussions.

src/compiler/comments.ts, line 143 [r1] (raw file):
emitted as

Put some sort of quote demarcation around the emit.

src/compiler/comments.ts, line 148 [r1] (raw file):
same here regarding demarcation and "as"

src/compiler/comments.ts, line 185 [r1] (raw file):
Just use a numeric comparison

src/compiler/utilities.ts, line 115 [r1] (raw file):
I thought it was CommonJS by default but this works too.

src/compiler/factory.ts, line 148 [r1] (raw file):
What's the point of this exactly? Why can't you say this just returns a PrimaryExpression?

Comments from the review on Reviewable.io

Review status: 0 of 7 files reviewed at latest revision, 5 unresolved discussions.

src/compiler/comments.ts, line 143 [r1] (raw file):
Copy/paste from emitter, but I'll take a look.

src/compiler/comments.ts, line 148 [r1] (raw file):
Copy/paste from emitter.

src/compiler/comments.ts, line 185 [r1] (raw file):
Copy/paste from emitter.

src/compiler/utilities.ts, line 115 [r1] (raw file):
Not when the target is ES6. It looks like someone else had the same idea though, so I will drop these in favor of getEmitScriptTarget and getEmitModuleKind.

Comments from the review on Reviewable.io

Review status: 0 of 7 files reviewed at latest revision, 13 unresolved discussions.

src/compiler/printer.ts, line 184 [r1] (raw file):
Make it explicit withTempFlags.Auto

src/compiler/printer.ts, line 742 [r1] (raw file):
Could we not emit a semicolon as part of emitting a signature list?

src/compiler/printer.ts, line 995 [r1] (raw file):
Does the pretty-printer make no assumptions about the structure? Is it the responsibility of the prior transformers to ensure that an object literal is not the immediate concise body?

src/compiler/printer.ts, line 1106 [r1] (raw file):
Why even do it with a prefix? Or if you do, why not just

emitExpressionWithPrefix(node.asteriskToken ? "yield* " : "yield ", node.expression);·

src/compiler/printer.ts, line 1788 [r1] (raw file):
Cache length

src/compiler/printer.ts, line 1798 [r1] (raw file):
I don't like how the function doesn't document its return value. Can you make the function name more explicit and add a JSDoc comment?

src/compiler/printer.ts, line 1870 [r1] (raw file):
Cache length

src/compiler/printer.ts, line 1984 [r1] (raw file):
What is this condition for?

What about default initializers and optional parameters? Do we have tests for these cases?

Comments from the review on Reviewable.io

Review status: 0 of 12 files reviewed at latest revision, 13 unresolved discussions.

src/compiler/printer.ts, line 742 [r1] (raw file):
This code path won't be used until such a time as we migrate the declaration emitter to use the pretty printer, but in the declaration emitter we always emit a semicolon.

src/compiler/printer.ts, line 995 [r1] (raw file):
That is correct, although I am adding some support to automatically parenthesize expressions to ensure the correct output. One reason for this is to make sure we have the right parentheses around nodes without having to know about the parent node during an individual transformation.

src/compiler/printer.ts, line 1106 [r1] (raw file):
Because yield; is legal in JavaScript, and we don't want to emit yield ;.

src/compiler/printer.ts, line 1788 [r1] (raw file):
I've discussed this with Vladimir Matveev (@vladima) in the past. V8 still does an array length lookup on every iteration to perform a bounds check, so caching the length doesn't necessarily have a real benefit.

src/compiler/printer.ts, line 1798 [r1] (raw file):
I'll fix this, though the equivalent function in emitter.ts is also undocumented.

src/compiler/printer.ts, line 1870 [r1] (raw file):
See above.

src/compiler/printer.ts, line 1984 [r1] (raw file):
These are effectively the same conditions as in emitSignatureParametersForArrow in emitter.ts, with some additional restrictions.

Comments from the review on Reviewable.io