Significant enhancements

[benjie]

~~I'm very limited on time but I managed to make this basically work for the Graphile Engine codebase. No doubt there's going to be a bunch of cleanup, but I though I'd share my modifications in case they're helpful to others. Sorry about all the code formatting changes, this is not a genuine PR that I expect you to merge, just felt bad keeping my changes to myself. Feel free to close!~~

This PR includes a lot of improvements - more types are converted, existing supported types are converted more accurately leading to better formatting of outputted code, TypeScript definitions are more thorough.

[bcherny]

Hey @benjie! Thanks for the contribution. I have to say, without tests and with all the formatting changes (single quote -> double, semicolons), this PR is super hard to review.

Would it be too much to ask to revert your formatting changes, and add a few tests for your biggest changes?

[benjie]

If I have sufficient time left once I’ve completed the TypeScript conversion for PostGraphile I’ll certainly try.

Sorry I let my autoformatter go mad on the codebase, I wasn’t expecting to do more than a couple tweaks, ended up being much bigger changes.

More to come.

[benjie]

Loads of improvements today; have the old tests passing again with a changed architecture (the rules being independent approach doesn't work well because they're often inter-dependent) and the conversion for Graphile Engine shows no regressions but a load of improvements. Pretty pleased!

Take a look at the updated tests: the output.ts in many are now properly formatted. I've added explicit support for interfaces too.

Still more to come before it's mergeable.

[mxstbr]

Just ran this on a medium-sized codebase and it worked amazingly well, thanks so much @benjie! Then only thing that didn't work was converting import Thing, { type ThingType } and import { Thing, type ThingType }—other than that it worked beautifully :100: :100: :100:

[benjie]

Thanks @mxstbr

[SimenB]

This is super awesome, looking forward to run it on Jest's code base 🙂

Found an error testing this out

type MyType = {
  numbericThing: 42,
};

Error: Note type not understood: 'NumberLiteralTypeAnnotation'

(a rebase since your prettier PR was merged would be awesome, btw 😀)

[benjie]

I'd love to get back to working on this; if anyone's able to offer sponsorship that'd be very welcome.

[janicduplessis]

Tested this on some files in my codebase and it works great! I added support for import {A, type B}, using $FlowFixMe as a type annotation ((a: $FlowFixMe) => void) by replacing with any and nullish coalescing operator (??, maybe we can just use the babel transform for this?).

I'll probably add a few more things when I convert more files.

Not sure what the best way to collaborate on this but feed free to integrate changes I made on https://github.com/janicduplessis/flow-to-typescript/commit/f887f5c2e490e77c1eb39dc40c10bbee26b83e6f which is based on this PR.

[janicduplessis]

