Use TypeScript build mode + ESLint

[paul-marechal]

What it does

Modified our tsconfigs to use TypeScript build mode. This means we can use incremental builds, and that tsc will be able to build a package dependencies if required. Build is faster.

Replaced tslint by eslint. This is the reason this patch is so heavy, had to rewritte most of our in-source comments that disabled rules for some lines/files.

I modified some of our build script so that it wouldn't clean everything anymore, but rather make use of caches (both tsc and eslint). You can still run the clean script, building will just not destroy caches by default now.

There is an issue with typescript-eslint where you cannot lint a whole project with too many packages. I did add a lint:oneshot script in our root package, but it tries to allocate 8GB of memory so that it doesn't crash.

Until the issue is fixed, we will lint per package using lerna, like we used to do.

Full build time before: ~450s.
Build time now: ~310s first time, ~60s if nothing to do.

TypeScript is actually super fast with 1s execution time for tsc if there is nothing new. Linting + webpack bundling is what takes the most time, so it is still better to try and only trigger those when required, and not every time.

Fixes #4442
Fixes #2077

How to test

Build should still pass, you should be able to watch packages. Be worry that watching will also look at our workspace dependencies: If you watch @theia/debug and modify @theia/core, the latter will get recompiled by the watcher. I think this is actually a good thing, but it used to not work that way.

Linting should pass.

Review checklist

• as an author, I have thoroughly tested my changes and carefully followed the review guidelines

Reminder for reviewers

• as a reviewer, I agree to behave in accordance with the review guidelines

[akosyakov]

@marechal-p that's cool, could you have a look at windows build please

[akosyakov]

While reviewing i skip ts file, i assume only tslint annotations are replaced by eslint. Is it correct? @marechal-p Or there are some code changes related to semantics?

[akosyakov]

Should not be tslint and tslint-language-service removed from dev dependencies?
Does VS Code and Theia report eslint errors and quick fixes in the editor?

[akosyakov]

Have you tried to use the latest typescript and see whether it gives better perf results? There is 3.8.0-beta as well.

[akosyakov]

watching is tremendously slow :( try to run yarn watch and then change something in the about dialog, it takes around 5-6 sec to compile everything

[akosyakov]

Cross package navigation does not work anymore, try to find references from ApplicationPackage. Typescript says it is not used anymore.

Auto import does not work anymore, try to type:

    @inject(StorageService)
    protected readonly storage: StorageService;

and use ctrl space to auto import StorageService and inject

Opening ts files with Theia reports different errors:

I have not tested with VS Code, but the new setup should no yield bad editing experience in VS Code and Theia. Please include testing for it in How to test section. Maybe it is an issue with typescript, please try latest and 3.8-beta versions

[paul-marechal]

watching is tremendously slow :( try to run yarn watch and then change something in the about dialog, it takes around 5-6 sec to compile everything

The about dialog lives in @theia/core, which is a dependency for all extensions... So when watching from the root, TypeScript will see that core changed, and it will look for other packages that were depending on it and rebuild those, because maybe the change in core compiled OK, but maybe it broke dependents. At least this is what I understood from the build mode.

Maybe I can add a watch:fast script? This one wouldn't use tsc build mode and only look at your current files? watch alone would do the more expensive work of watching your TypeScript references.

Does VS Code and Theia report eslint errors and quick fixes in the editor?

It did for me on VS Code, using the ESLint extension.

While reviewing i skip ts file, i assume only tslint annotations are replaced by eslint. Is it correct?

Yes, this is correct. I did change a few theiaext scripts, and added one to compile references between ts projects. I edited small pieces of code that got picked up by eslint and that weren't picked up before, but changes are strictly minimal.

Cross package navigation does not work anymore, try to find references from ApplicationPackage. Typescript says it is not used anymore.

I might have an idea why, I will experiment a bit with this.

[akosyakov]

Let's finish first #6883 and then work on this PR with typescript extension from VS Code. Maybe it resolves some editing issues. Not having cross package navigation (in both direction) and content assist will be a blocker.

[akosyakov]

The about dialog lives in @theia/core, which is a dependency for all extensions... So when watching from the root, TypeScript will see that core changed, and it will look for other packages that were depending on it and rebuild those, because maybe the change in core compiled OK, but maybe it broke dependents. At least this is what I understood from the build mode.

It makes sense only if api are changed, don't get why ts recompiles everything when i replace some string in render method. Probably they will optimize it later. For now keeping the ability to watch individual packages as before will be good.

Maybe I can add a watch:fast script? This one wouldn't use tsc build mode and only look at your current files? watch alone would do the more expensive work of watching your TypeScript references.

just keeping watch script in each package will be enough, so one can do npx run watch @theia/core to watch only core without dependents.

[akosyakov]

I've tried 3.8-beta for #6903 (comment), but it still has a regression for Promise.all which add unnecessary undefined: microsoft/TypeScript#33752 It's marked for 3.8.1.

[paul-marechal]

@akosyakov

just keeping watch script in each package will be enough, so one can do npx run watch @theia/core to watch only core without dependents.

Done.

Not having cross package navigation (in both direction) and content assist will be a blocker.

Fixed, this was due to some kind of issue with tsserver. Solution I found was to create a "fake" tsconfig which will only be picked up by the LS with shims to help it follow imports across packages. It is basically what was done before, but I made a script to compute these path mappings automatically, as I think we were missing some packages...

Please tell me if you find any other quirks.

[akosyakov]

Please tell me if you find any other quirks.

I will try after we land built-in extensions PR and rebase this PR. Otherwise it does not make sense to test against Theia extensions. Hopefully It happens tomorrow.

[akosyakov]

@marechal-p Please rebase and I will try again.

[paul-marechal]

Rebased.

[akosyakov]

Could we move compile.tsconfig.json under config? Generally any config which does not has to be in the root has to be moved to config.

[paul-marechal]

Unfortunately, I don't think we can. Something to do with rootDir and outDir.

[paul-marechal]

Unless you mean only the root one.

[akosyakov]

Yes, only keeping root clean from many config files

[akosyakov]

I've checked that:

• one can use as existing Theia distribution (current version of Gitpod Theia for instance) and Theia from sources and still get good editor support
• that build and watch scripts are still working, for watching it is better to use individual watches as before

That's unfortunate that we cannot run eslint over all files once yet, but it for sure going in the right direction. Thank you @marechal-p! Great job!

[akosyakov]

@marechal-p please mark relevant issues as to fix

[akosyakov]

@marechal-p Have you tried this advice for linting in one go: typescript-eslint/typescript-eslint#1192 (comment) so instead of using compile.tsconfig.json, use tsconfig.json from the root which we use for editing as well then everything should be compiled as one program

[paul-marechal]

@akosyakov I will try your idea tomorrow and check if there are any issue :)

[paul-marechal]

Will merge on monday.