Skip to content

Class emit for ES6 #2333

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

Merged
merged 25 commits into from
Mar 17, 2015
Merged

Class emit for ES6 #2333

merged 25 commits into from
Mar 17, 2015

Conversation

yuit
Copy link
Contributor

@yuit yuit commented Mar 12, 2015

Emit classDeclaration natively in ES6. This pull request also set parsing-context for class declaration to be strict mode code if target is es6.

Todo: I will update existed test-cases in ES6 conformance which contains class declaration

}
else {
write("super");
Debug.assert(languageVersion < ScriptTarget.ES6)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure you really need that.. it is the else of the if statement with the same condition. just looks like duplication to me,

emitTrailingComments(member);
}
else if (member.kind === SyntaxKind.GetAccessor || member.kind === SyntaxKind.SetAccessor) {
var accessors = getAllAccessorDeclarations(node.members, <AccessorDeclaration>member);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not convinced you need to get all the accessors and emit them together. I would just have one emit method, for methods and accessors, and just emit the word "get" or "set" if you need to, the same way you emit "static".

@DanielRosenwasser DanielRosenwasser changed the title Emit class Class emit for ES6 Mar 16, 2015
@DanielRosenwasser
Copy link
Member

I've renamed this PR so that we can find it more easily down the line.

@@ -3187,7 +3193,9 @@ module ts {
}
else {
write("(");
emitCommaList(node.arguments);
if (node.arguments.length) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually the only place we do a length check before calling emitCommaList. I don't feel that strongly about it, but it's one of those things that's a little odd to see out of place.

Yui T added 6 commits March 16, 2015 14:28
Conflicts:
	src/compiler/checker.ts
	src/compiler/diagnosticInformationMap.generated.ts
	src/compiler/diagnosticMessages.json
	src/compiler/emitter.ts
	src/compiler/parser.ts
@@ -157,6 +157,7 @@ module ts {
Catch_clause_variable_cannot_have_an_initializer: { code: 1197, category: DiagnosticCategory.Error, key: "Catch clause variable cannot have an initializer." },
An_extended_Unicode_escape_value_must_be_between_0x0_and_0x10FFFF_inclusive: { code: 1198, category: DiagnosticCategory.Error, key: "An extended Unicode escape value must be between 0x0 and 0x10FFFF inclusive." },
Unterminated_Unicode_escape_sequence: { code: 1199, category: DiagnosticCategory.Error, key: "Unterminated Unicode escape sequence." },
A_computed_property_name_cannot_reference_a_static_property: { code: 1200, category: DiagnosticCategory.Error, key: "A computed property name cannot reference a static property" },
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you do not need this any more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. It will be removed in the upcoming commit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

also the tests.

yuit added a commit that referenced this pull request Mar 17, 2015
@yuit yuit merged commit c4cb3e3 into master Mar 17, 2015
@yuit yuit deleted the emitClass branch March 17, 2015 00:40
@microsoft microsoft locked and limited conversation to collaborators Jun 18, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants