WIP: Fix incremental api for vue files (#219)

[WolfspiritM]

This is a first try to fix #219
As I'm new to the typescript compiler I'm not sure if it's the right way to do it.
Please let me know 😁

It adds ".ts" extension to all vue files by using the hooks.
It also hooks into getSourceFile of the program to return .vue.ts files as .vue so the linter is using the correct filename.

It is pretty similar to VueProgram which is not used in the Incremental API case (CompilerHost).
I also pushed the vue config down to the CompilerHost so any additional vue related code is only executed if vue is enabled.

This works as long as tsconfig does have "noEmitOnError" set to true.
Otherwise type errors don't seem to be reported correctly at all.
This seems to be like another issue so I'm going to open a new issue for that.

[WolfspiritM]

I noticed that this implementation has troubles with internal filenames on type errors.
They are displayed as *.vue.ts

So I think a better approach would be to create the Host with allowNonTsExtensions and overwrite resolveModuleNames as VueProgram does, but I wasn't able to figure out how to set allowNonTsExtensions yet. I will take a closer look tomorrow so I'll mark this as WIP.

[johnnyreilly]

Thanks so much for digging into this! Well done! 😄

Let me know when you think you're ready and I'll take a look. Thanks again!

[johnnyreilly]

Hey @WolfspiritM,

How's it going? Are you still working on this?

[WolfspiritM]

@johnnyreilly Hi, Yes I'm working on it right now but having some problems. The problem I had was easily fixed by stripping the .ts extension from the diagnostics. That way upcoming tools like displaying the codeframe was able to find the file. But the main problem is the current way of "monkeypatching" getSourceFile won't work for the IncrementalApi as it creates the "real" CompilerHost "on the fly" and getSourceCode is a CompilerHost function.
https://github.com/Microsoft/TypeScript/blob/9bd23652ef8c4e6a614a2f27c467b1a68ce3340e/src/compiler/watch.ts#L599
But even overwriting that doesn't work as getSourceFileByPath uses the variable getNewSourceFile which uses the original getSourceFile function. And overwriting getSourceFileByPath did throw errors cause it's missing something that is done by getVersionedSourceFileByPath

https://github.com/Microsoft/TypeScript/blob/9bd23652ef8c4e6a614a2f27c467b1a68ce3340e/src/compiler/watch.ts#L753

I got it working as expected by overwriting the hosts readFile and readDirectory but then was stuck with tsx support as typescript is checking the filename for that and without overwriting getSourceFile there is no way to let it know the scriptkind.
I tried overwriting resolveModuleNames and give back ".tsx" as extension but typescript doesn't care about that.

So right now I'm overwriting readDirectory, fileExists and readFile of the host to let typescript think it's dealing with actual .ts and .tsx files. But the only way I can get it work is by reading the file on each access to figure out if it's a .vue.ts or .vue.tsx
e.g.

function readDirectory() {
  return originalReadDirectory().map(x => VueProgram.isVue(x) ? getExtensionsFromScriptBlock(x) : x);
}

That should work but I'm not sure if that's a good approach.

I wonder if it might be better to implement it similar to ts-loader and have loader properties:
appendTsSuffixTo
appendTsxSuffixTo

That way tsx code needs to go not in ".vue" but in ".vuex" for example and that way we can easily decide if to extend it with .ts or with .tsx.

Any thoughts?

[WolfspiritM]

This is what I came up with so far. There are still some tests failing which might all be related to a single error. The incremental API Version reports that error and I'm not sure why right now:

ERROR TS18003: No inputs were found in config file '.../tsconfig-imports.json'. Specified 'include' paths were '["imports/Test.vue"]' and 'exclude' paths were '["node_modules"]'.

@johnnyreilly Could you take a look at it please and let me know what you think?

[johnnyreilly]

There are still some tests failing which might all be related to a single error. The incremental API Version reports that error and I'm not sure why right now:

According to Travis, all the tests are passing? 😊

I wonder if it might be better to implement it similar to ts-loader and have loader properties:
appendTsSuffixTo
appendTsxSuffixTo

That way tsx code needs to go not in ".vue" but in ".vuex" for example and that way we can easily decide if to extend it with .ts or with .tsx.

Any thoughts?

It's an approach that seems to have served ts-loader well. What would be the implications of switching to this approach? Do you want to try it out and report back?

[johnnyreilly]

ping @WolfspiritM 😄

According to Travis, all the tests are passing? ... Does this mean the approach works? Or is there a scenario the tests aren't covering?

[WolfspiritM]

@johnnyreilly They pass cause it currently includes the check for vue that has been pushed to the master. So it's not using the incremental api for the vue tests. I won't have the time to look further into it until the weekend, sorry. 😕

[johnnyreilly]

Don't worry - take your time. We completely appreciate the help you're providing @WolfspiritM - there's no rush ❤️🌻

[johnnyreilly]

Heya @WolfspiritM

I've just tweaked your branch so it runs the Vue tests when useTypescriptIncrementalApi is both true and false - I should have done this when we first added useTypescriptIncrementalApi support 😄

Also, reviewing some of your comments over the course of this PR I wonder if you might be interested in taking a look at some of the ideas in @phryneas PR here: #231

It has similar experimentation / thoughts around the module resolution which might be useful / interesting to you. (Or not - but there seems to be overlap and so I thought I'd draw your attention to it in case it's helpful)

[piotr-oles]

I will close it as there is no activity here, and I was able to release 5.0.0-alpha.1 version :)