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
Conversation
Beraliv
commented
Apr 4, 2020
•
edited
edited
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"; |
There was a problem hiding this comment.
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
packages/babel-types/src/modifications/typescript/removeTypeDuplicates.js
Outdated
Show resolved
Hide resolved
packages/babel-types/src/modifications/typescript/removeTypeDuplicates.js
Outdated
Show resolved
Hide resolved
🔄 Still working on fixing created test case ● babel-traverse/type reference › input
|
Now I have a different error:
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 |
✅ fixed test case |
packages/babel-types/src/modifications/typescript/removeTypeDuplicates.js
Outdated
Show resolved
Hide resolved
continue; | ||
} | ||
|
||
// TODO: add generic types |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First I tried TSTypeParameter
e0f3fc2#diff-31b76153974cd0302007f05a570dfde6R49
Now I'm sure it does NOT cover all cases
]); | ||
]; | ||
|
||
if (argumentTypes.every(t.isTSTypeAnnotation)) { |
There was a problem hiding this comment.
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
LogicalExpression
- https://github.com/babel/babel/pull/11378/files#diff-6c956a118dc9996dff6b731e42856479R85getTypeAnnotationBindingConstantViolations
- https://github.com/babel/babel/pull/11378/files#diff-1fce3c9f392392fe193c399ad4a97b65R99getConditionalAnnotation
- https://github.com/babel/babel/pull/11378/files#diff-1fce3c9f392392fe193c399ad4a97b65R212
@JLHwung please have a look at it |
packages/babel-traverse/src/path/inference/inferer-reference.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this 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.
@JLHwung fixed all comments ✅ |
} | ||
|
||
// Analogue of FlowBaseAnnotation | ||
if (isTSBaseType(node)) { |
There was a problem hiding this comment.
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
|
||
const nodeType = node.type; | ||
if ( | ||
nodeType === "TSBaseType" || |
There was a problem hiding this comment.
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
} | ||
|
||
if (t.isTSTypeAnnotation(types[0])) { | ||
return t.createTSUnionType(types); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
✅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Tests pass locally. |