WIP: typescript sys proxy #231

phryneas · 2019-03-16T12:06:33Z

Well, I had teasered this in #227 and I guess it's better reviewable if I have an open PR for this branch ;)

So far, this seems to work transparently with useTypescriptIncrementalApi: true and the module resolution is working just fine as well with the latest commit.

I'm currently testing this with .mdx files, but there should not be anything stopping this to work with .vue as well.
I think I'm gonna do some testing with useTypescriptIncrementalApi: false and get that to work.

From there on, I'm gonna wait for some feedback from you:

How would you structure this? (everything in one wrapTypeScript.ts is not really feasible - would splitting off a wrapperUtils.ts and a wrapCompilerHost.ts be enough?)
Should this replace the .vue code?
Do you want to keep the .vue and .mdx specific code in this repo (now that they're going to be only about 20 lines per extension) or do you still want to have a pluggable approach (which would most likely be more code than the extension-specific handling code itself).
Overall: what do you think of this? Is this even feasible?

johnnyreilly · 2019-03-16T14:05:43Z

You have been busy 😄

How would you structure this? (everything in one wrapTypeScript.ts is not really feasible - would splitting off a wrapperUtils.ts and a wrapCompilerHost.ts be enough?)

Splitting it seems like a good idea.

Should this replace the .vue code?

Possibly - I'd be interested to see how that looks.

Do you want to keep the .vue and .mdx specific code in this repo (now that they're going to be only about 20 lines per extension) or do you still want to have a pluggable approach (which would most likely be more code than the extension-specific handling code itself).

I'm not decided on this yet. My current feeling is that Vue should remain in the box at least for now since it's already there. But I'm unlikely to add anything else - including mdx I'm afraid. I'm still keen on a pluggable solution.

Overall: what do you think of this? Is this even feasible?

I think this is potentially very interesting. I'm afraid I'm occupied with a number of other things at present and so haven't devoted too much thought to it. But I think it has merit.

In terms of sequencing I'd like to see the Vue incremental fixes land first: #220

Essentially I want to get existing users to a good place before looking at changing the underlying architecture and potentially open up the possibility of new users.

I think the work you're doing is very cool though - and I'm grateful for the effort you're putting in 🌻❤️😄

phryneas · 2019-03-16T15:46:19Z

Well, now you can take a look at how it looks with .vue ^^
Essentially, all you need is a config like specified here: https://github.com/Realytics/fork-ts-checker-webpack-plugin/blob/12e85371d95c858cf82054948d8c312476a1bd1c/src/wrapTypeScript.ts#L19
That just references this loader: https://github.com/Realytics/fork-ts-checker-webpack-plugin/blob/12e85371d95c858cf82054948d8c312476a1bd1c/src/handleVueContents.ts#L4

But right now, there are some things now working yet and tests are failing.

From what I have gathered so far what goes wrong:

as I just attach .tsx on an early file system level to all vue files without looking at the file contents, every vue file is regarded as .tsx, no special handling for .ts is possible 😢 Guess I need to figure out a smart way around that sigh
module resolution still has it's kinks (the test with the @-namespace is failing for example)
generally, a lot of .vue-tests are failing. I haven't looked at all of them yet.

phryneas · 2019-03-16T16:06:54Z

Okay, apparently nothing is too bad with my module resolution - it's just that there seems to be an implicit mapping for a virtual "@"-module in vue.

Adding something like

baseUrl: './src',
        paths: {
          '@/*': ['./*']
        }

to the compilerOptions resolves that problem.

Now, I don't know vue myself. Is that something that should implicitly be added?

johnnyreilly · 2019-03-16T16:17:39Z

Now, I don't know vue myself. Is that something that should implicitly be added?

Also not a Vue user I'm afraid; don't know 😥

phryneas · 2019-03-16T16:56:08Z

It seems like that "@"-wildcard was added by @CKGrafico - can you tell us if that should be there implicitly or if that is something a user would have added anyways to their .tsconfig to be able to use it?

phryneas · 2019-03-16T17:01:14Z

Also, ts-loader (as well as apparently babel-loader?) is going with two configuration options (appendTsSuffixTo, appendTsxSuffixTo), to determine which vue files are ts and which ones are tsx - I think I will go for a similar approach for the time being.

Or is there something actually requiring this auto-detection that is currently in there?

johnnyreilly · 2019-03-16T17:08:04Z

Or is there something actually requiring this auto-detection that is currently in there?

Not to my knowledge. If memory serves @HerringtonDarkholme and I came up with this when Vue support was added to ts-loader. See TypeStrong/ts-loader#270 and TypeStrong/ts-loader#354

phryneas · 2019-03-16T17:20:05Z

Okay, then I'll go with that for now. Makes it a little less intimidating ;)

phryneas · 2019-03-16T18:18:43Z

For now, I've added the '@' namespace to the tsconfig of the associated test - I guess that is sensitive.
Also, I've associated .vuex files with .tsx and .vue files with .ts.

This makes all but three tests green: the tests that now complain that example-js.vue, example-jsx.vue and example-nolang.vue now also report errors.

An alternative solution to using file extensions would be to preprend files from handleVueContents.ts with their filetype like // @fork-ts-checker-handle-file-as (ts|tsx|js|jsx|json|none).

Checking & using that in IncrementalChecker would be no problem as the CompilerHost is wrapped there anyways and host.getSourceFile could easily be overridden, in ApiIncrementalChecker this would require to start wrapping the CompilerHost. Not the end of the world, but it still looks so prisine - so I'll leave that approach open to discussion 😁

Aside from that, everything should be working now (?) - it would be cool if an actual vue user could test this, as I only have the test cases ;)

johnnyreilly · 2019-03-16T20:26:28Z

This makes all but three tests green: the tests that now complain that example-js.vue, example-jsx.vue and example-nolang.vue now also report errors.

Might be worth tweaking the tests to get them to get more information....

I don't get why some Vue tests would pass and some would fail. Is there a pattern? Looks like ts and tsx are fine whilst js / jsx are not. That's interesting

phryneas · 2019-03-16T21:27:44Z

No, that's pretty much expected behaviour at this point. As all .vue files are now handled as either .tsx or .ts, the three files containing non-ts-javascript now throw type errors that would have been ignored before. So the tests actually do what they are supposed to do ;)

I guess this could only be mitigated by actually handling the files by their contents, not their extension - which is why I suggested that @fork-ts-checker-handle-file-as file header comment to be injected.

But I believe won't get around to implement that this weekend - maybe next weekend :)

johnnyreilly · 2019-03-16T22:07:00Z

I guess this could only be mitigated by actually handling the files by their contents, not their extension - which is why I suggested that @fork-ts-checker-handle-file-as file header comment to be injected.

Maybe that's a good shout. If memory serves @HerringtonDarkholme had some reservations about the appendTsxSuffixTo approach.

Thanks for your continued experimentation 😄

johnnyreilly · 2019-03-17T08:42:43Z

I've just tweaked your branch so it runs the Vue tests when useTypescriptIncrementalApi is both true and false - remember to git pull 😄

phryneas · 2019-03-17T14:19:40Z

I just did some reorganization - this way there should be less changes to the original files, everything is a little more transparent.

On the part of filetype-autodetection, I'm having a little bit of a blocker. For the IncrementalChecker, that won't be a problem - I can simply hook into host.getSourceFile and set the correct scriptKind. But I don't see a similar way for the ApiIncrementalChecker - do you have any idea how I could change the scriptKind there? (except changing the file extension dynamically by file content, I'd like to avoid that)

