Preserve blank lines in plugins using TypeScript transforms

[edsrzf]

The aim of this change is to fix #97. It makes three changes:

• Introducing an UpdateTracker class. It keeps a list of SourceTextUpdate changes to a source file and allows reusing a printer.
• Modify the add-conversions plugin to use UpdateTracker
• Modify the jsdoc plugin to use UpdateTracker

All in all I'm not thrilled with what this change does to the code, but it does seem to work pretty well and I have explored the other possibilities quite a bit. I view #97 as an issue that is important to fix, and think it's worth making some sacrifices to do so. That said, I'm very open to suggestions and feedback to make this better.

Details

Instead of printing the whole source file AST, we keep track of updates and make updates to the text. The updates are generally created by printing only select nodes. There is some duplication with the existing getTextPreservingWhitespace that it would be nice to eliminate in the future, but for now I wanted to be able to reuse the printer.

The ts.transform function is still used so that we have a ts.TransformationContext available. This is necessary if we want to call ts.visitEachChild, and it's hard/impossible to write these transforms without using that function. I'm not very happy about this aspect of the plugins now, since we're now performing side effects in the transform rather than using the AST that comes out, but I can't think of a better way to structure it.

There were some special considerations required for certain cases in each of the plugins:

• When printing an AsExpression, the printer doesn't know whether or not to add parentheses. To know that, we have to print a higher-level node such as the next parent statement. There is now a fair bit of logic in add-conversions to figure out the right node to replace.
• There was some special-casing necessary to handle parentheses around arrow functions in jsdoc.

Alternatives Attempted/Considered

I have thought about and tinkered with this...a lot, and tried several different approaches.

Patch Tools

I have hacked around this issue in some of my migrations by using patch tools. The results have been okay, but not great.

I played around with using something like jsdiff and similar libraries to generate a diff and do some post-processing, but it was hard to get it working well just felt very hacky in general.

Transformer Middleware

I tried writing TypeScript transformer middleware that would track the changed AST nodes. This proved more difficult than I initially thought, since AST nodes are immutable, so it's hard to distinguish between "this node changed and we need to reprint it" vs "this node changed only because one of its children changed, and we don't need to reprint it".

AST Diffing

I tried to keep both the old AST and the new AST, so I could visit all the children and re-print only the ones that had changed during the transformation. This seems like it could be a pretty good solution, except for the fact that I can't find a way to reuse any of the visitor functions from the TypeScript API.

I'd have to basically duplicate the logic from TypeScript's visitEachChild, but for two nodes at once. This is a considerable amount of code to duplicate and maintain. (Potentially I could leave off certain node types.)

Use jscodeshift

I haven't enjoyed working with jscodeshift that much in the explicit-any plugin. It also has issues to work around (#114 for example), the TypeScript types are not very good, and it generally doesn't feel well-maintained. It does not support JSDoc nodes needed for the jsdoc plugin. Finally, we have to work with the TypeScript API no matter what anyway, for diagnostics and other things.

In the future, assuming we go ahead with this PR, I'd like to explore re-working the plugins using jscodeshift to use this approach. Up to now, the only downside of using the TypeScript API was this blank line issue.

[Rudeg]

jsdoc plugin looks much cleaner now!

[Rudeg]

Sorry for the huge delay with review :(

• UpdateTracker class lgtm, seems like a good way to separate update logic from the plugins logic
• plugins changes look good as well
  In terms of reliability and future usage of this approach, it seems like a right way to proceed forward.

@edsrzf thank you for working on this!

[MartinCura]

Would this preserve all blank lines when using ts-migrate? In that case it would be wonderful if it was merged, @edsrzf !

I just did a whole migration but would much rather do it again than losing this much legibility.

[edsrzf]

@MartinCura Yes, it should preserve all blank lines when using ts-migrate. If you notice cases where it still doesn't, please file an issue.

Keep in mind that even though it's merged, it still needs to be released. I don't have access to do that myself. 😄

[MartinCura]

Thank you both!

I had some problems with the new version (a couple of JSX converted files became kind of broken), though i'm not completely sure if it wasn't a change in the codebase between my two attempts at migration. I'm just gonna use the tool without the plugins, so useful anyway! Thanks again!

[edsrzf]

@MartinCura I'm glad you found it useful! Could you please file an issue with more detail about the broken JSX files? The only existing one that I'm aware of is the one mentioned in #121 where ts-migrate's heuristic for detecting JSX is not quite good enough.

[MartinCura]

I'm sorry, i decided to just use the rename and reignore parts on this project, to avoid the hassle. The problems i encountered were a couple of files breaking (not sure why, to be honest, didn't invest time in figuring it out) and PropTypes->TS types losing a lot of specificity (which i preferred to rollback). As said, i'm not sure if the former was because of 0.1.20 particularly or a change in our codebase between attempts. In any case the tool was still useful, thanks and sorry !

[ajaska]

FWIW, we went with a fairly hacky approach that replaced all blank lines with a "magic" comment as the first plugin, and then replaced the "magic" comments with blank lines as the last plugin. Very cheap and easy to maintain, hasn't failed on ~1000 files so far :)