Added TypeScript 3.9 support, switched implementation to ts-morph

[mathieumg]

Let me know what you think. Normally all 5 commits are cherry-pickable if there are things you don't want.

See microsoft/TypeScript#38548

[simonhaenisch]

Thanks I’ll take a look later, but I definitely don’t want the yarn lock file 😄

[simonhaenisch]

Thanks for the extra tests definitely (I was a bit lazy regarding those). I'm not really keen to introduce a dependency for this plugin though... can we figure out what ts-morph does differently and just do it the same way instead?

BTW just wondering, if this works with ts-morph, then maybe we're just missing something in the ServiceHost? I feel like there's a simpler fix... will investigate!

[mathieumg]

I'm not really keen to introduce a dependency for this plugin though...

I guess that's fair, but given this is local tooling I didn't think it would be a huge issue to add a long standing and well-maintained TS higher level API wrapper. I had already spent a lot of time tracking down whether the problem came from TS itself, Prettier, this plugin, or even elsewhere; I was looking for a way to fix it hehe. I haven't managed to reproduce the issue in the TS test suite either.

BTW just wondering, if this works with ts-morph, then maybe we're just missing something in the ServiceHost? I feel like there's a simpler fix... will investigate!

Possibly. It could be something in:

https://github.com/dsherret/ts-morph/blob/dcfbc37725885452b32309fa27c4c0b5bce4fbb1/packages/common/src/compiler/createHosts.ts#L33-L80

Thank you!

[simonhaenisch]

I've just figured out that it doesn't happen if there isn't a new line:

import { foo, bar, baz } from "foobar";

console.log({ foo, bar, baz });

This passes 🤷🏻‍♂️ I've tried adding this.getNewLine = () => '\n' into the compiler host but it must be something else... will check the ts-morph service host, thanks for the link!

[simonhaenisch]

Funny, these also pass:

import { foo,
	bar, baz } from "foobar";

import {
	foo,
	bar, baz } from "foobar";

import {
	foo,
	bar, baz
} from "foobar";

[simonhaenisch]

Ok I figured it out... if I pass { newLineCharacter: '\n' } as the preferences object (third param) into organizeImports, then it works. What I don't yet understand is why it doesn't pick up the getNewLine method from the compiler host then.

[simonhaenisch]

@mathieumg I kind of tracked it down inside Typescript:

https://github.com/microsoft/TypeScript/blob/c1f676dd3f1337286bd99d3d189bc59c930b703d/src/services/textChanges.ts#L973

This line returns a set of changes like this:

{ changes:
   [ { span: { start: 8, length: 1 }, newText: undefined },
     { span: { start: 14, length: 4 }, newText: '' },
     { span: { start: 22, length: 1 }, newText: '' },
     { span: { start: 26, length: 1 }, newText: undefined } ] }

and those two undefined newTexts are the ones that end up in the formatted code (I tried mapping them to null and then you'll have null in the code instead).

I can't really tell whether the mistake lies in the fact that formatNodeGivenIndentation returns invalid changes (i. e. undefined new text) or whether applyChanges should filter out changes that have newText: undefined (but I think it's the former). I also couldn't track down how this doesn't happen when you explicitly set newLineCharacter in the formatting options. But I'd definitely consider this a Typescript bug. I'll comment on your issue there.

[mathieumg]

I wonder if it could be related to microsoft/TypeScript@237ea52 which was for https://devblogs.microsoft.com/typescript/announcing-typescript-3-9/#preserve-newlines