Got some more time to work on this. I switched the parser / printer to use recast (https://github.com/benjamn/recast) to preserve the source formatting and added support for optional chaining https://github.com/janicduplessis/flow-to-typescript/commit/735f88b44070f7f39563bf2cdb509f402a4b551c

The generated diffs are clean enough now that I feel confident applying this to a large codebase.

[benjie]

Awesome!!

[benjie]

Can you add a couple tests for it so I can pull it into this branch?

[benjie]

Small update: I've merged the latest master so the diff is easier to review.

[bcherny]

Closing + opening to trigger CircleCI.

[niieani]

Found some more unsupported syntax issues:

export type {Abc} from './abc'
// ==> converted to ==>
export {Abc}

Also, comments inside of types are removed :(

export type Reason = {
  // some comment
  thing: string;
};
==>
export type Reason = {
  thing: string;
};

I also tried to use @janicduplessis fork with recast, but then ALL the comments are gone (even non-typing ones).

[DylanVann]

I'm converting a large codebase to TypeScript and am attempting to use @janicduplessis fork.

I changed the tests around a bit since they weren't working when I checked out the code.

After the changes they use snapshots, so they're easier to update.

I think I've resolved the comment issue, but I'm not sure it covers all cases.

https://github.com/dylanvann/flow-to-typescript

Comment commit - https://github.com/DylanVann/flow-to-typescript/commit/ba7ddd58be5c2959ee361dac3874a827751e481d

You can see what it does with comments here:

• Block Comments
• Normal Comments

The format of the snapshots is that it prints the original code in the first snapshot, then the output in the second snapshot.

[janicduplessis]

@DylanVann Nice work! I haven't had the issues with comments that @niieani mentioned so I'm not sure what happened. I did noticed that the code that was supposed to strip the @flow comments did not work thought.

[benjie]

Upgraded to the latest versions of various dependencies so have managed to remove a bunch of @ts-ignore comments. Addressed a few more bits of feedback.

[benjie]

And another commit to parenthesise functions definitions in union types.

[DylanVann]

I believe it's specifically comments preceding/following nodes that are replaced by the converter.

The same problem can occur when writing codemods, as I'm experiencing right now trying to write a codemod to do some other transformations as part of this conversion.

@babel/preset-typescript doesn't like it when you do export { AType } because AType will be undefined, so I'm doing a pass of converting those to inline exports. If they're inline then the whole line goes away and Babel doesn't complain.

Might be something to include in this repo as well.

[benjie]

Added support for function rest parameters and use lazy heuristics to copy more comments across.

[benjie]

@bcherny This is actually a genuine PR now, and ready for review.

Here's the result of running it against the Graphile Engine codebase. I've not attempted to actually compile the generated TypeScript yet, but it looks broadly okay by eye.

https://github.com/graphile/graphile-engine/pull/370/files

[benjie]

Okay, now it's ready for review. I re-enabled noImplicitAny which means I've had to add a few explicit any. They're fine.

I've also pulled in more correct type files, so visitors are now type-checked too 🎉

[niieani]

Looks like there's an off-by-one error in how comments are handled in the example PR you mentioned: https://github.com/graphile/graphile-engine/pull/370/files.

What I mean is:

• the preceding comments are now inline: example
• the inline comments are now in the next line: example

[benjie]

@niieani Indeed; that's at least better than losing the comments though.

Here's the code that handles copying the comments across, if you can suggest improvements I'm all ears 👍

https://github.com/bcherny/flow-to-typescript/blob/04ccd25f583f94098b8428ca681aa310ebcdc4c2/src/convert.ts#L224-L234

[DylanVann]

I like that it's more generalized, I fixed that issue on my fork, but it only works one level deep, which isn't very good.

[DylanVann]

I came up with a less drastic way of improving the tests, so I can keep track of changes more easily.

This will allow you to use AVA's -u or --update-snapshots CLI arguments to update the output.txt files.

Feel free to add to this PR or repo if you think it helps:

Code (since I may force push on my fork later)

import test from 'ava'
import { sync } from 'glob'
import { readFile, writeFile } from 'mz/fs'
import { basename, resolve } from 'path'
import { compile } from '../src'

let paths = ['e2e', 'rules', 'unit']

// Kind of hacky thing to figure out if -u or --update-snapshots was passed to AVA.
// We manually write expected output in this test.
// It's more clear, and allows us to have input and output files for each test.
const argvString = process.env.npm_config_argv as string
const argv = JSON.parse(argvString || '')
const argvOriginal = argv.original || []
const update =
  argvOriginal.includes('-u') || argvOriginal.includes('--update-snapshots')

paths.forEach(path => {
  // TODO: Why does glob catch tslint.json even with the trailing slash?
  const folders = sync(resolve(__dirname, `../../test/${path}/*/`))
    .filter(_ => !_.endsWith('.json'))
    .filter(_ => !basename(_).startsWith('_'))

  const tests = folders.map(folder => ({ folder, name: basename(folder) }))

  tests.forEach(({ name, folder }) =>
    test(name, async t => {
      try {
        const inputPath = resolve(folder, 'input.txt')
        const outputPath = resolve(folder, 'output.txt')
        const input = await readFile(inputPath, 'utf-8')
        const output = await compile(input, inputPath)
        if (update) {
          await writeFile(outputPath, output)
          t.pass()
        } else {
          const expectedOutput = await readFile(outputPath, 'utf-8')
          t.is(output, expectedOutput)
        }
      } catch (e) {
        console.log('error', e)
      }
    })
  )
})

[benjie]

This PR has diverged too far to be a sensible PR any more; so I've forked the project to benjie/flow2typescript and released it to npm.

[benjie]

0.2.0 of flow2typescript is out including a large number more fixes; we've used it to help translate GraphiQL to TypeScript and I'm currently using it to convert Graphile Engine.

[niieani]

Thanks for the update @benjie. I wonder if you saw or @bcherny saw two other projects that aim to convert flow to typescript that I have found out about in the last months:

• https://github.com/grubersjoe/reflow
• https://github.com/zxbodya/flowts

I haven't had the chance to compare them with your update, but they both seem to have had quite a bit of progress.

[benjie]

No, I've not researched those. They're welcome to merge in parts of my code if they want; it's all MIT licensed. (I'm not planning to work on this any more, flow2typescript does everything I need it to do.)