Skip to content
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

Support TypeScript 4.3 get/set type members #13089

Conversation

sosukesuzuki
Copy link
Member

Q                       A
Fixed Issues? Fixes #13071
Minor: New Feature? Y
Tests Added + Pass? Yes
License MIT

@sosukesuzuki sosukesuzuki added PR: New Feature 🚀 A type of pull request used for our changelog categories pkg: generator area: typescript pkg: types labels Apr 2, 2021
@babel-bot
Copy link
Collaborator

babel-bot commented Apr 2, 2021

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/45139/

@codesandbox-ci
Copy link

codesandbox-ci bot commented Apr 2, 2021

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit 64e389b:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

packages/babel-parser/src/plugins/typescript/index.js Outdated Show resolved Hide resolved
@@ -609,6 +656,7 @@ export default (superClass: Class<Parser>): Class<Parser> =>
return idx;
}

this.tsParseMethodSignatureKind(node);
this.parsePropertyName(node, /* isPrivateNameAllowed */ false);
Copy link
Member

Choose a reason for hiding this comment

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

Instead of using lookahead (which is expensive) in tsParseMethodSignatureKind, can we:

  • Call this.parsePropertyName
  • If not computed, not private, does not have type parameters, and the identifier name is get or set:
    • If tsNextTokenCanFollowModifier, then we are parsing an accessor and we call this.parsePropertyName again
    • Otherwise, it's a method/property named get or set
  • Otherwise, it's a method/property

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah we parse class accessors using the same strategy mentioned above.

Copy link
Member Author

Choose a reason for hiding this comment

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

What the AST babel should return for below code?

interface Foo {
    set foo;
}

In current behaviour, two properties named set and foo are returned (with recoverable error Missing semicolon). If we should keep it, we should detect if the property is a method signature after first this.parsePropertyName call. But then the token is the name of the property, so I don't know how to do that without lookahead...

Copy link
Member

Choose a reason for hiding this comment

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

It's ok to throw "Expected (":

  • It's what we already do for
    class Foo {
      set foo
    }
  • It's more likely that someone didn't finish writing a setter, than putting two properties on the same line without ; and that the name of the first one was exactly set.

@existentialism existentialism mentioned this pull request Apr 6, 2021
9 tasks
@nicolo-ribaudo
Copy link
Member

Could you rebase this on top of #13098, to avoid the merge conflicts? 🙏

@sosukesuzuki sosukesuzuki changed the base branch from main to feat-7.14.0/parser-updates April 7, 2021 05:12
Copy link
Contributor

@JLHwung JLHwung left a comment

Choose a reason for hiding this comment

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

Can you add the following test cases?

// 'get' and 'set' accessors cannot declare 'this' parameters.
interface Foo {
  get bar(this: Foo);
  set bar(this: Foo);
}
// A 'set' accessor cannot have an optional parameter.
interface Foo {
  set bar(v?);
}
// A 'set' accessor cannot have rest parameter.
interface Foo {
  set bar(...v);
}

@nicolo-ribaudo
Copy link
Member

I rebased feat-7.14.0/parser-updates. Could you rebase this PR removing the commits already in feat-7.14.0/parser-updates? 🙏

@nicolo-ribaudo
Copy link
Member

Thanks!

@nicolo-ribaudo nicolo-ribaudo merged commit f2ca928 into babel:feat-7.14.0/parser-updates Apr 12, 2021
@sosukesuzuki sosukesuzuki deleted the ts4.3-setter-getter-types branch April 13, 2021 01:36
nicolo-ribaudo added a commit that referenced this pull request Apr 13, 2021
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
JLHwung pushed a commit that referenced this pull request Apr 16, 2021
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
JLHwung pushed a commit that referenced this pull request Apr 17, 2021
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
JLHwung pushed a commit that referenced this pull request Apr 19, 2021
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
nicolo-ribaudo added a commit that referenced this pull request Apr 20, 2021
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
nicolo-ribaudo added a commit that referenced this pull request Apr 20, 2021
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
nicolo-ribaudo added a commit that referenced this pull request Apr 22, 2021
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
nicolo-ribaudo added a commit that referenced this pull request Apr 23, 2021
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
nicolo-ribaudo added a commit that referenced this pull request Apr 26, 2021
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
nicolo-ribaudo added a commit that referenced this pull request Apr 28, 2021
Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
@github-actions github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Jul 13, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 13, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area: typescript outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: generator pkg: types PR: New Feature 🚀 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support new TypeScript syntax - get/set in non-class scopes
4 participants