-
Notifications
You must be signed in to change notification settings - Fork 2
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
Add support for TypeScript type annotations #4
Conversation
…le and function declarations as well as call and arrow expressions. Added question token record to object properties. Stubbed out interfaces and type aliases.
…and CallExpressionE.
… mostly as documentation hints. These can be commented out or deleted later on.
src/term-spec.js
Outdated
@@ -32,7 +32,7 @@ declare export class NamedObjectProperty extends ObjectProperty { | |||
name: PropertyName; | |||
} | |||
declare export class MethodDefinition extends NamedObjectProperty { | |||
body: FunctionBody; | |||
body?: FunctionBody; // undefined only on abstract class methods |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not ideal. There are certainly other options but I'm not sure which one is best, so I went with this "minimum impact" approach for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's better to split it out into MethodDefinition
with non-optional body
and AbstractMethodDefinition
with no body
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did this and while at it I found two bugs which I tried to fix. Note that I also had to add AbstractMethod
, AbstractGetter
and AbstractSetter
which mirror the non-abstract counterparts.
src/term-spec.js
Outdated
declare export class ImplementsClause extends HeritageClause {} | ||
|
||
declare export class IndexSignatureDeclaration extends Term { | ||
parent?: ClassExpression | ClassDeclaration | InterfaceDeclaration | TypeLiteralNode; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These parent?
members are more for documentation than anything else. They could be turned into comments or deleted altogether.
src/term-spec.js
Outdated
elements: ClassElement[]; | ||
typeParameters?: TypeParameterDeclaration[]; | ||
heritageClauses?: HeritageClause[]; | ||
elements: (ClassElement | IndexSignatureDeclaration)[]; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The IndexSignatureDeclaration
is handled as a special case since
- It cannot be static nor have an access modifier;
- It can be present on interfaces and type literals as well.
There might be a better way of dealing with it.
src/term-spec.js
Outdated
accessModifier?: any; // 'public' | 'protected' | 'private' | ||
hasReadonlyModifier: any; // boolean | ||
parameter: ParameterDeclaration; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I ended up with this special way of representing constructor arguments to avoid having access/readonly modifiers on all ParameterDeclaration
s.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@disnet Now that I'm looking at it again, it might be nicer to have ConstructorParameterDeclaration
inherit from ParameterDeclaration
.
src/term-spec.js
Outdated
|
||
// enum | ||
|
||
declare export class EnumDeclaration extends Statement { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enum support is not strictly related to type annotations, so they could be dropped from here.
// example: | ||
// `SomeClass[number]` | ||
// where | ||
// `class SomeClass { [index: number]: string }` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The comment above fails to mention that SomeClass[number]
would then essentially resolve to the string
type.
src/term-spec.js
Outdated
parameter: ParameterDeclaration; | ||
} | ||
|
||
declare export class SemicolonClassElement extends ClassElement {} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what's the point of representing semicolons in class bodies in the AST?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This sneaked in from the TypeScript source. The comment in there says the following:
For when we encounter a semicolon in a class declaration. ES6 allows these as class elements.
So it's perhaps only used for recording and later emitting these empty semicolon statments as can be seen here.
When I added it, I probably thought it had some deep significance. Shall I remove it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah lets remove it. We of course have to parse them but as long as TypeScript doesn't give any semantic meaning to the extra semis there no reason to preserve them in the AST.
…f setter parameter. Made a separate AbstractMethodDefinition term.
} | ||
|
||
declare export class DataProperty extends NamedObjectProperty { | ||
hasQuestionToken: any; // boolean |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I may have overdone it with the question tokens. I think I added them only where the TypeScript AST permits them but some of them (like this one, but also the one on FunctionDeclaration
) don't seem to make too much sense...
As discussed in sweet-js/sweet-core#742 we are going to probably need to use a reducer at the end of expansion to transform our shift-like AST into a babel AST to do codegen. Our type nodes don't need to be identical with what babel does as long as transforming ours to theirs is straightforward. That said one thing I'd definitely like to copy from babel is splitting the type nodes in |
@disnet I can certainly make an attempt to split The Babel AST seems rather undetailed when it comes to Flow type annotations - it just says I don't use Flow very much, so I'm quite ignorant. But I'm thinking I could either follow Flow documentation and create nodes for everything I see there, or I could follow Flow AST. The former is easier but may lead to imprecision and/or omissions. The latter sounds like fun since I don't know OCaml at all - the only issue may be that it will take a while until I'm done with it. Another question: Is any sort of structural sharing across |
…ameterDeclaration to make them (structurally) more alike
Ah yeah, the babylon spec is missing all the flow detail. We could probably figure it out from reading the plugin that generates the nodes though.
I think no. Yeah we will probably have some duplication in the spec but that's fine I think; having them separate makes it easier if flow or ts evolves and forces a change to their respective AST. btw, as long as we split the nodes (prefix ts specific nodes with |
I'm sorry for the long delay. I added the I'm not aware of any outstanding issues in this PR - I'll continue working on it if/when I get more feedback. As for Flow types, I'll take a look at the babel plugin and I can start adding them to a separate fork (which builds on the work here) within the following week. |
No worries, this is open source. It takes how long it takes 😄 I'll try and do a review soon and then we can get it merged and published under a pre-release tag. I suspect we'll find tweaks we want to make as we go about updating the parser. |
src/term-spec.js
Outdated
body: FunctionBody; | ||
} | ||
|
||
declare export class AbstractMethodDefinition extends Term { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this be TsAbstractMethodDefinition
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that makes a lot of sense. The change is already in.
src/term-spec.js
Outdated
name: PropertyName; | ||
binding?: ObjectAssignmentTarget | ArrayAssignmentTarget | AssignmentTargetIdentifier | MemberAssignmentTarget | AssignmentTargetWithDefault; | ||
} | ||
|
||
|
||
|
||
// class | ||
|
||
declare export class ClassExpression extends Expression { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love renaming super
to heritageClauses
. How about we split the TS stuff out into ClassExpression
and TsClassExpression
(with shared attributes in ClassExpressionBase
)?
Alternatively we could type it as super: Expression | HeritageClause[]
but in a given AST it will only ever be one or the other so it's nice to keep that clear with two different nodes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made the split of both ClassExpression
and ClassDeclaration
. Once that was done, it also seemed natural to split ClassElement
, so I did that, too. The nice thing about the result is that it is backwards-compatible with the previous version of classes.
I also added ClassExpressionBase
, ClassDeclarationBase
and ClassElementBase
- they only hold one field each but are perhaps still more preferable than duplicating those fields (and later triplicating for Flow*
nodes).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! I'm going to merge but hold off on publishing until we get experience with it while implementing the changes in core.
What does the merge of this pull request mean for sweet.js? Will I soon be able to run Thanks! |
That's the plan! |
Excellent! Any idea when it will be ready?
|
When it’s ready 😜
I personally won’t have time to work on it for a while (life etc.) but am
happy to review PRs.
…On Mon, Oct 2, 2017 at 5:39 PM Colin Caine ***@***.***> wrote:
Excellent! Any idea when it will be ready?
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#4 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAAJOop_2Or11lRwyVFhsgvmtBsOCYXxks5soYJAgaJpZM4O8lms>
.
|
@disnet Can you explain what's missing? (I know how frontends and Scheme macros work in general, but not any particulars about Sweet). I understand this PR added the AST definitions; what remains aside from a parser and an emitter in sweet-core? |
The changes are adaptations of TypeScript's AST nodes. The most notable differences are:
MethodDefinition
is aNamedObjectProperty
but not aClassElement
as that would require multiple inheritance; composition is used insteadIndexSignatureDeclaration
is treated as a special case since in TypeScript it is the onlyTypeElement
that is also aClassElement
- perhaps this should be handled differentlyMissingDeclaration