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

transform-spread: create TS types (not Flow) when using TS #11378

Merged
merged 11 commits into from Apr 15, 2020
Merged

transform-spread: create TS types (not Flow) when using TS #11378

merged 11 commits into from Apr 15, 2020

Conversation

Beraliv
Copy link
Contributor

@Beraliv Beraliv commented Apr 4, 2020

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

@@ -0,0 +1,16 @@
import { TSUnionType } from "../generated";
import removeTypeDuplicates from "../../modifications/typescript/removeTypeDuplicates";
Copy link
Contributor Author

@Beraliv Beraliv Apr 4, 2020

Choose a reason for hiding this comment

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

@Beraliv
Copy link
Contributor Author

Beraliv commented Apr 4, 2020

🔄 Still working on fixing created test case

● babel-traverse/type reference › input

TypeError: /Users/beraliv/Documents/Code/babel/packages/babel-traverse/test/fixtures/type-reference/input.ts: Property types[0] of UnionTypeAnnotation expected node to be of a type ["FlowType"] but instead got "TSTypeAnnotation"

  130 |     }
  131 | 
> 132 |     throw new TypeError(`Property ${key} of ${node.type} expected node to be of a type ${JSON.stringify(types)} but instead got ${JSON.stringify(val && val.type)}`);
      |           ^
  133 |   }
  134 | 
  135 |   validate.oneOfNodeTypes = types;

  at validate (packages/babel-types/lib/definitions/utils.js:132:11)
  at validator (packages/babel-types/lib/definitions/utils.js:103:7)
  at Object.validate (packages/babel-types/lib/definitions/utils.js:229:7)
  at validateField (packages/babel-types/lib/validators/validate.js:24:9)
  at validate (packages/babel-types/lib/validators/validate.js:17:3)
  at builder (packages/babel-types/lib/builders/builder.js:38:27)
  at UnionTypeAnnotation (packages/babel-types/lib/builders/generated/index.js:757:31)
  at Object.createUnionTypeAnnotation (packages/babel-types/lib/builders/flow/createUnionTypeAnnotation.js:20:47)
  at NodePath.ConditionalExpression (packages/babel-traverse/lib/path/inference/inferers.js:123:12)
  at NodePath._getTypeAnnotation (packages/babel-traverse/lib/path/inference/index.js:57:20)

@Beraliv
Copy link
Contributor Author

Beraliv commented Apr 5, 2020

🔄 Still working on fixing created test case

Now I have a different error:

TypeError: /Users/beraliv/Documents/Code/babel/packages/babel-traverse/test/fixtures/type-reference/input.ts: Property types[0] of TSUnionType expected 
node to be of a type ["TSType"] but instead got "TSTypeAnnotation"
The tree example for bug case
[ Node {
          type: 'TSTypeAnnotation',
          start: 77,
          end: 87,
          loc: SourceLocation { start: [Position], end: [Position] },
          typeAnnotation:
           Node {
             type: 'TSArrayType',
             start: 79,
             end: 87,
             loc: [SourceLocation],
             elementType: [Node],
             leadingComments: undefined,
             innerComments: undefined,
             trailingComments: undefined },
          leadingComments: undefined,
          innerComments: undefined,
          trailingComments: undefined },
        Node {
          type: 'TSTypeAnnotation',
          start: 118,
          end: 128,
          loc: SourceLocation { start: [Position], end: [Position] },
          typeAnnotation:
           Node {
             type: 'TSArrayType',
             start: 120,
             end: 128,
             loc: [SourceLocation],
             elementType: [Node],
             leadingComments: undefined,
             innerComments: undefined,
             trailingComments: undefined },
          leadingComments: undefined,
          innerComments: undefined,
          trailingComments: undefined } ]

How can I move from TSTypeAnnotation to TSArrayType?

@Beraliv
Copy link
Contributor Author

Beraliv commented Apr 5, 2020

Now I have a different error:

TypeError: /Users/beraliv/Documents/Code/babel/packages/babel-traverse/test/fixtures/type-reference/input.ts: Property types[0] of TSUnionType expected 
node to be of a type ["TSType"] but instead got "TSTypeAnnotation"

✅ fixed test case

continue;
}

// TODO: add generic types
Copy link
Contributor Author

Choose a reason for hiding this comment

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

⚠️ I need clarification here

First I tried TSTypeParameter e0f3fc2#diff-31b76153974cd0302007f05a570dfde6R49

Now I'm sure it does NOT cover all cases

]);
];

if (argumentTypes.every(t.isTSTypeAnnotation)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Beraliv Beraliv marked this pull request as ready for review April 7, 2020 14:46
@Beraliv Beraliv changed the title Fix transform-spread + typescript builds Flow annotation instead of TypeScript transform-spread: 🧪 use createTSUnionType for transform-spread instead of createUnionTypeAnnotation Apr 7, 2020
@Beraliv Beraliv changed the title transform-spread: 🧪 use createTSUnionType for transform-spread instead of createUnionTypeAnnotation transform-spread: 🧪 use createTSUnionType instead of createUnionTypeAnnotation Apr 7, 2020
@Beraliv
Copy link
Contributor Author

Beraliv commented Apr 8, 2020

@JLHwung please have a look at it

@nicolo-ribaudo nicolo-ribaudo added area: typescript pkg: types PR: Bug Fix 🐛 A type of pull request used for our changelog categories labels Apr 8, 2020
@JLHwung JLHwung self-requested a review April 8, 2020 14:31
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.

This PR is good for me as a bugfix.

On reviewing this I realize the whole path inference module is assuming the flow type annotation. Since we add TS type support later, we should revisit later, isolate different implementations on flow/TS and hook with the other general inferrer routines.

@Beraliv
Copy link
Contributor Author

Beraliv commented Apr 8, 2020

@JLHwung fixed all comments ✅

}

// Analogue of FlowBaseAnnotation
if (isTSBaseType(node)) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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


const nodeType = node.type;
if (
nodeType === "TSBaseType" ||
Copy link
Contributor Author

Choose a reason for hiding this comment

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

}

if (t.isTSTypeAnnotation(types[0])) {
return t.createTSUnionType(types);
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 technically a breaking change: it makes the next @babel/traverse version incompatible with @babel/types <= 7.9.5, because it doesn't contain this function.

We should use t.createTSUnionType?.(types) instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh, this is true, thanks for mentioning that

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@nicolo-ribaudo it will change the behaviour as returns UnionTypeAnnotation and now it returns undefined

I added if-check in 5e9c920

Copy link
Member

@nicolo-ribaudo nicolo-ribaudo left a comment

Choose a reason for hiding this comment

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

Thanks!

@nicolo-ribaudo
Copy link
Member

Tests pass locally.

@nicolo-ribaudo nicolo-ribaudo changed the title transform-spread: 🧪 use createTSUnionType instead of createUnionTypeAnnotation transform-spread: create TS types (not Flow) when using TS Apr 15, 2020
@nicolo-ribaudo nicolo-ribaudo merged commit 6b8f6ab into babel:master Apr 15, 2020
@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 16, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 16, 2020
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.

transform-spread + typescript builds Flow annotation instead of TypeScript
4 participants