convert @babel/traverse to TypeScript #12488
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/38265/ |
|
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 9df3894:
|
nicolo-ribaudo
added
the
Flow -> TS
zxbodya
force-pushed
the
ts-migration/babel-traverse
branch
2 times, most recently
from
fbf7978
to
1bb12e1
Compare
| @@ -23,7 +23,7 @@ const typeAnnotationInferringNodes = new WeakSet(); | |||
| * todo: split up this method | |||
| */ | |||
|
|
|||
| export function _getTypeAnnotation(): ?Object { | |||
| export function _getTypeAnnotation(): any | undefined | null { | |||
There was a problem hiding this comment.
undefined | null can be removed since any already contains all types
There was a problem hiding this comment.
indeed, it can be removed - will update the PR
(adding special case for nullable any in flow to typescript tool, so it would be updated in other places as well)
zxbodya
force-pushed
the
ts-migration/babel-traverse
branch
from
1bb12e1
to
464c316
Compare
|
Any news on it? Thanks |
zxbodya
force-pushed
the
ts-migration/babel-traverse
branch
from
464c316
to
310eccf
Compare
rebased resolving the conflict with changes in main |
| !path.scope.getBinding(name, true) && | ||
| property.isIdentifier && | ||
| !path.scope.getBinding(name) && | ||
| property.isIdentifier() && |
|
|
||
| if ( | ||
| object.isIdentifier() && | ||
| name === "String" && | ||
| !path.scope.getBinding(name, true) && |
There was a problem hiding this comment.
I doubt it should have been hasBinding(name, true), as the binding is never used later. Can you leave a todo note here?
There was a problem hiding this comment.
sure, added a comment
sounds like - this might be the case
| import * as inferers from "./inferers"; | ||
| import * as t from "@babel/types"; | ||
|
|
||
| /** | ||
| * Infer the type of the current `NodePath`. | ||
| */ | ||
|
|
||
| export function getTypeAnnotation(): Object { | ||
| export function getTypeAnnotation(): t.FlowType { |
There was a problem hiding this comment.
It could also return TSType, we can leave is as-is and refine later.
There was a problem hiding this comment.
Added t.FlowType type annotation, because in current implementation it uses t.anyTypeAnnotation() inside, so currently it probably is correct only for flow
Makefile
Outdated
| @@ -36,6 +36,7 @@ generate-tsconfig: | |||
|
|
|||
| generate-type-helpers: | |||
| $(YARN) gulp generate-type-helpers | |||
| node packages/babel-traverse/scripts/generateTypeHelpers.js | |||
There was a problem hiding this comment.
I'll push a commit to move this to gulp like the other generators.
There was a problem hiding this comment.
I went ahead and pushed some commits rather than just leaving comments to speed up merging this PR.
I'm sorry you already had to rebase it: this PR is big and while I'm not satisfied with how @babel/traverse is typed, it's already better than what we had with Flow.
We'll need to come back and improve @babel/traverse types before publishing, but it's good enough for now.
|
GH correctly detects all the files in this PR as renamed, so there is no need to merge it as two commits. |
github-actions
bot
added
the
outdated
@babel/traverse of #11578