Fix: Only set optional property on certain node property (fixes #289)

[soda0289]

I moved the optional property logic to each parent node instead of trying to figure it out in the Identifier node.

[eslintbot]

LGTM

[JamesHenry]

I think this is still broken in the case where the method or property is computed. E.g. adapting your test case:

class X {
    private ['foo']? = undefined;
}

...optional is missing from the AST as I mention here https://github.com/JamesHenry/tsep-babylon-test/issues/13#issuecomment-301612343

[eslintbot]

LGTM

[soda0289]

@JamesHenry Sorry about the delay. For computed properties there is no Identifier node so instead I placed the optional flag on the ClassProperty node itself which is a little strange since if it's an Identifier and its not a computed property the optional flag appears on the Identifier node.

[JamesHenry]

@soda0289 I don't think we should allow that. As per JamesHenry/tsep-babylon-test#13, I think we should be consistent with our placement of optional. Is there anything I am missing?

[soda0289]

@JamesHenry Are you saying the optional flag should always be on the parent node? In this case the ClassProperty Node. This would not be consistent with function parameters since functions use an array Identifier nodes for params and do not have a parent node, other than the FunctionExpression. We could make an exception for that case.

[JamesHenry]

Hmm yeah upon further digging the TypeScript AST (with its top level Parameter node) is actually a bit more powerful than ESTree/Flow for function parameters in terms of what it can encapsulate. We are currently aligned with Flow in the positioning of the optional flag on the identifier in the case of functions, but Flow does not have the concept of an optional class property because it does not use them as interfaces in the same way...

I think we should confirm this decision either way with Andy on JamesHenry/tsep-babylon-test#13 - it's not going to be a perfect either way, we just need to align on it I think.

However, given that my take a while, let's get this change in as-is and loop back to it if we have to make another breaking change.

[soda0289]

Sounds good. I'll ask for a release today after this is merged in.

[vjeux]

After this commit,

interface I {
    [Symbol.iterator]?: { x };
}

no longer has optional: true

[soda0289]

@vjeux I can look into it

[vjeux]

I'm really excited, the list of remaining issues for typescript in prettier is getting real close to 0, keep up the awesome work!
prettier/prettier#1787

[soda0289]

@JamesHenry @vjeux Currently optional: true is set on the MemberExpression node. computed is set to false which I guess is still a bug since I think computed should be moved to the TSPropertySignatureNode

[vjeux]

Okay, so for

interface I {
    [Symbol.iterator]?: { x };
}

there's now a questionToken instead of optional: true. Right now we print it the ? if n.questionToken && !n.name.optional. I tried doing n.questionToken || n.name.optional but then

interface Base {
   x: {
-    a?: string,
+    a??: string,
     b: string
   };
 }

-var v: { e?(): number };
+var v: { e??(): number };

[vjeux]

If I remove all the handling of questionToken, there's the issues mentioned above and

-var x: { [A in keyof B]?: any };
+var x: { [A in keyof B]: any };

I'm not really sure if that's intended

[soda0289]

QuestionToken should not a appear that is a left over from typescript ast. I can change it so that optional: true appears on the TSPropertySignatureNode.

[vjeux]

Right now we have code that handles questionToken in TSMethodSignature, TSMappedType and TSPropertySignature if that helps.

[soda0289]

@vjeux Can you give this branch a try: fix-computed-property

It's a breaking change. We use the property key instead of name and params instead of parameters. But we do set the computed and optional on the TSPropertySignature instead of the inside the key/name property.

[vjeux]

Let me try!

[vjeux]

It seems to fix all the issues I outlined above. I'm still working through all the fatals it introduces, give me a few more minutes :)

[vjeux]

With the branch, it seems like public is missing from

interface I {
  public [a: string]: number;
}

{ type: 'TSInterfaceBody',
  body:
   [ { type: 'TSIndexSignature',
       range: [Object],
       loc: [Object],
       index: [Object],
       typeAnnotation: [Object] } ],
  range: [ 12, 45 ],
  loc: { start: { line: 1, column: 12 }, end: { line: 3, column: 1 } } }

{ type: 'TSIndexSignature',
  range: [ 16, 43 ],
  loc: { start: { line: 2, column: 2 }, end: { line: 2, column: 29 } },
  index:
   { type: 'Identifier',
     range: [ 24, 25 ],
     loc: { start: [Object], end: [Object] },
     name: 'a',
     typeAnnotation:
      { type: 'TypeAnnotation',
        loc: [Object],
        range: [Object],
        typeAnnotation: [Object] } },
  typeAnnotation:
   { type: 'TypeAnnotation',
     loc: { start: [Object], end: [Object] },
     range: [ 36, 42 ],
     typeAnnotation: { type: 'TSNumberKeyword', range: [Object], loc: [Object] } } }

[vjeux]

Same for readonly

interface I {
  readonly [a: string]: number;
}

[vjeux]

It's also missing initializers

interface Foo {
  bar: number = 5;
}

{ type: 'TSInterfaceBody',
  body:
   [ { type: 'TSPropertySignature',
       range: [Object],
       loc: [Object],
       optional: false,
       computed: false,
       key: [Object],
       typeAnnotation: [Object] } ],
  range: [ 14, 36 ],
  loc: { start: { line: 1, column: 14 }, end: { line: 3, column: 1 } } }

{ type: 'TSPropertySignature',
  range: [ 18, 34 ],
  loc: { start: { line: 2, column: 2 }, end: { line: 2, column: 18 } },
  optional: false,
  computed: false,
  key:
   { type: 'Identifier',
     range: [ 18, 21 ],
     loc: { start: [Object], end: [Object] },
     name: 'bar' },
  typeAnnotation:
   { type: 'TypeAnnotation',
     loc: { start: [Object], end: [Object] },
     range: [ 23, 29 ],
     typeAnnotation: { type: 'TSNumberKeyword', range: [Object], loc: [Object] } } }

[soda0289]

Thanks, will update the branch

[vjeux]

Okay, those are the only things that are breaking :) I was able to tweak the rendering output of prettier to make everything else pass again!

[soda0289]

Just push new commit to branch. It adds the properties accessibility, export, and static to TSPropertySignature, TSIndexSignature, and TSMethodSignature. Also add the property initializer to TSPropertySignature

Let me know if I'm missing anything or if AST doesn't make sense, still need to add tests for these new properties

[vjeux]

Everything passes with 2d09fb1 !

[vjeux]

Okay, so once that PR lands and #265, we'll be all green from prettier perspective! Thanks for driving all this :)

[soda0289]

Once we get a minor release out we can start merging in the breaking changes.