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

Initial support for TypeScript in Recast via Babylon 7 parser #461

Merged
merged 73 commits into from
Feb 19, 2018

Conversation

Phoenixmatrix
Copy link
Contributor

@Phoenixmatrix Phoenixmatrix commented Feb 5, 2018

@benjamn

Would resolve #424

I think it's time I get an eye on this.

It also includes #458 and #460, but those are their own PRs and you won't hurt my feelings we have to remove them from this one :)

As discussed in the typescript issue, this uses the babylon parser and uses an initial (not complete by any mean) ast to provide basic TypeScript printing. For the scope of this PR I focused on supporting as many of the common ast nodes as I could and printing in the common/simple case. I don't yet take into consideration options other than tabWidth, and some more complex elements (like module bodies) are probably not complete, but this is enough to work with common Recast uses with TypeScript.

I still need to do a bit of cleanup, but would like your input on the general approach. Thanks! (Opening the ast-types PR shortly)

(build failing is just because ast-types isn't published)

@benjamn benjamn self-assigned this Feb 5, 2018
@benjamn benjamn self-requested a review February 5, 2018 21:40
@benjamn benjamn removed their assignment Feb 5, 2018
@benjamn
Copy link
Owner

benjamn commented Feb 11, 2018

For comparison, it might be useful to refer to Prettier's printer implementation, since Prettier can print TypeScript types, and Prettier was originally forked from the Recast pretty-printer, so their implementation should be more or less applicable here. They've changed some things, of course—e.g. they don't use Lines objects—but I think the basic case structure is similar.

Copy link
Owner

@benjamn benjamn 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 looking really great. You’ve picked up the quirks and conventions of this codebase, and reused existing code as well as I could have hoped. I’m sure there will be some additional cases to implement after benjamn/ast-types#249 is merged, but the general approach here is 👌. Thanks for jumping into this!

lib/printer.js Outdated
" = ",
path.call(print, "initializer")
);
parts.push();
Copy link
Owner

Choose a reason for hiding this comment

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

What’s this empty .push() doing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Absolutely nothing. Copy paste mistake :) Ill fix it as soon as I have some free time to get back into this later this week.

@benjamn
Copy link
Owner

benjamn commented Feb 11, 2018

Oh also, when you get a chance, could you rebase against master?

@blaster151
Copy link

Naive question (and probably not in the right place to ask). I'm a little new to the GitHub/node ecosystem. If I wanted to test out the PRs for recast and ast-types from jscodeshift, is that possible using this assumed approach?

  • npm init
  • npm install jscodeshift
  • update package.json with newer dependency versions to match the PRs for recast/ast-types (like babylon 7)
  • npm install
  • download recast source, paste over node_modules/recast
  • download ast-types source, paste over node_modules/ast-types
  • run jscodeshift as normal with --parser babylon

@Phoenixmatrix
Copy link
Contributor Author

You can take a look at this comment on the JSCodeshift issue as someone has done some work on getting it working (I think there's a few tweaks on the jscodeshift side needed?

The main thing is: you don't copy paste the code over. Use npm link for that to link local version of packages. You also need to manually configure the parser on the jscodeshift side because the built in ones won't have the right plugins.

@benjamn
Copy link
Owner

benjamn commented Feb 13, 2018

@blaster151 Instead of downloading the source and copying it into node_modules, you can actually use a GitHub tarball URL as the version for ast-types or recast in your package.json file:

"dependencies": {
  "recast": "https://github.com/Phoenixmatrix/recast/tarball/typescript-initial",
  "ast-types": "https://github.com/Phoenixmatrix/ast-types/tarball/typescript-initial"
}

Alternatively, you can install these versions using npm:

cd path/to/your/project
npm install https://github.com/Phoenixmatrix/ast-types/tarball/typescript-initial
npm install https://github.com/Phoenixmatrix/recast/tarball/typescript-initial

However, since this version of ast-types probably won't satisfy recast's version constraint, you may need to delete the nested copy node_modules/recast/node_modules/ast-types, to force node_modules/recast to use node_modules/ast-types.

@benjamn
Copy link
Owner

benjamn commented Feb 13, 2018

As @Phoenixmatrix suggested, npm link is another good option if you're planning to modify ast-types or recast, and you want those changes to be reflected in your node_modules/{recast,ast-types} package directories. If you're not planning to modify the packages, the GitHub tarball URL strategy may be a little easier.

@blaster151
Copy link

Wow, my mind is blown - I didn't even know npm could work with GitHub tarball URLs. Thanks so much!

@blaster151
Copy link

Thank you again, @benjamn and @Phoenixmatrix, for the direction. I got jscodeshift working (similarly to that poster on the thread you linked to - I hadn't seen that issue on jscodeshift's GitHub page and I'm assuming because it's currently closed). Giddy!

@blaster151
Copy link

Although I'm wondering if I should just be using recast straight-up. Is jscodeshift basically the most popular command-line "runner" for recast?

@benjamn
Copy link
Owner

benjamn commented Feb 15, 2018

Recast itself has a very simple (and old) require("recast").run API. You can see an example of that here. The example/add-braces module can be run as a script on the command-line, taking the name of a JS file to transform. The implementation of run is so trivial that I just inlined it into main.js. By comparison, jscodeshift is a much more sophisticated and fully-featured runner, but you're welcome to try a more manual approach. My guess is that the experience would leave you with a deeper appreciation for the benefits that jscodeshift provides!

@benjamn benjamn changed the title [WIP] Initial support for TypeScript in recast via Babylon parser Initial support for TypeScript in Recast via Babylon 7 parser Feb 19, 2018
@benjamn benjamn merged commit c311159 into benjamn:master Feb 19, 2018
@benjamn
Copy link
Owner

benjamn commented Feb 20, 2018

Ok! I'm confident we've covered all the different kinds of TypeScript syntax that can be parsed by the Babylon 7 parser, and that we've implemented pretty-printer cases for all those new AST types. This means it's now possible to use Recast to transform TypeScript code, which is something that's never been possible before!

Two areas for future improvement:

  • Using Recast to process TypeScript is still somewhat tricky because you have to override the default parser (Esprima) and configure Babylon appropriately. I think this could be easier, though I'm not ready to enable TypeScript parsing by default. Perhaps Recast could provide some pre-configured parsers, like Reify does here?
  • Recast can now reprint TypeScript syntax well enough that the generated code re-parses to an equivalent AST; however, I'm sure there are lots of opportunities to improve the pretty-printing of TypeScript syntax, so that it looks more like the code someone would write by hand. For example, long lines could be broken more intelligently. Please keep an eye out for any awkward printing as you experiment with this new functionality.

@Phoenixmatrix
Copy link
Contributor Author

We'll be using this in the real world soon enough, so I'll keep an eye for opportunities to open PRs when we hit cases that could be pretty-printed better :)

Thank you so much for helping with this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support TypeScript
3 participants