Convert `@babel/template` from Flow to TS #12317

nicolo-ribaudo · 2020-11-06T02:27:38Z

Q	A
Tests Added + Pass?	Yes
Any Dependency Changes?	Yes, devDeps
License	MIT

This PR is extracted from #11578. Since #11578 is so big that it's almost impossible to review, I'm attempting to enable a gradual migration (package by package) from Flow to TS.

Both flow and TS check their files, until we finish migrating
make tscheck only works after building the monorepo. We can avoid this by migrating packages in topological order if possible.
tsconfig.json files are gitignored: it's easy to compute them in make tscheck, so I think we can avoid committing one new "noise" file per folder.
we need to keep flow definitions of the converted files. This is not necessarily bad, we may even publish them when we have finished.

Btw, while working on this PR I already noticed that TS in VSCode is definitely more helpful than Flow 😁

cc @zxbodya

nicolo-ribaudo · 2020-11-06T02:30:00Z

scripts/generators/tsconfig.js

+          // Until we have converted every package, we cannot store
+          // .d.ts files inside lib/ because it causes conflicts
+          // with Babel-related type definitions in node_modules/@types
+          outDir: "./dts",


This is because, until all of our packages have .ts files, we:

Rely on some packages that are still .js filed and have the corresponding node_moduels/@types/ definitions

Those packages rely on our already converted packages

They load the .d.ts files (only if we store them in lib)

TS refuses to update the .d.ts files because they are used as inputs

This is a tricky one, the issue is happening only when having kinda circular dependency - in other words, generated d.ts files in a package are somehow imported by itself while compiling. And this is something that needs to be fixed, while migrating.

I would suggest, @babel/template is just not the right starting point - dependencies of it can be migrated first, and that would solve the issue.

nicolo-ribaudo · 2020-11-06T02:30:37Z

