Cleanup Theia's core extension APIs

[paul-marechal]

Almost no other npm package requires you to import symbols from nested files, e.g.

// Few examples of conventional package APIs:
import { Container, injectable, inject, postConstruct } from 'inversify';
import { Command, TextLine, TextDocument, Uri, window } from 'vscode';
import { expect } from 'chai';
import { ChildProcess, spawn, fork, ForkOptions } from 'child_process';

Theia's API is currently scattered in various files, and not properly exposing those through a common index.ts entry-point makes internal refactorings painful as renaming files or moving symbols will break dependents.

One problem is that Theia packages often contain code for multiple runtimes, which means there can't really be a single entry point.

It would still help if we exposed all public symbols through each entry point, e.g.

import { ... } from '@theia/core/lib/browser';
import { ... } from '@theia/core/lib/common';
import { ... } from '@theia/core/lib/node';

In some situations we already have index.ts files that re-export symbols but it is done in a blanket way and symbols can be missing, e.g.

export * from './a';
export * from './b';
export * from './c';
// ...
// But since then, files './d' and './e' got added but are not re-exported.
// Moreover some symbols from './b` are internal and aren't considered public API.

As a rule of thumb, we should never have to go one level below @theia/<package>/lib/<runtime> to import a symbol from another extension. Doing so would imply relying on non-public/non-standard internal API and we reserve ourselves the right to change it for any reason.

Re-exports should also look a bit more like the following:

export { PublicClassA, OtherPublicClassA } from './a';
export { PublicClassB } from './b';

This way we get a more fine-grained control over what gets exposed so that we minimize the leak of internal classes.

[colin-grant-work]

I support this proposal under the general heading of API management. At the moment, we don't have much of notion of what parts of our code represent API that we should really avoid changing. In principle, every method and every field of every class becomes API as soon as it's merged, and every change of signature, however minor, is potentially breaking for somebody. It would be a great step forward for the framework to establish a way of managing adopters' expectations about what will be maintained across releases and when they may be venturing into more unstable waters.

[msujew]

While we're add it, I would suggest to re-export the /lib/ directory under it's parent path, so that we don't expose our directory structure. We can have something like this in the package root:

export * from './lib/browser';

That allows us to reference stuff like @theia/core/browser instead of @theia/core/lib/browser. It's nothing major, but just improves the look and feel when consuming Theia code (which also applies to our own repo).

[paul-marechal]

I'm having an unpleasant issue trying to make this happen: We export both vanilla TS/JS components along with Inversify components from our core/common modules. If we import an Inversify component decorated with @injectable() it automatically requires your Node process to have the reflect-metadata package loaded.

From there either:

1. We go along with it, everyone will need to make sure their runtimes are patched with reflect-metadata.
2. We separate Inversify components out of common folders and/or node folder.

I could be in favor of (1), as it requires the least refactoring... Note that (1) is only an issue when dealing with code in subprocesses, the backend process is already patched.

[msujew]

I'd also be in favor of 1. It seems to be the most straightforward and easily explainable solution.

[colin-grant-work]

What are the circumstances under which Theia code would be running in processes that haven't loaded reflect-metadata? Is this happening in the plugin host?

[paul-marechal]

Plugin host used to not require reflect-metadata and I think the RPC changes made it required.

Any Node process acting as an IPC server that would require stuff from @theia/core or other APIs containing Inversify classes would need to require reflect-metadata in its entry-point now to load properly.