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 ES2017 target #11407

Merged
merged 15 commits into from Oct 15, 2016

Conversation

Projects
None yet
6 participants
@andrejbaran
Copy link
Contributor

andrejbaran commented Oct 5, 2016

Hi there,
please consider this as a start for ES2017 target. Fixes #10768.

10/13/2016: This PR also includes a consolidation of naming of ES versions to reflect the official naming scheme (past es5) ES_YYYY_. This is a change through out the codebase.

@msftclas

This comment has been minimized.

Copy link

msftclas commented Oct 5, 2016

Hi @andrejbaran, I'm your friendly neighborhood Microsoft Pull Request Bot (You can call me MSBOT). Thanks for your contribution!
You've already signed the contribution license agreement. Thanks!

The agreement was validated by Microsoft and real humans are currently evaluating your PR.

TTYL, MSBOT;

@@ -262,7 +262,9 @@ namespace ts {
"es3": ScriptTarget.ES3,
"es5": ScriptTarget.ES5,
"es6": ScriptTarget.ES6,
"es8": ScriptTarget.ES8,

This comment has been minimized.

@mhegazy

mhegazy Oct 5, 2016

Contributor

Officially there is no ES8, it is ES2017, so i would not add ES8 here or anywhere else. i would just leave it to ES2017

This comment has been minimized.

@andrejbaran

andrejbaran Oct 12, 2016

Contributor

done

@@ -262,7 +262,9 @@ namespace ts {
"es3": ScriptTarget.ES3,
"es5": ScriptTarget.ES5,
"es6": ScriptTarget.ES6,
"es8": ScriptTarget.ES8,

This comment has been minimized.

@mhegazy

mhegazy Oct 5, 2016

Contributor

Can you also add ES2016. it is a bit funny that we skip one ES 2016 (though not very useful but should be there for consistency).

This comment has been minimized.

@andrejbaran

andrejbaran Oct 12, 2016

Contributor

done

@@ -262,7 +262,9 @@ namespace ts {
"es3": ScriptTarget.ES3,
"es5": ScriptTarget.ES5,
"es6": ScriptTarget.ES6,
"es8": ScriptTarget.ES8,

This comment has been minimized.

@mhegazy

mhegazy Oct 5, 2016

Contributor

you also need to update getDefaultLibFileName to inject the right library based on the target.

This comment has been minimized.

@andrejbaran

andrejbaran Oct 12, 2016

Contributor

done

@@ -2453,14 +2464,28 @@ namespace ts {
* @param node The await expression node.
*/
function visitAwaitExpression(node: AwaitExpression): Expression {

This comment has been minimized.

@mhegazy

mhegazy Oct 6, 2016

Contributor

async/await should be split into its own transform, es2017.ts, then we do not need to run it in this case, just like ES7 transform.

This comment has been minimized.

@andrejbaran

andrejbaran Oct 12, 2016

Contributor

done

@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented Oct 6, 2016

thanks @andrejbaran for the contribution. a few comments. also cc @rbuckton

@yortus

This comment has been minimized.

Copy link
Contributor

yortus commented Oct 6, 2016

In theory trailing commas should also be preserved in the output for fidelity with target: es2017. Is that being too pedantic? I can't imagine any scenarios where it would matter.

@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented Oct 6, 2016

In theory trailing commas should also be preserved in the output for fidelity with target: es2017. Is that being too pedantic? I can't imagine any scenarios where it would matter.

yes, but the implementation today would make this a bit hard to achieve, not sure if there is much value though.

@yortus

This comment has been minimized.

Copy link
Contributor

yortus commented Oct 6, 2016

I can't imagine any scenarios where it would matter.

Thinking about it more, at least one scenario comes to mind: ES feature detection. Not sure how useful that is in practice (NB the linked repo has 0 stars). Probably safe to ignore until someone raises an actual issue.

@andrejbaran

This comment has been minimized.

Copy link
Contributor

andrejbaran commented Oct 6, 2016

Thanks for the review @mhegazy i'll get back to it tonight and will let you know

andrejbaran added some commits Oct 12, 2016

Don't use es8. Add es2016 target.
Rename es7 to es2016. Update getDefaultLibFileName for new targets.
@andrejbaran

This comment has been minimized.

Copy link
Contributor

andrejbaran commented Oct 12, 2016

@mhegazy It turned out to be little more complicated than I anticipated, please review again. I have also renamed es7 to es2016 for consistency? (maybe es6 should be renamed too, dunno but I believe es2015 is preferred)

@sandersn
Copy link
Member

sandersn left a comment

Looks good except for the binder marking async/await as Typescript even though I think they should just be ES2017 now. @rbuckton can you take a look at that?

@@ -2656,7 +2666,7 @@ namespace ts {

// If a FunctionDeclaration is async, then it is TypeScript syntax.
if (modifierFlags & ModifierFlags.Async) {
transformFlags |= TransformFlags.AssertTypeScript;
transformFlags |= TransformFlags.AssertTypeScript | TransformFlags.AssertES2017;

This comment has been minimized.

@sandersn

sandersn Oct 12, 2016

Member

is this comment still accurate? Or are async functions valid ES2017 too? If so, they probably shouldn't be marked as typescript.

This comment has been minimized.

@andrejbaran

andrejbaran Oct 12, 2016

Contributor

According to this async/await is finished and will be part of es2017, so I believe you have a valid point there.

This comment has been minimized.

@andrejbaran

andrejbaran Oct 13, 2016

Contributor

done

@@ -2687,7 +2697,7 @@ namespace ts {

// An async function expression is TypeScript syntax.
if (modifierFlags & ModifierFlags.Async) {
transformFlags |= TransformFlags.AssertTypeScript;
transformFlags |= TransformFlags.AssertTypeScript | TransformFlags.AssertES2017;

This comment has been minimized.

@sandersn

sandersn Oct 12, 2016

Member

same comment as for async function declarations

This comment has been minimized.

@andrejbaran

andrejbaran Oct 13, 2016

Contributor

done

@@ -2856,14 +2866,18 @@ namespace ts {
let excludeFlags = TransformFlags.NodeExcludes;

switch (kind) {
case SyntaxKind.AsyncKeyword:
case SyntaxKind.AwaitExpression:
// Typescript async/await are ES2017 features

This comment has been minimized.

@sandersn

sandersn Oct 12, 2016

Member

same comment as elsewhere. probably should just be AssertES2017

This comment has been minimized.

@andrejbaran

andrejbaran Oct 13, 2016

Contributor

done

/// <reference path="../visitor.ts" />

/*@internal*/
namespace ts {

This comment has been minimized.

@sandersn

sandersn Oct 12, 2016

Member

@rbuckton do you mind taking a look at least at this file and ts.ts?

@@ -386,7 +379,7 @@ namespace ts {
return visitEnumDeclaration(<EnumDeclaration>node);

case SyntaxKind.AwaitExpression:
// TypeScript 'await' expressions must be transformed.
// Typescript ES2017 async/await are handled by ES2017 transformer

This comment has been minimized.

@sandersn

sandersn Oct 12, 2016

Member

I don't think you will even need this case if you mark async/await as ES2017 only

This comment has been minimized.

@andrejbaran

andrejbaran Oct 13, 2016

Contributor

removed

if (isAsyncFunctionLike(node)) {
return <FunctionBody>transformAsyncFunctionBody(node);
}
// if (isAsyncFunctionLike(node) && languageVersion < ScriptTarget.ES2017) {

This comment has been minimized.

@sandersn

sandersn Oct 12, 2016

Member

delete this and the next comment section

This comment has been minimized.

@andrejbaran

andrejbaran Oct 12, 2016

Contributor

done

@andrejbaran andrejbaran changed the title Add ES8/ES2017 target Add ES2017 target Oct 12, 2016

@@ -2555,6 +2555,11 @@ namespace ts {
// extends clause of a class.
let transformFlags = subtreeFlags | TransformFlags.AssertES6;

// propagate ES2017

This comment has been minimized.

@rbuckton

rbuckton Oct 13, 2016

Member

Any TransformFlags of a subtree that should propagate should already be set in subtreeFlags. There should be no need to explicitly propagate.

This comment has been minimized.

@andrejbaran

andrejbaran Oct 13, 2016

Contributor

done

@@ -2595,6 +2600,11 @@ namespace ts {
transformFlags |= TransformFlags.AssertTypeScript;
}

// Async MethodDeclaration is ES2017
if (modifierFlags & ModifierFlags.Async) {

This comment has been minimized.

@rbuckton

rbuckton Oct 13, 2016

Member

Since async is ES2017, the test at line 2598 is now incorrect.

This comment has been minimized.

@andrejbaran

andrejbaran Oct 13, 2016

Contributor

done

return undefined;

case SyntaxKind.AwaitExpression:
// Typescript 'await' expressions must be transformed for targets < ES2017.

This comment has been minimized.

@rbuckton

rbuckton Oct 13, 2016

Member

Should be ES2017 and not Typescript here

This comment has been minimized.

@andrejbaran

andrejbaran Oct 13, 2016

Contributor

done

return visitAwaitExpression(<AwaitExpression>node);

case SyntaxKind.MethodDeclaration:
// TypeScript method declarations may be 'async'

This comment has been minimized.

@rbuckton

rbuckton Oct 13, 2016

Member

Should be ES2017 and not Typescript here

This comment has been minimized.

@andrejbaran

andrejbaran Oct 13, 2016

Contributor

done

return visitMethodDeclaration(<MethodDeclaration>node);

case SyntaxKind.FunctionDeclaration:
// TypeScript function declarations may be 'async'

This comment has been minimized.

@rbuckton

rbuckton Oct 13, 2016

Member

Should be ES2017 and not Typescript here

This comment has been minimized.

@andrejbaran

andrejbaran Oct 13, 2016

Contributor

done

ContainsES2017 = 1 << 5,
ES2016 = 1 << 6,
ContainsES2016 = 1 << 7,
ES6 = 1 << 8,

This comment has been minimized.

@rbuckton

rbuckton Oct 13, 2016

Member

Please change to ES2015 for consistency.

This comment has been minimized.

@andrejbaran

andrejbaran Oct 13, 2016

Contributor

done

ES2016 = 1 << 6,
ContainsES2016 = 1 << 7,
ES6 = 1 << 8,
ContainsES6 = 1 << 9,

This comment has been minimized.

@rbuckton

rbuckton Oct 13, 2016

Member

Please change to ContainsES2015 for consistency.

This comment has been minimized.

@andrejbaran

andrejbaran Oct 13, 2016

Contributor

done


HasComputedFlags = 1 << 29, // Transform flags have been computed.

// Assertions
// - Bitmasks that are used to assert facts about the syntax of a node and its subtree.
AssertTypeScript = TypeScript | ContainsTypeScript,
AssertJsx = Jsx | ContainsJsx,
AssertES7 = ES7 | ContainsES7,
AssertES2017 = ES2017 | ContainsES2017,
AssertES2016 = ES2016 | ContainsES2016,
AssertES6 = ES6 | ContainsES6,

This comment has been minimized.

@rbuckton

rbuckton Oct 13, 2016

Member

Please change to AssertES2015 for consistency.

This comment has been minimized.

@andrejbaran

andrejbaran Oct 13, 2016

Contributor

done

return "lib.es2017.d.ts";
case ScriptTarget.ES2016:
return "lib.es2016.d.ts";
case ScriptTarget.ES6:

This comment has been minimized.

@rbuckton

rbuckton Oct 13, 2016

Member

Please use ES2015 here for consistency.

This comment has been minimized.

@andrejbaran

andrejbaran Oct 13, 2016

Contributor

done

return "lib.es2017.d.ts";
case ts.ScriptTarget.ES2016:
return "lib.es2016.d.ts";
case ts.ScriptTarget.ES6:

This comment has been minimized.

@rbuckton

rbuckton Oct 13, 2016

Member

Please use ES2015 here for consistency.

This comment has been minimized.

@andrejbaran

andrejbaran Oct 13, 2016

Contributor

done

@andrejbaran

This comment has been minimized.

Copy link
Contributor

andrejbaran commented Oct 13, 2016

@mhegazy @rbuckton @sandersn Review the changes pls. I also made the change from ES6 -> ES2015 through out the code so the naming scheme for ES versions is as consistent as possible. Let me know if that's not desired as I realise it's not the purpose of this PR, but since this PR kind of kicked off the consolidation I went for it. Again let me know if you're not ok with it (since it's quite a big change)

@sandersn
Copy link
Member

sandersn left a comment

One minor comment, otherwise looks good. I'm not sure whether we need to duplicate all the async tests to have 2017 versions, but I'll let @rbuckton take a look and decide on that.

@@ -3055,7 +3055,9 @@ namespace ts {
ES5 = 1,
ES6 = 2,

This comment has been minimized.

@sandersn

sandersn Oct 13, 2016

Member

this is minor, but can you make ES2015=2 and ES6=ES2015 ?

This comment has been minimized.

@sandersn

sandersn Oct 13, 2016

Member

Or maybe even delete ES6 if it's not used any more. I can't remember if this has anything to do with command line parsing, so it might be hard to tell.

This comment has been minimized.

@andrejbaran

andrejbaran Oct 14, 2016

Contributor

The only place where ES6 is used (other than comments) is in command line/compiler options parsing.

This comment has been minimized.

@andrejbaran

andrejbaran Oct 14, 2016

Contributor

removed ES6 completely

@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented Oct 14, 2016

@rbuckton any comments?

@rbuckton

This comment has been minimized.

Copy link
Member

rbuckton commented Oct 14, 2016

I concur with @sandersn that there are a number of unnecessarily duplicated tests. I need to go through them to see which ones are unneeded. Other than that, 👍

@andrejbaran

This comment has been minimized.

Copy link
Contributor

andrejbaran commented Oct 14, 2016

@sandersn @rbuckton sorry about that, I thought it would be good to have them for es2017, but didn't find time to dig deeper.

@rbuckton
Copy link
Member

rbuckton left a comment

Most of the tests are fine as they also are vetting the emit for --target ES2017. I have marked some tests that are specific to either the checker or our down-level emit for ES6 that do not need to be duplicated.

I can approve this once those changes are made.

await 0;
}

const asycnArrowFunc = async (): Promise<void> => {

This comment has been minimized.

@rbuckton

rbuckton Oct 14, 2016

Member

"async"

@@ -0,0 +1,6 @@
// @target: es2017

This comment has been minimized.

@rbuckton

rbuckton Oct 14, 2016

Member

This test is unnecessary as its only purpose is to verify return type check for async functions, which is already handled by the existing tests. The reason we differ between es5/es6 is that the rules governing whether this is allowed are different when targeting es5/3, but will be the same for es6+

@@ -0,0 +1,4 @@
// @target: es2017

This comment has been minimized.

@rbuckton

rbuckton Oct 14, 2016

Member

This test is unnecessary as its only purpose is to verify the checker with respect to async functions, which is already handled by existing tests.

@@ -0,0 +1,6 @@
// @target: es2017

This comment has been minimized.

@rbuckton

rbuckton Oct 14, 2016

Member

This test is unnecessary as its only purpose is to verify the checker with respect to async functions, which is already handled by existing tests.

@@ -0,0 +1,3 @@
// @target: es2017

This comment has been minimized.

@rbuckton

rbuckton Oct 14, 2016

Member

This test is unnecessary as its only purpose is to verify the checker with respect to async functions, which is already handled by existing tests.

@@ -0,0 +1,5 @@
// @target: es2017

This comment has been minimized.

@rbuckton

rbuckton Oct 14, 2016

Member

This test isn't really needed as it was added only to ensure the __awaiter helper was added to the correct file.

@@ -0,0 +1,9 @@
// @target: es2017

This comment has been minimized.

@rbuckton

rbuckton Oct 14, 2016

Member

This test is unnecessary as its only purpose is to verify the checker with respect to async functions, which is already handled by existing tests.

@@ -0,0 +1,6 @@
// @target: es2017

This comment has been minimized.

@rbuckton

rbuckton Oct 14, 2016

Member

This test is unnecessary as its only purpose is to verify the checker with respect to async functions, which is already handled by existing tests.

@@ -0,0 +1,14 @@
// @target: es2017

This comment has been minimized.

@rbuckton

rbuckton Oct 14, 2016

Member

This test is unnecessary as its only purpose is to verify the checker with respect to async functions, which is already handled by existing tests.

@@ -0,0 +1,25 @@
// @target: es2017

This comment has been minimized.

@rbuckton

rbuckton Oct 14, 2016

Member

This test is unnecessary as its only purpose is to verify the checker with respect to async functions, which is already handled by existing tests.

@andrejbaran

This comment has been minimized.

Copy link
Contributor

andrejbaran commented Oct 14, 2016

@rbuckton done. Thanks!

@mhegazy

This comment has been minimized.

Copy link
Contributor

mhegazy commented Oct 14, 2016

thanks @andrejbaran! waiting on the travis, it has been 🐌 sloooow 🐌 today

@rbuckton

This comment has been minimized.

Copy link
Member

rbuckton commented Oct 15, 2016

Travis CI looks to be running behind. Once the build passes I'll merge the pull request. Thanks @andrejbaran for the contribution!

@rbuckton

This comment has been minimized.

Copy link
Member

rbuckton commented Oct 15, 2016

@andrejbaran My appologies, can you take a look at the merge conflict?

@andrejbaran

This comment has been minimized.

Copy link
Contributor

andrejbaran commented Oct 15, 2016

@rbuckton should be ok now

@rbuckton rbuckton merged commit 63160de into Microsoft:master Oct 15, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@mhegazy mhegazy referenced this pull request Oct 15, 2016

Closed

Add ES2016 target #5361

@andrejbaran

This comment has been minimized.

Copy link
Contributor

andrejbaran commented Oct 17, 2016

Many thanks guys!

@andrejbaran andrejbaran deleted the andrejbaran:es2017-target branch Oct 17, 2016

@mhegazy mhegazy referenced this pull request May 29, 2018

Open

Support for --target ES2018 #20463

1 of 3 tasks complete

@Microsoft Microsoft locked and limited conversation to collaborators Jun 19, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.