packages/babel-template/src/parse.ts

      state.isLegacyRef.value = false;
    }
  } else if (state.isLegacyRef.value === false || state.syntacticPlaceholders) {
    return;
  } else if (t.isIdentifier(node) || t.isJSXIdentifier(node)) {
-    name = ((node: any): BabelNodeIdentifier).name;
+    name = node.name;


Here TS is better than flow at refining node

it can be even better - if to first convert @babel/types and @babel/traverse packages (there are many types missing or to be improved comparing to definitely typed)

babel-bot · 2020-11-06T02:31:05Z

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

nicolo-ribaudo · 2020-11-06T02:31:19Z

packages/babel-parser/typings/babel-parser.d.ts

-export function parse(input: string, options?: ParserOptions): import('@babel/types').File;
+export function parse(
+  input: string,
+  options?: ParserOptions
+): import("@babel/types").File;


This file is just reformatted, since I enabled Prettier on TS files.

codesandbox-ci · 2020-11-06T02:32:25Z

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 4c8ea55:

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

zxbodya · 2020-11-06T10:42:27Z

packages/babel-template/src/index.js

@@ -1,31 +0,0 @@
-// @flow


to avoid loosing file edits history, I would suggest having two separate commits (one to rename the files, and other to convert the content)

zxbodya · 2020-11-06T11:21:07Z

I agree that #11578, if considering entire thing at once - it is too big to be properly reviewed.

But a very big part of the PR is automatically converted from the flow sources, and during that it is already verified to remain exactly the same after types are removed - and so, it does not actually needs to be reviewed, it is safe to merge as is.

Except that automatic step - there are just a few bigger things that would take some time to review, but most of the commits there it a fairly trivial(that is on purpose - I avoided much refactor, trying to have only minimal type fixes, specifically to allow migration to be fast)

And so, I am not sure it worth to go really gradually keeping both flow and typescript type coverage at the same time - I would suggest that would add too much overhead.

And instead, I would suggest following bigger steps:

add support for having typescript sources (linter and build configration)
convert everything to typescript at once, but - do not yet run typecheck in CI, and do not publish generated type definitions
do remaining manual fixes, by the way adding finished packages to by typechecked in CI
align types in babel to where possible be the same as in definitely typed, and start publishing them(that is not yet in Migrate Babel from Flow to TypeScript (except Babel parser) #11578 but I already did some progress there - it is not too big)

The biggest drawback would be - for a brief time there would be no type check in the repository, but given flow coverage is not good anyway - so, I do not really worry about that.

nicolo-ribaudo · 2020-11-06T23:39:37Z

(#12317 (comment)) This is a tricky one, the issue is happening only when having kinda circular dependency - in other words, generated d.ts files in a package are somehow imported by itself while compiling. And this is something that needs to be fixed, while migrating.

I noticed that the @RyanCavanaugh (TS team) was looking into circular project references (microsoft/TypeScript#33685), and the issue is marked as Awaiting More Feedback. Maybe our repository can provide an example of use case 😄

(#12317 (comment)) I would suggest, @babel/template is just not the right starting point - dependencies of it can be migrated first, and that would solve the issue.

Yeah, probably the best starting point would have been something like @babel/helper-validator-identifier which doesn't depend on other @babel/* packages. However, even if we start from something else (@babel/template in this case) is that the types of the package it imports is any until we migrate its dependencies.
However, the only untyped dependency on @babel/template is @babel/code-frame and it's used in a place where any doesn't propagate, so it isn't a real problem.

(#12317 (comment)) And so, I am not sure it worth to go really gradually keeping both flow and typescript type coverage at the same time - I would suggest that would add too much overhead.

I don't think that running both Flow and TS at the same time adds too much overhead. We run type checking as part of CI, and it's definitely not one of the slowest jobs.
During development, TypeScript is run by the editor in the background so it doesn't slow us down.

In the end, both strategies (by "conceptual" chunks as you are proposing, or by groups of packages as I'm proposing) will give the same result, so the decision should just be about what's easier for us to manage during the migration.

I prefer the approach I'm proposing because, even if the automated transform works from a theoretical point of view, there are different things that we might still want to change.
Some examples of things that can only be caught by carefully reviewing the diff (even if it's automated) that I changed in this PR are:

I manually added names to inline function types (https://github.com/babel/babel/pull/12317/files#diff-5885c0e14e03fdb38ad7f559ca7e775ab7898c828d1843d4414880c239f3cb62R13). I noticed that your tool set them to a and that works, but I think we can improve the code readability by properly setting them.
I removed some explicit type arguments (https://github.com/babel/babel/pull/12317/files#diff-38f9768c5065caa9d6c6cf1636e0764270e9005cb6ce07e625b6a7c6c6153fa0R4) that were not needed and they were just hurting readability. In flow we used <*>, in TS we can just omit them.
I noticed we were explicitly defining an ast property in DefaultTemplateBuilder (https://github.com/babel/babel/pull/12317/files#diff-38f9768c5065caa9d6c6cf1636e0764270e9005cb6ce07e625b6a7c6c6153fa0R10), which isn't needed because it's already in typeof smart.

All those things are really nitpicks, but they improve the code quality and are harder to notice in a big automated PR.

@JLHwung / @existentialism / @hzoo / @kaicataldo Do you have any opinion on this?

hzoo · 2020-11-06T23:56:20Z

automated transform works from a theoretical point of view

If we aren't comfortable with automated approach (I think if we were we would of already done it, and we'd have to review it either way), then just doing it slowly manually seems fine, esp if given someone is looking at it you can catch things along the way. If we start this, maybe just need to be disciplined in actually reviewing/merging and having some plan to move things over.

nicolo-ribaudo · 2020-11-07T00:04:11Z

If we aren't comfortable with automated approach

Actually, without @zxbodya's tool migrating would be order of magnitudes harder. The automated tool gives us a very good basis, but the output still needs to be tweaked (and thus, reviewed).

What I did in this PR was cherry-picking files from the other PR, and then reviewing and slightly adjusting them.

JLHwung · 2020-11-09T14:19:38Z

The automated tool gives us a very good basis, but the output still needs to be tweaked (and thus, reviewed).

💯 for the current approach. LGTM as well as type check is good. We can always iterate later.

(I have updated the colour of "Flow -> TS" label to match the theme colour of typescriptlang.org)

nicolo-ribaudo · 2020-11-09T14:22:43Z

(I have updated the colour of "Flow -> TS" label to match the theme colour of typescriptlang.org)

Thanks, I had that intention but just guessed since I was too lazy to check what it should have been 😂

zxbodya · 2020-11-09T15:08:30Z

I don't think that running both Flow and TS at the same time adds too much overhead. We run type checking as part of CI, and it's definitely not one of the slowest jobs.

I mean extra work needed to keep flow working for remaining packages - like adding lib/babel-packages.js.flow in this PR or to have build/bundling config for both type systems. Not sure how much work it is, but still given that would be useful only for a brief time - might be nice to avoid.

I prefer the approach I'm proposing because, even if the automated transform works from a theoretical point of view, there are different things that we might still want to change.

When converting, the tool literally checks that output when using flow preset and output using typescript preset on converted code are the same - so it should be pretty safe from perspective of not breaking anything except the type checks.

Except automated part - there are some manual changes required to fix the differences between type systems, to make type check passing - that indeed needs a review, but can be done separately (if we are ok, to have some broken types for migration time)

The automated tool gives us a very good basis, but the output still needs to be tweaked (and thus, reviewed).

I agree there are many things that would be nice to be tweaked, but I am not sure how much of it should be done during initial migration - my feeling is, better to have as much of it as possible separately from the initial step:

this way migration can be faster, because less changes are to be made and so reviewed
might be bigger benefit - when doing additional improvements separately, it would be possible to check them against usages in the rest of codebase, which might be helpful to catch errors early

In #11578, I tired to avoid changes that are not really necessary and instead adding todo(flow->ts) where something needs probably non trivial improvement.

Also on type improvements, I would suggest after initial migration it would be nice to consider adding strict type checking in tsconfig, maybe that can be combined with other type improvements…

About migration approach - I do not really have a strong preference.

Both look fine for me, the biggest difference I see - doing each package separately - would be a bit more work, but would also result in more detailed review.

I see two main options:

faster/easier (kinda done in Migrate Babel from Flow to TypeScript (except Babel parser) #11578 - only to split to separate PRs)
- migrate everything at once (automated part)
- update build/lint config to support only TS
- separate PRs with type fixes/improvements
a bit slower, but with more detailed review and maybe a bit safer
- update build/lint config to support both flow an typescript (I would suggest this would be more complicated)
- create separate PR to convert each package (automated part + manual fixes)

Lets decide on one - and then, I can split #11578 into multiple PRs(thinking to make a script for that),

if option 2 - one PR per package
if option 1 - maybe to group some packages to go together (when changes are very minimal)

Anther thing to consider, is how to deal with existing types in definitely typed, like here:
https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/babel__template

I would suggest - postpone this, and and do not publish generated types, while not everything is migrated

Once everything is in typescript - where possible update sources to match types from DefinitelyTyped(often types there are more detailed, and more importantly already used by external projects (like jest). And only then start publishing the types, and to deprecate DefinitelyTyped for newer versions.

zxbodya · 2020-11-09T15:12:50Z

scripts/generators/tsconfig.js

+    {
+      files: [],
+      references: tsPkgs.map(({ dir }) => ({
+        path: path.relative(root, dir),


to keep in mind - this is not always correct: some dev packages may need to not be included(because will cause cyclical dependency)

also in many cases - @babel/types, @babel/traverse would need to be added

so, I would say - this should be used only once to generate initial configs which then to be committed

I'm not tracking devDependencies here, because they are not used in src folders (this also avoids cycles, at least for the non-plugin packages).

@babel/types has its own type building logic (it already has its published .d.ts file) and it isn't build using tsc, so tsc already picks up the type definitions.

nicolo-ribaudo · 2020-11-09T18:39:42Z

I mean extra work needed to keep flow working for remaining packages - like adding lib/babel-packages.js.flow in this PR or to have build/bundling config for both type systems. Not sure how much work it is, but still given that would be useful only for a brief time - might be nice to avoid.

I'd be glad to help with the Flow definitions! It shouldn't be too much work, and it will give me a clear idea of our API surface which is good.

Also on type improvements, I would suggest after initial migration it would be nice to consider adding strict type checking in tsconfig, maybe that can be combined with other type improvements…

100% agree 😄

Anther thing to consider, is how to deal with existing types in definitely typed, like here:
https://github.com/DefinitelyTyped/DefinitelyTyped/tree/master/types/babel__template

I would suggest - postpone this, and and do not publish generated types, while not everything is migrated

Yeah I agree, we shouldn't publish types yet. I think we can keep checking DefinitelyTyped (and maybe testing our types on the Jest source) as the last step.

Lets decide on one - and then, I can split #11578 into multiple PRs

Since some packages are very small, we'll go with a "mixed" approach. I have prepared a list of "batches" (they are still a small number of packages, but it's a good starting point): they don't have cycles, and they have the correct size to be properly reviewed.
Every PR should contain a commit to rename the files .js -> .ts and then the commits with the actual migration. When squashing/merging I'll make sure to keep two separate commits.

I'll cross-post this list (if it looks good to you) in your draft PR, and I'll start preparing Flow definitions for the first batch.

Batch 1:

@babel/helper-validator-identifier
@babel/highlight
@babel/core-frame

Batch 2:

@babel/generator

Batch 3:

@babel/helper-get-function-arity
@babel/helper-function-name
@babel/helper-split-export-declaration

Batch 4: (depends on 2 and 3)

@babel/traverse

Batch 5:

@babel/helper-module-imports

Batch 6: (depends on 3 and 4)

@babel/helper-annotate-as-pure
@babel/helper-wrap-function
@babel/helper-remap-async-to-generator

Co-authored-by: Bogdan Savluk <savluk.bogdan@gmail.com>

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

zxbodya · 2020-11-09T19:48:33Z

I'd be glad to help with the Flow definitions! It shouldn't be too much work, and it will give me a clear idea of our API surface which is good.

As an option to help with flow type definitions - it should be possible to use compilation result after converting to TypeScript.

It is possible to bundle d.ts files(using some of tools mentioned here: timocov/dts-bundle-generator#68, api-extractor btw looks to be most advanced, and probably can be used in future to generate api documentation for babel - but it has some limitations). When d.ts bundle is ready - flowgen can be used to convert it to flow, effectively getting almost complete flow type definition for the package.

btw, I rebased #11578 so it can be used for latest code version.

I think we can keep checking DefinitelyTyped

I would say - we should not use it for now, and only later to use type tests from it to align type definitions later… (if to use it - there would be weird issues because, like - inability to generate files, because they are used as input 😆, or need to rename things(which later, with typescript support can be much easier leveraging IDE features))

about a ways to exclude @types/* - I did a following hack, in top level package.json resolutions:

    "@types/babel__core": "link:./nope",
    "@types/babel__traverse": "link:./nope"

(basically resolving specific type packages to something non-existing…)

Also, as a first steps -- I would say, we should include @babel/types right after Batch1 - there would be are some additional types to be used in next packages, and reducing need in manual fixes.

Co-authored-by: Bogdan Savluk <savluk.bogdan@gmail.com>

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

nicolo-ribaudo · 2020-11-09T20:35:35Z

I got some type errors while rebasing, so I added f0a2a6d (#12317): we already publish @babel/types dts, but we didn't add type for assert* methods (because of backward compat with older TS versions). I discovered the typesVersions, so I used it and it seems to work.

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

nicolo-ribaudo · 2020-11-09T20:39:05Z

Also, as a first steps -- I would say, we should include @babel/types right after Batch1 - there would be are some additional types to be used in next packages, and reducing need in manual fixes.

If you think that something is needed in a different order than what I proposed feel free to do it. However, we already generate types for @babel/types so it might have a lower priority.

Btw, since this is a lot of work feel free to ask me to continue when you don't want to.

Co-authored-by: Bogdan Savluk <savluk.bogdan@gmail.com>

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

nicolo-ribaudo · 2020-11-09T22:18:52Z

a9bc9be, f80478c and 089c200.

I rebased instead of squashing because the commits are conceptually separated and have different authors.

nicolo-ribaudo commented Nov 6, 2020

View reviewed changes

nicolo-ribaudo added the Flow -> TS Tracking repository migration from Flow to TS label Nov 6, 2020

zxbodya reviewed Nov 6, 2020

View reviewed changes

nicolo-ribaudo force-pushed the typescript-babel-template branch from c90459c to dfe15bd Compare November 6, 2020 23:14

JLHwung approved these changes Nov 9, 2020

View reviewed changes

nicolo-ribaudo force-pushed the typescript-babel-template branch from dfe15bd to 1c1e3be Compare November 9, 2020 14:41

zxbodya reviewed Nov 9, 2020

View reviewed changes

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Nov 9, 2020

Prepare repository for gradual flow->ts migration (babel#12317)

fbd3b0e

Co-authored-by: Bogdan Savluk <savluk.bogdan@gmail.com>

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Nov 9, 2020

Convert @babel/template from Flow to TS (babel#12317)

d6d9063

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

nicolo-ribaudo force-pushed the typescript-babel-template branch from 1c1e3be to d6d9063 Compare November 9, 2020 19:03

nicolo-ribaudo and others added 2 commits November 9, 2020 21:33

Use TS3.7 when possible for @babel/type dts (babel#12317)

f0a2a6d

Prepare repository for gradual flow->ts migration (babel#12317)

70ee035

Co-authored-by: Bogdan Savluk <savluk.bogdan@gmail.com>

nicolo-ribaudo added a commit to nicolo-ribaudo/babel that referenced this pull request Nov 9, 2020

Convert @babel/template from Flow to TS (babel#12317)

c2d1852

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

nicolo-ribaudo force-pushed the typescript-babel-template branch from d6d9063 to c2d1852 Compare November 9, 2020 20:34

Convert @babel/template from Flow to TS (babel#12317)

4c8ea55

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

nicolo-ribaudo force-pushed the typescript-babel-template branch from c2d1852 to 4c8ea55 Compare November 9, 2020 20:36

JLHwung approved these changes Nov 9, 2020

View reviewed changes

nicolo-ribaudo merged commit 6654c04 into babel:main Nov 9, 2020

nicolo-ribaudo deleted the typescript-babel-template branch November 9, 2020 22:15

nicolo-ribaudo added a commit that referenced this pull request Nov 9, 2020

Use TS3.7 when possible for @babel/type dts (#12317)

a9bc9be

nicolo-ribaudo added a commit that referenced this pull request Nov 9, 2020

Prepare repository for gradual flow->ts migration (#12317)

f80478c

Co-authored-by: Bogdan Savluk <savluk.bogdan@gmail.com>

nicolo-ribaudo added a commit that referenced this pull request Nov 9, 2020

Convert @babel/template from Flow to TS (#12317)

089c200

Co-authored-by: Nicolò Ribaudo <nicolo.ribaudo@gmail.com>

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label Feb 9, 2021

github-actions bot locked as resolved and limited conversation to collaborators Feb 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Convert `@babel/template` from Flow to TS #12317

Convert `@babel/template` from Flow to TS #12317

nicolo-ribaudo commented Nov 6, 2020 •

edited by gitpod-io bot

nicolo-ribaudo Nov 6, 2020

zxbodya Nov 6, 2020

nicolo-ribaudo Nov 6, 2020

zxbodya Nov 6, 2020

babel-bot commented Nov 6, 2020 •

edited

nicolo-ribaudo Nov 6, 2020

codesandbox-ci bot commented Nov 6, 2020 •

edited

zxbodya Nov 6, 2020

zxbodya commented Nov 6, 2020

nicolo-ribaudo commented Nov 6, 2020

hzoo commented Nov 6, 2020

nicolo-ribaudo commented Nov 7, 2020 •

edited

JLHwung commented Nov 9, 2020

nicolo-ribaudo commented Nov 9, 2020

zxbodya commented Nov 9, 2020 •

edited

zxbodya Nov 9, 2020 •

edited

nicolo-ribaudo Nov 9, 2020

nicolo-ribaudo commented Nov 9, 2020

zxbodya commented Nov 9, 2020

nicolo-ribaudo commented Nov 9, 2020

nicolo-ribaudo commented Nov 9, 2020

nicolo-ribaudo commented Nov 9, 2020

Convert @babel/template from Flow to TS #12317

Convert @babel/template from Flow to TS #12317

Conversation

nicolo-ribaudo commented Nov 6, 2020 • edited by gitpod-io bot

nicolo-ribaudo Nov 6, 2020

Choose a reason for hiding this comment

zxbodya Nov 6, 2020

Choose a reason for hiding this comment

nicolo-ribaudo Nov 6, 2020

Choose a reason for hiding this comment

zxbodya Nov 6, 2020

Choose a reason for hiding this comment

babel-bot commented Nov 6, 2020 • edited

nicolo-ribaudo Nov 6, 2020

Choose a reason for hiding this comment

codesandbox-ci bot commented Nov 6, 2020 • edited

zxbodya Nov 6, 2020

Choose a reason for hiding this comment

zxbodya commented Nov 6, 2020

nicolo-ribaudo commented Nov 6, 2020

hzoo commented Nov 6, 2020

nicolo-ribaudo commented Nov 7, 2020 • edited

JLHwung commented Nov 9, 2020

nicolo-ribaudo commented Nov 9, 2020

zxbodya commented Nov 9, 2020 • edited

zxbodya Nov 9, 2020 • edited

Choose a reason for hiding this comment

nicolo-ribaudo Nov 9, 2020

Choose a reason for hiding this comment

nicolo-ribaudo commented Nov 9, 2020

zxbodya commented Nov 9, 2020

nicolo-ribaudo commented Nov 9, 2020

nicolo-ribaudo commented Nov 9, 2020

nicolo-ribaudo commented Nov 9, 2020

Convert `@babel/template` from Flow to TS #12317

Convert `@babel/template` from Flow to TS #12317

nicolo-ribaudo commented Nov 6, 2020 •

edited by gitpod-io bot

babel-bot commented Nov 6, 2020 •

edited

codesandbox-ci bot commented Nov 6, 2020 •

edited

nicolo-ribaudo commented Nov 7, 2020 •

edited

zxbodya commented Nov 9, 2020 •

edited

zxbodya Nov 9, 2020 •

edited