Skip to content

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

Merged
nicolo-ribaudo merged 11 commits intobabel:masterfrom
Beraliv:master
Apr 15, 2020
Merged

transform-spread: create TS types (not Flow) when using TS#11378
nicolo-ribaudo merged 11 commits intobabel:masterfrom
Beraliv:master

Conversation

@Beraliv
Copy link
Copy Markdown
Contributor

@Beraliv Beraliv commented Apr 4, 2020 •
edited
Loading

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

Beraliv
Beraliv commented Apr 4, 2020
View reviewed changes
Beraliv
Beraliv commented Apr 4, 2020
View reviewed changes
Beraliv
Beraliv commented Apr 4, 2020
View reviewed changes
packages/babel-types/src/builders/typescript/createTSUnionType.js
@@ -0,0 +1,16 @@
import { TSUnionType } from "../generated";
import removeTypeDuplicates from "../../modifications/typescript/removeTypeDuplicates";
Copy link
Copy Markdown
Contributor Author

@Beraliv Beraliv Apr 4, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

using typescript implementation here: https://github.com/babel/babel/pull/11378/files#diff-31b76153974cd0302007f05a570dfde6R10

@Beraliv
Copy link
Copy Markdown
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
Beraliv commented Apr 4, 2020
View reviewed changes
@Beraliv
Copy link
Copy Markdown
Contributor Author

Beraliv commented Apr 5, 2020 •
edited
Loading

🔄 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
Copy Markdown
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

Beraliv
Beraliv commented Apr 5, 2020
View reviewed changes
packages/babel-types/src/modifications/typescript/removeTypeDuplicates.js
continue;
}

// TODO: add generic types
Copy link
Copy Markdown
Contributor Author

@Beraliv Beraliv Apr 5, 2020

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

Beraliv
Beraliv commented Apr 5, 2020
View reviewed changes
packages/babel-traverse/src/path/inference/inferers.js Outdated
]);
];

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

@Beraliv Beraliv Apr 5, 2020

Choose a reason for hiding this comment

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

The fix 💊

Changes are also applied for

@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
Copy Markdown
Contributor Author

Beraliv commented Apr 8, 2020 •
edited
Loading

@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
JLHwung
JLHwung reviewed Apr 8, 2020
View reviewed changes
JLHwung
JLHwung approved these changes Apr 8, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Beraliv commented Apr 8, 2020

@JLHwung fixed all comments ✅

Beraliv
Beraliv commented Apr 8, 2020
View reviewed changes
packages/babel-types/src/modifications/typescript/removeTypeDuplicates.js
}

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

@Beraliv Beraliv Apr 8, 2020

Choose a reason for hiding this comment

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

use TSBaseType here which I added before https://github.com/babel/babel/pull/11378/files#diff-0729be6aceed3c3d27b34e6226aa1509R146

Beraliv
Beraliv commented Apr 8, 2020
View reviewed changes
packages/babel-types/src/validators/generated/index.js

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

@Beraliv Beraliv Apr 8, 2020

Choose a reason for hiding this comment

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

moved logic from commit aa8c16a#diff-31b76153974cd0302007f05a570dfde6R47 here

nicolo-ribaudo
nicolo-ribaudo reviewed Apr 14, 2020
View reviewed changes
packages/babel-traverse/src/path/inference/inferer-reference.js
}

if (t.isTSTypeAnnotation(types[0])) {
return t.createTSUnionType(types);
Copy link
Copy Markdown
Member

@nicolo-ribaudo nicolo-ribaudo Apr 14, 2020

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
Copy Markdown
Contributor Author

@Beraliv Beraliv Apr 15, 2020

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
Copy Markdown
Contributor Author

@Beraliv Beraliv Apr 15, 2020

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

✅

nicolo-ribaudo
nicolo-ribaudo approved these changes Apr 15, 2020
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Member

nicolo-ribaudo commented Apr 15, 2020

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
@commit-lint commit-lint bot mentioned this pull request Apr 15, 2020
Merged
This was referenced Apr 20, 2020
Merged
Merged
@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.

Reviewers

@existentialism existentialism existentialism approved these changes

@JLHwung JLHwung JLHwung approved these changes

@nicolo-ribaudo nicolo-ribaudo nicolo-ribaudo approved these changes

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

transform-spread + typescript builds Flow annotation instead of TypeScript

4 participants

@Beraliv @nicolo-ribaudo @existentialism @JLHwung