PS: #219 does that by hooking into host.fileExists and checking the contents, deciding depending on the contents if the file "should exists" - of course I could do something like that, but then every file would get parsed at least twice. So I'd love a more elegant solution :/

phryneas · 2019-03-17T14:43:38Z

Okay... I could hook into typescript.createEmitAndSemanticDiagnosticsBuilderProgram and wrap the host parameter there once more, from there I can wrap getSourceFile even for ApiIncrementalChecker. Not very elegant but seems to be working. I've gotta give this some thought. And if you have a better idea - I'd be happy to hear it :)

johnnyreilly · 2019-03-17T15:02:08Z

Not very elegant but seems to be working. I've gotta give this some thought. And if you have a better idea - I'd be happy to hear it :)

If it's a straight choice between not working and inelegant but working I'll always go for the second option 😄

johnnyreilly · 2019-03-17T19:11:17Z

BTW - is this is what MDX is? https://gatsby-mdx.netlify.com Pretty cool!

Up until now I'd thought it was this: https://en.m.wikipedia.org/wiki/MultiDimensional_eXpressions - without having the first idea how that could be so 😁

phryneas · 2019-03-17T21:03:54Z

Oh, lol, I didn't even know MultiDimensional eXpressions ^^

Yup, that is the mdx I was talking about. Markdown with JSX. Also used in https://github.com/jxnblk/mdx-deck and https://www.docz.site/ - and even currently being implemented to be a part of storybook.

phryneas · 2019-03-17T22:03:47Z

FYI: I just noticed that your commit to run tests with useTypescriptIncrementalApi true|false is pretty pointless: Later in helpers.createVueCompiler, a IncrementalChecker is created manually, so most tests run as if useTypescriptIncrementalApi were false. Would be cool if you could look into this :)

I'm out for now - long day ;)

johnnyreilly · 2019-03-18T05:47:40Z

I just noticed that your commit to run tests with useTypescriptIncrementalApi true|false is pretty pointless: Later in helpers.createVueCompiler, a IncrementalChecker is created manually, so most tests run as if useTypescriptIncrementalApi were false.

I guess you mean this:

https://github.com/Realytics/fork-ts-checker-webpack-plugin/blob/2c03b7e45a9fc023efa3053403cc05ecff9146c0/test/integration/helpers.js#L59

This was added when initial Vue support for the incremental API was being implemented:

53d690b

@0xorial can you recall what the purpose of this was please? Presumably we shouldn't need this? This line seems particularly interesting:

https://github.com/Realytics/fork-ts-checker-webpack-plugin/blob/2c03b7e45a9fc023efa3053403cc05ecff9146c0/test/integration/helpers.js#L77

0xorial · 2019-03-18T11:23:19Z

I've looked through the history and creation of IncrementalChecker seems to be there from very beginning. I merely moved it into another file (probably because I had an intention to make 2 separate test sets for incremental=true/false), but I did not pay close attention back then I presume... So, unfortunately, I have no idea why is it there.
It have a feeling that author understood 'integration test' as 'any test that covers more than 1 file/class' while currently they are more like 'test that tests whole plugin as a black box'

phryneas · 2019-06-02T18:11:44Z

So after thinking about this some time I've put more work into this.

I've removed the code that was faking the typescript.sys object and taken a different approach for this:
Now I'm replacing the fs module with a fake one using mock-require which we had as a devDependency until now anyways. (We could also use something like monkey-fs, but those don't really do something different...)
That fake file system now modifies all paths passed to fs functions if they have specific extensions, just like I was doing it in the wrapped typescript.sys before. (X.vue <=> X.vue.fake.ts etc.)

That way, there is no special handling for this in our code any more, and almost no additional wrapping of TypeScript interna is required.

So far, this looks very promising. I will, however, have to take a look at the watching code, as currently that will emit events with un-faked filenames.
I believe it might be pretty simple to get this to work with TypeScript itself, as according to #256 it internally uses fs.watch and fs.watchFile - but our own code is using chokidar, which handles this differently. On the other hand, if we were to drop the old watch mode, supporting this might not even be necessary.

Just to give you a short report on what's happening here :)

johnnyreilly · 2019-06-02T18:13:42Z

Awesome work @phryneas!

zackschuster · 2019-06-02T18:51:59Z

~~github notified me of this because i was apparently mentioned somewhere, but i can't find the reference.~~ nvm found it

thanks for the hard work, @phryneas!

phryneas · 2019-06-09T17:26:33Z

So, I think I've got the file watching situation under control, at least for the incrementalApi: true mode. Also, I've added a pretty extensive unit test for the filesystem wrapper.

I've created a new build that can be used with

yarn add fork-ts-checker-webpack-plugin@https://registry.yarnpkg.com/@phryneas/experimental-fork-ts-checker-webpack-plugin/-/experimental-fork-ts-checker-webpack-plugin-0.0.3.tgz

As for me, I'll start dogfooding this version, but as I'm not a vue user myself I won't really see much of a change (and I still don't know any vue open source project already using fork-ts-checker-webpack-plugin to test this on). It could be great if a few vue users (@zackschuster ? 😄 ) could give this a go :)

PS: looks like the tests are breaking in the CI sometimes. file event watching is a flimsy thing, especially in a CI :(

PPS: the tests are failing for mac exclusively - but I think the code itself is right, only some mac-specifics I don't know yet in the tests. Gonna get a colleague with a Mac to run them for me next week.

johnnyreilly · 2019-06-09T18:41:52Z

Awesome work! @zackschuster would you be able to give this a try please?

Likewise I'll start dogfooding when I'm back working on front end stuff again (hopefully in a week 🤞)

zackschuster · 2019-06-10T18:02:00Z

@johnnyreilly @phryneas i don't currently have any vuejs projects that use typescript but i'll give it a shot when i'm back on my dev machine.

are there any known concerns i can give special attention to?

johnnyreilly · 2019-06-10T20:45:12Z

Thanks @zackschuster! It would be great to know if experimentalWatchApi: true and experimentalWatchApi: false still work as expected. Also that a production build still works with Vue.

Anything else @phryneas ?

phryneas · 2019-06-10T21:34:37Z

@johnnyreilly I think you mixed the experimentalWatchApi up with ts-loader, in fork-ts-checker-webpack-plugin it's incrementalApi 😋

@zackschuster Thanks a lot!
It would be great if you could test it with incrementalApi: true and incrementalApi: false.
Typechecking in general, but especially if .vue files are being watched.
Vue with incrementalApi: true wasn't working before and should be working now, but on the downside this patch might be breaking file watching with incrementalApi: false. I'm especially interested if that's the case (also, if it really works with true of course 😄 )

There's one minor breaking change I can think of: Before, vue files were seen by the typescript compiler as filename.vue.ts, now they're seen as filename.vue.__fake__.ts - this should not change anything if your tsconfig.json is just including all .ts files, but if it only includes specific files you might need to change that there.

WolfspiritM · 2019-06-11T14:50:02Z

Hi, Sorry that I wasn't able to continue on the vue fix but I gave this a try on our (not open source, sorry) project.

TL;DR

Fastest was this experimental plugin without incrementalApi.
Linter won't be able to show codeframe anymore due to the "fake" name I think.
With incrementalApi enabled there is a HUGE (as in VERY HUGE...6 Minutes compared to 20 Seconds without incrementalApi) performance drop on initial build and also on incremental builds.
With incrementalApi enabled Linter errors are not reported immediatly but instead the compilation is done within milliseconds (I assume it fails silently?) after changing something else the compilation takes longer and finally reports the error.

For incremental building I changed a string in a *.vue file. Nothing big and always the same one.

Config

 new ForkTsCheckerWebpackPlugin({
                tslint: true,
                vue: true,
                useTypescriptIncrementalApi: ....,
                async: false,
                formatter: "codeframe",
                measureCompilationTime: true,
            }),

Tests

"fork-ts-checker-webpack-plugin" (watch)

Configuration: useTypescriptIncrementalApi: false

initial build:

Using 1 worker with 2048MB memory limit
webpack is watching the files…
Compilation took: 22557.66 ms.```

incremental build 1: Compilation took: 9706.11 ms.
incremental build 2: Compilation took: 5526.23 ms.
incremental build 3: Compilation took: 5517.79 ms.

syntax error build:

  Compilation took: 5711.98 ms
ERROR in C:\src\Project\scripts\components\Admin\AdminManager.vue.ts
./scripts/components/Admin/AdminManager.vue?vue&type=script&lang=ts& (C:/src/Project/node_modules/ts-loader??ref--2-0!C:/src/Project/node_modules/vue-loader/lib??vue-loader-options!./scripts/components/Admin/AdminManager.vue?vue&type=script&lang=ts&)
[tsl] ERROR in C:\src\Project\scripts\components\Admin\AdminManager.vue.ts(87,29)
      TS1005: ',' expected.
 @ ./scripts/components/Admin/AdminManager.vue?vue&type=script&lang=ts& 1:0-193 1:209-212 1:214-404 1:214-404
 @ ./scripts/components/Admin/AdminManager.vue
 @ ./scripts/app.ts
 @ multi ./scripts/app.ts

lint error build:

   Compilation took: 5689.21 ms.
   ERROR in C:/src/Project/scripts/components/Admin/AdminManager.vue
   ERROR in C:/src/Project/scripts/components/Admin/AdminManager.vue
   95:1 Consecutive blank lines are forbidden
       93 |     // tslint:enable:object-literal-sort-keys
       94 | 
     > 95 | 
          | ^
       96 |     private created() {
       97 |         this.loadAll({from: 0, count: 20});
       98 |     }

type error build:

 Compilation took: 6052.88 ms.
 ERROR in C:/src/Project/scripts/components/Admin/AdminManager.vue
 ERROR in C:/src/Project/scripts/components/Admin/AdminManager.vue
 95:13 Type '0' is not assignable to type 'string'.
     93 |     // tslint:enable:object-literal-sort-keys
     94 | 
   > 95 |     private shouldBeString: string = 0;
        |             ^
     96 | 
     97 |     private created() {
     98 |         this.loadAll({from: 0, count: 20});

"@phryneas/experimental-fork-ts-checker-webpack-plugin" (watch)

Configuration: useTypescriptIncrementalApi: false

initial build:

Using 1 worker with 2048MB memory limit
webpack is watching the files…
Compilation took: 19435.08 ms.

incremental build 1: Compilation took: 4081.24 ms
incremental build 2: Compilation took: 3112.74 ms
incremental build 3: Compilation took: 2911.64 ms

syntax error build:

Compilation took: 3098.1 ms.
ERROR in C:\src\Project\scripts\components\Admin\AdminManager.vue.ts
./scripts/components/Admin/AdminManager.vue?vue&type=script&lang=ts& (C:/src/Project/node_modules/ts-loader??ref--2-0!C:/src/Project/node_modules/vue-loader/lib??vue-loader-options!./scripts/components/Admin/AdminManager.vue?vue&type=script&lang=ts&)
[tsl] ERROR in C:\src\Project\scripts\components\Admin\AdminManager.vue.ts(87,26)
      TS1005: ',' expected.
 @ ./scripts/components/Admin/AdminManager.vue?vue&type=script&lang=ts& 1:0-193 1:209-212 1:214-404 1:214-404
 @ ./scripts/components/Admin/AdminManager.vue
 @ ./scripts/app.ts
 @ multi ./scripts/app.ts

lint error build:

 Compilation took: 2393.22 ms.
 ERROR in C:/src/Project/scripts/components/Admin/AdminManager.vue.__fake__.ts
 ERROR in C:/src/Project/scripts/components/Admin/AdminManager.vue.__fake__.ts
 95:1 Consecutive blank lines are forbidden

type error build:

 Compilation took: 2815.3 ms
 ERROR in C:/src/Project/scripts/components/Admin/AdminManager.vue
 ERROR in C:/src/Project/scripts/components/Admin/AdminManager.vue
 95:13 Type '0' is not assignable to type 'string'.
     93 |     // tslint:enable:object-literal-sort-keys
     94 | 
   > 95 |     private shouldBestring: string = 0;
        |             ^
     96 | 
     97 |     private created() {
     98 |         this.loadAll({from: 0, count: 20});

"@phryneas/experimental-fork-ts-checker-webpack-plugin" (watch)

Configuration: useTypescriptIncrementalApi: true

initial build:

  Using 1 worker with 2048MB memory limit
  webpack is watching the files…
  Compilation took: 400250.35 ms.

incremental build 1: Compilation took: 11017.46 ms.
incremental build 2: Compilation took: 16267.02 ms.
incremental build 3: Compilation took: 10982.65 ms.
incremental build 4: Compilation took: 10485.2 ms.
incremental build 5: Compilation took: 10634.52 ms.

syntax error build:

Compilation took: 3098.1 ms.
ERROR in C:\src\Project\scripts\components\Admin\AdminManager.vue.ts
./scripts/components/Admin/AdminManager.vue?vue&type=script&lang=ts& (C:/src/Project/node_modules/ts-loader??ref--2-0!C:/src/Project/node_modules/vue-loader/lib??vue-loader-options!./scripts/components/Admin/AdminManager.vue?vue&type=script&lang=ts&)
[tsl] ERROR in C:\src\Project\scripts\components\Admin\AdminManager.vue.ts(87,29)
      TS1005: ',' expected.
 @ ./scripts/components/Admin/AdminManager.vue?vue&type=script&lang=ts& 1:0-193 1:209-212 1:214-404 1:214-404
 @ ./scripts/components/Admin/AdminManager.vue
 @ ./scripts/app.ts
 @ multi ./scripts/app.ts

lint error build: Compilation took: 88.46 ms. (No error reported!)

lint error build (changed something else unrelated):

 Compilation took: 15772.66 ms.
 ERROR in C:/src/Project/scripts/components/Admin/AdminManager.vue.__fake__.ts
 ERROR in C:/src/Project/scripts/components/Admin/AdminManager.vue.__fake__.ts
 95:1 Consecutive blank lines are forbidden

type error build:

 Compilation took: 11127.61 ms.
 ERROR in C:/src/Project/scripts/components/Admin/AdminManager.vue
 ERROR in C:/src/Project/scripts/components/Admin/AdminManager.vue
 95:13 Type '0' is not assignable to type 'string'.
     93 |     // tslint:enable:object-literal-sort-keys
     94 | 
   > 95 |     private shouldBeString: string = 0;
        |             ^
     96 | 
     97 |     private created() {
     98 |         this.loadAll({from: 0, count: 20});

phryneas · 2019-06-11T16:35:14Z

@WolfspiritM phew, these results are a bummer 😞
Thank you for testing though, it's a great help! 👍
One thing I would be interested in: could you give the tests a second round with async: true? There seems to be weird behaviour with different values of async sometimes, so I'm interested if that makes a difference.

WolfspiritM · 2019-08-13T22:34:57Z

Sorry for the late response. While trying to figure out TypeStrong/ts-loader#988 I gave this a try again as it's more complicated to fix ts-loader and maybe ts-loader can use the mocked FS aswell as I was playing around with a similar way to get ts-loader working correctly by replacing ts.sys. But why reimplementing the wheel as you've already done that here.

I think I figured out why it is so slow.

It seems like the initial compile is fast (10 seconds compared to more then 20-30 before) but it's the linter that is very slow. The reason seems to be the call to getProgram() as the program seems to be always invalidated and recreated for every single file. The reason why that happens is that typescript seems to invalidate the program as soon as the host does custom module resolution.

https://github.com/microsoft/TypeScript/blob/01e1b1bb276f48eaa0612a34dd8f1eec5d86bd49/src/compiler/watch.ts#L805

I then removed the "resolveModuleNames" function from the CompilerHost as it didn't seem to do anything special here and everything was done within 10 seconds!

On watch it did first recompile with about 1 second and afterwards it even compiled within 300ms! Awesome!

The project I was testing with usually took around 30 Seconds to compile and 5-10 seconds to recompile.

Here are the changes I made: WolfspiritM@bd913cd

EDIT: Tested type checks and linter checks and both seem to work fine for normal mode aswell as watch mode

phryneas · 2019-08-14T07:05:49Z

This is awesome, thank you for taking another look! And for those awesome findings! I'm currently on vacation but I'll spend some time with this once I'm back home. -- Sent from my Android phone with K-9 Mail. Please excuse my brevity.

zackschuster · 2019-10-08T21:42:41Z

@johnnyreilly @phryneas fyi i haven't done any testing for vue and i don't plan on using it for future projects, so i won't be following through on testing it. my apologies!

WolfspiritM · 2019-11-22T13:18:02Z

@phryneas Anything new?

phryneas · 2019-11-23T14:38:48Z

@WolfspiritM I'd love to get to this, but seeing that I'd have to essentially re-do the whole PR for a third or fourth time on the current master (this is too far away for a simple rebase), I just haven't found the time for it yet.
I hope to maybe get to it in the holidays after new year's, but that's the earliest I'm seeing for this.
If you want to take a go with it before that, feel free to do so though :)

WolfspiritM · 2019-11-25T16:30:22Z

I've redone the changes on master and pushed everything to a new branch:
https://github.com/WolfspiritM/fork-ts-checker-webpack-plugin/tree/vue-fake-filesystem

I have some problems to get the tests working. Even on master the vue tests take ages to complete for me. And then fail with:
Timeout - Async callback was not invoked within the 60000ms timeout specified by jest.setTimeout.Error: Timeout - Async callback was not invoked within the 60000ms timeout specified by jest.setTimeout.
Not sure yet why that is the case. Same happens on my branch.

Also I had to remove the precommit hooks (locally).
Husky doesn't let me commit anything otherwise.
Is it possible that master is having eslint issues?
On a freshly pulled master:

yarn run v1.19.2
$ tslint --project src/tsconfig.json && eslint ./test

ERROR: C:/Work/fork-ts-checker-webpack-plugin2/src/FilesRegister.ts:5:27 - Module 'eslint' is not listed as dependency in package.json
ERROR: C:/Work/fork-ts-checker-webpack-plugin2/src/NormalizedMessageFactories.ts:6:25 - Module 'eslint' is not listed as dependency in package.json
ERROR: C:/Work/fork-ts-checker-webpack-plugin2/src/index.ts:244:5 - Multiple variable declarations in the same statement are forbidden
ERROR: C:/Work/fork-ts-checker-webpack-plugin2/src/index.ts:276:5 - Multiple variable declarations in the same statement are forbidden
ERROR: C:/Work/fork-ts-checker-webpack-plugin2/src/index.ts:305:31 - Module 'eslint' is not listed as dependency in package.json

error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I've merged the settings with the patchTypescript config.
With that branch everything rebuilds within 500ms with one worker...with the current master it takes about >=10 Seconds with multiple workers.

@phryneas Can you maybe help me with the tests?

phryneas · 2019-11-25T21:34:10Z

I've redone the changes on master and pushed everything to a new branch:
https://github.com/WolfspiritM/fork-ts-checker-webpack-plugin/tree/vue-fake-filesystem

I have some problems to get the tests working. Even on master the vue tests take ages to complete for me. And then fail with:
Timeout - Async callback was not invoked within the 60000ms timeout specified by jest.setTimeout.Error: Timeout - Async callback was not invoked within the 60000ms timeout specified by jest.setTimeout.
Not sure yet why that is the case. Same happens on my branch.

Hm. Back then, the vue integration tests were running at 150-200 seconds. Since then, they almost doubled with #355, and they take about 350 seconds on my system with your branch. So while that is a lot of time, it reflects what I would expect, considering those integration tests spawn a webpack instance for almost every test.

I have no timeouts though. Maybe that's a Windows thing?

I would go through those tests and run them test by test using it.only instead of just it.

Also I had to remove the precommit hooks (locally).
Husky doesn't let me commit anything otherwise.

You can always commit by force with git commit --no-verify.

Is it possible that master is having eslint issues?
On a freshly pulled master:

yarn run v1.19.2
$ tslint --project src/tsconfig.json && eslint ./test

ERROR: C:/Work/fork-ts-checker-webpack-plugin2/src/FilesRegister.ts:5:27 - Module 'eslint' is not listed as dependency in package.json
ERROR: C:/Work/fork-ts-checker-webpack-plugin2/src/NormalizedMessageFactories.ts:6:25 - Module 'eslint' is not listed as dependency in package.json
ERROR: C:/Work/fork-ts-checker-webpack-plugin2/src/index.ts:244:5 - Multiple variable declarations in the same statement are forbidden
ERROR: C:/Work/fork-ts-checker-webpack-plugin2/src/index.ts:276:5 - Multiple variable declarations in the same statement are forbidden
ERROR: C:/Work/fork-ts-checker-webpack-plugin2/src/index.ts:305:31 - Module 'eslint' is not listed as dependency in package.json

error Command failed with exit code 2.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

I can confirm that - the affected lines have last been touched in #305. @johnnyreilly, do you remember anything related from that PR?

I've merged the settings with the patchTypescript config.
With that branch everything rebuilds within 500ms with one worker...with the current master it takes about >=10 Seconds with multiple workers.

@phryneas Can you maybe help me with the tests?

WolfspiritM · 2019-11-26T00:07:37Z

I have no timeouts though. Maybe that's a Windows thing?

Good guess! It was a windows thing. Interestingly the tests don't seem to run on windows as webpack is having issues. adding some console.logs allowed me to catch many errors like that:

Module parse failed: Unexpected token (2:0)
    File was processed with these loaders:
     * ../../../../../node_modules/vue-loader/lib/index.js
    You may need an additional loader to handle the result of these loaders.
    |
    > <div class="hello">
    |   <h1>Example</h1>
    | </div>

However...that's another issue. I am now using WSL to test and it works fine. That way I was able to figure out that all that was left was adding corresponding __fake__.ts to the tsconfig includes. I pushed that to my branch and on my end all tests seem to pass now.

I'm wondering if it's okay to have the user specify the suffix in tsconfig (and add it to the readme) or if we need to figure out a way to rewrite the tsconfig aswell (including all "extend"s)

johnnyreilly · 2019-11-26T06:27:54Z

@johnnyreilly, do you remember anything related from that PR?

Could you be more specific? I'm not sure what the question is 😄

Side note: I mean to do this sometime soon: #356

phryneas · 2019-11-26T07:17:41Z

@johnnyreilly, do you remember anything related from that PR?

Could you be more specific? I'm not sure what the question is smile

Side note: I mean to do this sometime soon: #356

These eslint warnings all seemed to be "last touched" by that PR. But, also, taking a second look, currently eslint should only execute on the test' folder, but those errors are inside src` o_O

Anyways, I guess they'll be gone after #356, so lets ignore them for now.

phryneas · 2019-11-26T07:19:04Z

I'm wondering if it's okay to have the user specify the suffix in tsconfig (and add it to the readme) or if we need to figure out a way to rewrite the tsconfig aswell (including all "extend"s)

If you can get that to work reliably, I think that'd be cool!

piotr-oles · 2020-05-23T18:40:49Z

I will close it as I was able to release v5.0.0-alpha.1 version :)

phryneas added 2 commits March 10, 2019 21:20

wrap typescript.sys in a proxy for handling of different file extensions

75ece48

working module resolution

03f56ab

phryneas added 2 commits March 16, 2019 15:12

fix for useTypescriptIncrementalApi: false

cb5f6ac

make wrapTypeScript configurable from the outside, start vue integration

12e8537

identify vuex by extension, not by contents

49c25be

skip VueProgram tests since VueProgram was deleted

8ebdb38

johnnyreilly mentioned this pull request Mar 17, 2019

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

Closed

make vue tests run using useTypescriptIncrementalApi true and false

6fa928b

wrap creation of compilerHosts - less code changes to master

f8d1827

phryneas added 2 commits June 9, 2019 18:39

handle watching & promisifying, add test

d17dcd4

remove obsolete test

fc2d2b7

try to make tests more resilient in the CI

66f4381

phryneas mentioned this pull request Jul 15, 2019

Plugin runs without running loaders #314

Closed

johnnyreilly mentioned this pull request Sep 12, 2019

Feature: Support incremental build TypeStrong/ts-loader#999

Closed

ark120202 mentioned this pull request Sep 13, 2019

fix: support long running "watch" lint sessions typescript-eslint/typescript-eslint#973

Merged

phryneas mentioned this pull request Dec 17, 2019

feat: drop support for multi-process mode #368

Merged

piotr-oles closed this May 23, 2020

WIP: typescript sys proxy #231

WIP: typescript sys proxy #231

Conversation

phryneas commented Mar 16, 2019

johnnyreilly commented Mar 16, 2019

phryneas commented Mar 16, 2019

phryneas commented Mar 16, 2019

johnnyreilly commented Mar 16, 2019

phryneas commented Mar 16, 2019

phryneas commented Mar 16, 2019

johnnyreilly commented Mar 16, 2019 • edited

phryneas commented Mar 16, 2019 • edited

phryneas commented Mar 16, 2019 • edited

johnnyreilly commented Mar 16, 2019 • edited

phryneas commented Mar 16, 2019

johnnyreilly commented Mar 16, 2019 • edited

johnnyreilly commented Mar 17, 2019

phryneas commented Mar 17, 2019 • edited

phryneas commented Mar 17, 2019 • edited

johnnyreilly commented Mar 17, 2019

johnnyreilly commented Mar 17, 2019

phryneas commented Mar 17, 2019

phryneas commented Mar 17, 2019

johnnyreilly commented Mar 18, 2019 • edited

0xorial commented Mar 18, 2019

phryneas commented Jun 2, 2019

johnnyreilly commented Jun 2, 2019

zackschuster commented Jun 2, 2019 • edited

phryneas commented Jun 9, 2019 • edited

johnnyreilly commented Jun 9, 2019

zackschuster commented Jun 10, 2019

johnnyreilly commented Jun 10, 2019

phryneas commented Jun 10, 2019 • edited

WolfspiritM commented Jun 11, 2019 • edited

TL;DR

Config

Tests

"fork-ts-checker-webpack-plugin" (watch)

Configuration: useTypescriptIncrementalApi: false

"@phryneas/experimental-fork-ts-checker-webpack-plugin" (watch)

Configuration: useTypescriptIncrementalApi: false

"@phryneas/experimental-fork-ts-checker-webpack-plugin" (watch)

Configuration: useTypescriptIncrementalApi: true

phryneas commented Jun 11, 2019

WolfspiritM commented Aug 13, 2019 • edited

phryneas commented Aug 14, 2019 via email

zackschuster commented Oct 8, 2019

WolfspiritM commented Nov 22, 2019

phryneas commented Nov 23, 2019

WolfspiritM commented Nov 25, 2019 • edited

phryneas commented Nov 25, 2019 • edited

WolfspiritM commented Nov 26, 2019

johnnyreilly commented Nov 26, 2019

phryneas commented Nov 26, 2019

phryneas commented Nov 26, 2019

piotr-oles commented May 23, 2020

johnnyreilly commented Mar 16, 2019 •

edited

phryneas commented Mar 16, 2019 •

edited

phryneas commented Mar 16, 2019 •

edited

johnnyreilly commented Mar 16, 2019 •

edited

johnnyreilly commented Mar 16, 2019 •

edited

phryneas commented Mar 17, 2019 •

edited

phryneas commented Mar 17, 2019 •

edited

johnnyreilly commented Mar 18, 2019 •

edited

zackschuster commented Jun 2, 2019 •

edited

phryneas commented Jun 9, 2019 •

edited

phryneas commented Jun 10, 2019 •

edited

WolfspiritM commented Jun 11, 2019 •

edited

WolfspiritM commented Aug 13, 2019 •

edited

WolfspiritM commented Nov 25, 2019 •

edited

phryneas commented Nov 25, 2019 •

edited