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

Fix type definitions to fully support Typescript #6939

Merged

Conversation

dpoindexter
Copy link
Contributor

Q                       A
Fixed Issues? Fixes #6933
Patch: Bug Fix? Yes
Major: Breaking Change? No
Minor: New Feature? No
Tests Added + Pass? Yes
Documentation PR No
Any Dependency Changes? No
License MIT

This addresses missing or incorrect Typescript types in other type definitions.

For core, experimental, and ES2015 types, any type definition referencing a Flow node type (for example, the typeParameters property of the ArrowFunctionExpression node type) now also allows the equivalent Typescript node type. (In the case of ArrowFunctionExpression.typeParameters, allowed types became TypeParameterDeclaration | TSTypeParameterDeclaration | Noop.

For Typescript types, any type definition referencing a Flow node type replaced that reference with the equivalent Typescript node type. For example, in TSInterfaceDeclaration.typeParameters, allowed types became TSTypeParameterDeclaration rather than TypeParameterDeclaration.

@babel-bot
Copy link
Collaborator

babel-bot commented Nov 30, 2017

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

@hzoo hzoo requested a review from a user November 30, 2017 20:15
@existentialism existentialism added area: typescript PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Dec 1, 2017
@@ -291,7 +294,11 @@ export const functionCommon = {
optional: true,
},
typeParameters: {
validate: assertNodeType("TypeParameterDeclaration", "Noop"),
validate: assertNodeType(
Copy link
Member

Choose a reason for hiding this comment

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

Guess one thing to think about, is that this change will make tSDeclareMethod and tSDeclareFunction (which spread in functionCommon) accept TypeParameterDeclaration.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch -- I missed that. Would you rather override the property for the TS types, or compose more granularly?

defineType("TSDeclareFunction", {
  ...functionCommon,
  returnType {
    ...functionCommon.returnType
    validate: ...etc
  },
  typeParameters: {
    ...functionCommon.typeParameters,
    validate: ...etc.
  }
});

vs.

export const functionCommon = {
  // everything there now but returnType and typeAnnotation
}

// only used for core types that need to accept either Flow or TS nodes
const functionCommonWithTypes = {
  returnType: {},
  typeAnnotation: {}
}

I didn't see other examples of overriding spread properties in definintions -- if that's generally the case, then the second approach might be more consistent with the rest of the code.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah let's go with the second 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@existentialism It's not immediately obvious because I squashed the commit, but this has been addressed.

I split the type annotations out of functionCommon into functionTypeAnnotationCommon, and spread both objects on the relevant core/es2015 types.

functionDeclarationCommon and classMethodOrDeclareMethodCommon still spread just functionCommon. Types that use those now also spread either functionTypeAnnotationCommon or tSFunctionTypeAnnotationCommon as appropriate.

@dpoindexter dpoindexter force-pushed the fix/typescript-type-definitions branch 2 times, most recently from 19e40bd to 69db29b Compare December 4, 2017 17:00
@dpoindexter dpoindexter force-pushed the fix/typescript-type-definitions branch from 69db29b to 3bba280 Compare December 4, 2017 23:01
@existentialism existentialism merged commit 12ac1bc into babel:master Dec 8, 2017
@existentialism
Copy link
Member

@dpoindexter thanks again!

@lock lock bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Oct 5, 2019
@lock lock bot locked as resolved and limited conversation to collaborators Oct 5, 2019
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: types PR: Bug Fix 🐛 A type of pull request used for our changelog categories
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cannot build Typescript AST
4 participants