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

convert @babel/types to typescript #12431

Merged
merged 2 commits into from Dec 10, 2020

Conversation

zxbodya
Copy link
Contributor

@zxbodya zxbodya commented Dec 1, 2020

Q                       A
License MIT

@babel/types part of #11578


EDIT by @nicolo-ribaudo

This should be merged as two different commits, to help git track file renames (useful for things like git blame):

@codesandbox-ci
Copy link

codesandbox-ci bot commented Dec 1, 2020

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 c9a67e9:

Sandbox Source
babel-repl-custom-plugin Configuration
babel-plugin-multi-config Configuration

Makefile Outdated Show resolved Hide resolved
Makefile Outdated
@@ -149,6 +148,10 @@ clean-tsconfig:
rm -f packages/*/tsconfig.{tsbuildinfo,json}
rm -f codemods/*/tsconfig.{tsbuildinfo,json}
rm -f eslint/*/tsconfig.{tsbuildinfo,json}
# revert files in git
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think we have added any tsconfig.json to git. Why we have to revert here?

@zxbodya If it is for rebasing the ts branch, you can split it in a separate commit used by that branch only.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

there is separate PR #12420 - this is needed when converting other packages, for tsconfigs that can not be generated

@babel-bot
Copy link
Collaborator

babel-bot commented Dec 1, 2020

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

@nicolo-ribaudo nicolo-ribaudo added the Flow -> TS Tracking repository migration from Flow to TS label Dec 1, 2020
@nicolo-ribaudo
Copy link
Member

nicolo-ribaudo commented Dec 4, 2020

@zxbodya I'll open a PR to your fork later today to fix the e2e test failure.

zxbodya#1

@nicolo-ribaudo
Copy link
Member

(I'm preparing a longer review)

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.

Overall, this is awesome!

export default function builder(
type: t.Node["type"],
...args: Array<any>
): any {
Copy link
Member

Choose a reason for hiding this comment

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

This should return a t.Node. Maybe even better, we can type it as

export default function builder<T extends t.Node>(
  type: T["type"],
  ...args: Array<any>
): T

export default function createTSUnionType(
typeAnnotations: Array<Object>,
): Object {
export default function createTSUnionType(typeAnnotations: Array<any>): any {
Copy link
Member

Choose a reason for hiding this comment

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

This should be typed similarly to createFlowUnionType

function createTSUnionType<T extends t.TSType>(types: [T] | Array<T>): T | t.TSUnionType

Or maybe this is enough?

function createTSUnionType(types: Array<t.TSType>): t.TSType

Copy link
Contributor Author

Choose a reason for hiding this comment

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

interesting one - when added types, found few bugs - will add fix to pr 🙈

@@ -13,6 +12,7 @@ export { default as createUnionTypeAnnotation } from "./builders/flow/createFlow
export { default as createFlowUnionType } from "./builders/flow/createFlowUnionType";
Copy link
Member

Choose a reason for hiding this comment

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

Could you mark the line above this one (createUnionTypeAnnotation) as deprecated?

Comment on lines 22 to 23
declars: Array<any>,
): any | undefined | null {
Copy link
Member

Choose a reason for hiding this comment

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

Here instead of any we should use t.Node at least.

(with flow we had some problems with t.Node so we often used Object)

export default function toBlock(node: Object, parent: Object): Object {
export default function toBlock(
node: t.Statement | t.Expression,
parent?: t.Node | null,
Copy link
Member

Choose a reason for hiding this comment

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

Q: Do we pass null anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

not sure… I suspect somewhere it was called with null and just added it to match the usage

but it looks to be not used anymore - removing the null

child: Object,
parent: Object,
): void {
export default function inherit(key: string, child: any, parent: any): void {
Copy link
Member

Choose a reason for hiding this comment

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

t.Node instead of any.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

child: {
value: string;
},
args: Array<any>,
Copy link
Member

Choose a reason for hiding this comment

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

t.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.

done

@@ -10,7 +9,8 @@ export default function isPlaceholderType(
): boolean {
if (placeholderType === targetType) return true;

const aliases: ?Array<string> = PLACEHOLDERS_ALIAS[placeholderType];
const aliases: Array<string> | undefined | null =
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 never null

type: T,
node: t.Node | null | undefined,
opts?: undefined,
): node is Extract<t.Node, { type: T }>;
Copy link
Member

Choose a reason for hiding this comment

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

Neat!

if (nodeType === targetType) return true;

// This is a fast-path. If the test above failed, but an alias key is found, then the
// targetType was a primary node type, so there's no need to check the aliases.
if (ALIAS_KEYS[targetType]) return false;

const aliases: ?Array<string> = FLIPPED_ALIAS_KEYS[targetType];
const aliases: Array<string> | undefined | null =
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 never null

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated
this btw, is coming from from converted flow type: ?Array<string> ==> null | undefined | Array<string>

const types = typeAnnotations.map(type => type.typeAnnotations);
typeAnnotations: Array<t.TSTypeAnnotation>,
): t.TSType {
const types = typeAnnotations.map(type => type.typeAnnotation);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

looking at usage from babel-traverse apparently this was a typo and instead it should be . typeAnnotation instead

@nicolo-ribaudo
Copy link
Member

Here you can see the published package - the published types look good.

types-7.12.10.tgz

build-typescript-typings:
echo "TODO: maybe add types for typescript 3.7"
build-typescript-3.7-typings:
$(NODE) packages/babel-types/scripts/generators/typescript-3.7.js > packages/babel-types/lib/index-ts3.7.d.ts
Copy link
Contributor Author

@zxbodya zxbodya Dec 9, 2020

Choose a reason for hiding this comment

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

would like to not have duplicated script for typescript type generation… - going to try https://github.com/sandersn/downlevel-dts (but this can also be after merging this pr)

Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer to keep this solution that has been working well for a while. We'll remove it in Babel 8 anyway.

@nicolo-ribaudo nicolo-ribaudo force-pushed the ts-migration/babel-types branch 2 times, most recently from a6a268a to 1669fe6 Compare December 10, 2020 14:45
@nicolo-ribaudo
Copy link
Member

I'm rebasing

zxbodya and others added 2 commits December 10, 2020 18:54
* babel-types flowts convert
* babel-types code generation updates
* babel-types add more generated types to src (copy logic from scripts/typescript)
* babel-types update generateBuilders to specify exact argument types
* babel-types fix deprecated types generation
* babel-types fixes
* babel-types fix potential bug
* babel-types update generateBuilders, to include proper return types
* babel-types backport types from generated index.d.ts to be included in sources
* babel-types fixes
* babel-types avoid export of builders starting with upper case in ts sources
* babel-types
* babel-types todo updates, small fixes
* babel-types generate helpers
* babel-types remove typescript definitions generation
* babel-types generate files
* babel-types copy d.ts file for deprecated ast node creators into lib folder
* babel-types use t as alias for ast node types namespace
* babel-types update generateBuilders script to use "t" as alias instead of "types"
* babel-types update deprecated builders generation to have more compact file
* babel-types publish the .d.ts file for `@babel/types`
* Publish the .d.ts file for `@babel/types`
* Fix flowcheck
* Prettier
* No need to lint generated .d.ts files (now they are generated by tsc)
* Run prepublish-prepare-dts from prepublish-build
* Fix Babel 8 tests
* Update codecov config
* babel-types more improvements
  - better type for gatherSequenceExpressions
  - mark createUnionTypeAnnotation as deprecated
  - fix createTSUnionType
  - babel-types better type for builder
  - other improvements from PR comments
* babel-types lint fix
* babel-types fix createTSUnionType
* Don't commit .d.ts file in lib, and rename "deprecated"->"uppercase" for builders
* Add back types for TS 3.7
* Update packages/babel-types/package.json
* Cleanup converters types

Co-Authored-By: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>
@@ -102,6 +113,8 @@ function generateTypeHelpers(helperKind) {
})
)
.pipe(gulp.dest(dest));

return finish(stream);
Copy link
Member

Choose a reason for hiding this comment

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

@JLHwung This is a bug introduced by the recent gulpfile changes: we were returning a stream instead of a promise, and we were passing those streams to Promise.all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah good catch!

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 is awesome!

@nicolo-ribaudo
Copy link
Member

:shipit:

@nicolo-ribaudo nicolo-ribaudo merged commit 1290d21 into babel:main Dec 10, 2020
@nicolo-ribaudo nicolo-ribaudo deleted the ts-migration/babel-types branch December 10, 2020 20:22
@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 Mar 12, 2021
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Flow -> TS Tracking repository migration from Flow to TS outdated A closed issue/PR that is archived due to age. Recommended to make a new issue pkg: types
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants