Revert "Remove duplicate ForkTsCheckerWebpackPlugin for web build"

[jtbandes]

🏓 Reverts #1144

This is needed to catch TS errors in files in web/src/. By default the ForkTsChecker will look at web/tsconfig.json, which is only used for top-level .ts files in the web folder (i.e. web/webpack.config.ts) but will not typecheck files under src. An example error (with CI green) can be seen in #1151 ... this is the same error that was fixed in #1109 when I added the ForkTsChecker.

[amacneil]

Thanks for context. If you want to keep this though, I don't think you are intending to have two ForkTsCheckerWebpackPlugin enabled? It needs to either be removed from webpack.ts entirely, or provide some way to configure the shared version.

[amacneil]

Also, FYI the reason the web build is broken is I think you will need the paths configuration from webpack.ts. I don't think those are loaded correctly from tsconfig.json for whatever reason.

Separate question - why do we even have web/src/tsconfig.json? We don't follow that pattern anywhere else. Maybe it would be simpler to just remove that file.

[jtbandes]

I don't think you are intending to have two ForkTsCheckerWebpackPlugin enabled?

Well they are checking different TS files. I guess we could explore a single tsconfig that also includes the top level scripts but I'm not sure how easy that is nor if it's actually an improvement in any way.

why do we even have web/src/tsconfig.json? We don't follow that pattern anywhere else

We do also have desktop/{main,preload,renderer}/tsconfig.json...

[jtbandes]

you will need the paths configuration from webpack.ts. I don't think those are loaded correctly from tsconfig.json for whatever reason

Why do we only need these now? (Also commented at #1017 (comment))

[amacneil]

Well they are checking different TS files. I guess we could explore a single tsconfig that also includes the top level scripts but I'm not sure how easy that is nor if it's actually an improvement in any way.

This is the pattern used everywhere else in the repo, so it's less about which way is better and more just consistency. I'll make a PR to demonstrate and we can see if it correctly fails your tests.

We do also have desktop/{main,preload,renderer}/tsconfig.json...

We do have those, we don't have a second src/tsconfig.json anywhere else that I'm aware of. The config files are being caught by the root /tsconfig.json (e.g. it has an include for "./**/jest.config.ts"). Again, I'm not arguing which pattern is better, just that it's nicer if we stick to one.

Why do we only need these now? (Also commented at #1017 (comment))

Replied over there

[amacneil]

Interesting - it looks like since jest files were converted from ts to json, there are no longer any packages in the repo (other than web) which have both a src folder and ts config files in the package root. That said, every other package still keeps the tsconfig in the package root and references src, rather than having src/tsconfig.json

[jtbandes]

The config files are being caught by the root /tsconfig.json (e.g. it has an include for "./**/jest.config.ts").

Ok so it has one for jest, but not webpack... if I add some type errors to web/webpack.config.ts and run yarn tsc --noEmit at the repo root it still succeeds. We could add this but it's also easy to forget to add new patterns to the root tsconfig. I'm not certain what the right solution is but it seems like a *.ts in the root of each package is maybe likely to catch more things...

we don't have a second src/tsconfig.json anywhere else that I'm aware of

I guess I meant that these main/preload/renderer folders are kind of like a desktop/src but split apart into multiple src dirs.

Most of our other packages don't have any .ts files at the top level so that's probably why they never wanted/needed a tsconfig at the top level.

Looking at the web configs now I remembered one thing that forced us to have two separate tsconfigs: the "module": "commonjs" setting was needed for web/tsconfig, but does not work for web/src/tsconfig (don't remember details off the top of my head).

[amacneil]

Yeah, you're right. I did some experimentation in #1153, but it looks like it's not easy to tell webpack to use a specific named tsconfig.json without installing yet another module (docs).

We did have this exact problem in the past with jest.config.ts files, but it has since become a non issue, and I think jest must have had a different method of finding a relevant tsconfig (actually checking whether it was covered by include paths). Since webpack/ts-node ignores the include paths, it's kinda crazy that they even both respecting the module flag.

I think the current src/tsconfig.json solution is ok then.