Remove explicit dependency on terminal

[thegecko]

Description

There are many cases when the terminal functionality isn't required. It gives full access to the system being run in a web context and should be easily turned off. e.g. #5663

The problem is, many extensions have a direct or indirect dependency on terminal (e.g. plugin-ext, debug, cpp), so taking any of these means the terminal functionality appears.

As discussed in the dev meeting, there are a few ways this could be resolved:

• Split each extension requiring terminal into two parts; a base part and the part needing the terminal for extended functionality
• Implement a dummy implementation of terminal in theia/core which will satisfy all the plugins, but not register any commands or menus. Including the terminal extension would override this dummy.

I think the last option here makes most sense, but as both may require large changes, I want to get some input from others first.

@marcdumais-work @marechal-p @akosyakov @svenefftinge Thoughts?

[akosyakov]

Split each extension requiring terminal into two parts; a base part and the part needing the terminal for extended functionality

I was thinking along these lines:

• @theia/terminal-api extension which only defines no-op implementations with interfaces, i.e. like we do for QuickPickService.
• @theia/terminal is an implementation of @theia/terminal-api

Other extensions can depend only on @theia/terminal-api. @theia/terminal should be added to a concrete product to get terminal actually to work if needed. Some APIs can be marked as optional with ? that clients can react if API is not implemented, i.e. error reporting.

I think it is the standard way how such issues are resolved for OSGI bundles, for example.

[thegecko]

We discussed that approach in the meeting (similar to a header file and concrete implementation split), but @svenefftinge wasn't keen.

[paul-marechal]

@theia/terminal-api extension which only defines no-op implementations with interfaces, i.e. like we do for QuickPickService.

I am not so sure about no-ops, thinking about why debug depends on the terminals. In a perfect world it would be nice to be able to not include the terminals, at all, and have the debug adapter client in Theia report that runInTerminal reverse requests are not supported.

If we want to keep the package count minimal, maybe we can make each Theia package contribute multiple Inversify modules? (the theiaExtensions array the generators look up to build apps).

Then all we lack is a way to precisely pick which modules we want to checkout.

The use case I had in mind concerned the @theia/terminal extension: It currently depends on @theia/editor for instance. @marcdumais-work tried to make a Theia application that would only provide you with terminals, but because of the dependencies we end up with a bigger application than expected. When you look at it, the editor is only required when you want to open links prompted in stdout. So if we break apart functionality in different Inversify modules and allow people to only check some out, I think it could solve this kind of issues. In this case we would prevent @theia/editor from being loaded in our application, and only load the @theia/terminal part that doesn't depend on something else than @theia/core.

I think it is a matter of splitting functionality more than what we currently do, either by having more packages or setting up something to control what theia module extensions to precisely load. I am not fond of having stub implementations that need to be overriden.

Another example: @theia/preferences. If you do not include this extension, your application will most likely fail. The reason is that the concept of having a configuration system is not optional from @theia/core point of view, yet we leave it unbound. I want your opinions here, but I think we should have the actual core service implementations in @theia/core, and @theia/preferences would add more complex features, such as the edition of the configuration in editors, ability to load configuration from user home and workspaces, etc...

My opinion is that each package does too much at the moment to allow for granular composition of an application. We either need to split in more packages, or split in more modules and being able to pick what to use within each package.

[akosyakov]

We either need to split in more packages, or split in more modules and being able to pick what to use within each package.

I don't think we want to go into picking modules. I would prefer to keep package.json as definition of what extension package is exposing and what application packages are consuming.

I think it is a matter of splitting functionality more than what we currently do, either by having more packages or setting up something to control what theia module extensions to precisely load. I am not fond of having stub implementations that need to be overriden.

The point is not in stabbing, but separating definition from the implementation. Also It is not something that I'm inventing. We have bundle, feature (bundle composition) and product (bundle+feature composition) definitions in Eclipse for long time. You can see in plain java world, i.e you can pick junit api as dev dependency and another module as its implementation at runtime.

The use case I had in mind concerned the @theia/terminal extension: It currently depends on @theia/editor for instance. @marcdumais-work tried to make a Theia application that would only provide you with terminals, but because of the dependencies we end up with a bigger application than expected. When you look at it, the editor is only required when you want to open links prompted in stdout. So if we break apart functionality in different Inversify modules and allow people to only check some out, I think it could solve this kind of issues. In this case we would prevent @theia/editor from being loaded in our application, and only load the @theia/terminal part that doesn't depend on something else than @theia/core.

In my proposal @theia/terminal should check using @theia/editor-api whether implementation is available and only when contribute editor related bindings.

Another example: @theia/preferences. If you do not include this extension, your application will most likely fail. The reason is that the concept of having a configuration system is not optional from @theia/core point of view, yet we leave it unbound. I want your opinions here, but I think we should have the actual core service implementations in @theia/core, and @theia/preferences would add more complex features, such as the edition of the configuration in editors, ability to load configuration from user home and workspaces, etc...

It still won't help, since you need then dependency to @theia/filesystem to make it work at all. We are not going to pull it in core. Core is just about the application framework and widgets. It has stubs API supporting it, not implementation of them. What about other stubs (definitions) like QuickPickService should we pull @theia/monaco in core? These design decisions were made to allow separation of concerns, mixing everything in one huge package does not help.

We could improve though process of picking extensions, like stub implementations can warn that real implementation is missing and such can be used. It should be suppressible then.

[akosyakov]

While reviewing PRs, I'm generally frustrated with how hard it to add new functionality and how cross package functionality actually couple everything together. 😢

For example, let's say we notice that @theia/terminal pulls @theia/editor as a dependency and requested changes. What contributor can do?

• Add another package @theia/terminal-editor? it does not scale because we then need all kind of combinations of projects names. And does it worth to wait additional time to start up tsc/tslint process to build all small packages?
• Design new abstract API in core which provides means to implement such thing just for this case. It is kind of a lot of work and requires proper review to accept such thing. That's a way which we "do" currently. Long term is good, but it slows down progress, especially if one just want to get a feature done. And if it does not then it couples.

It would be nice to make it simple. If each package is split to API (i don't mean that we should use typescript interfaces here, I hate boilerplate) and implementation sides, then:

• A contributor can add a dependency to @theia/editor-api, it is not a violation.
• He will need some runtime check whether actually implementation is provided, like injecting some class with @optional annotation and checking whether an instance is defined. We will need to think about it, but think how you check whether some browser API is supported or not.
• If it is there he will use it.

It allows us to go a way of coupling everything and designing new abstractions each time when we want to break coupling.

There are challenges with that, but they are not new:

• We should ensure that api packages are used. We are bad at controlling dependencies right now as well. tslint rule like tslint rule to enforce project organization #5873 would help.
• We should improve our setup if amount of packages double otherwise we will need to wait several minutes more. We have more and more packages which makes our build slow. It is time already to move from tslint to eslint, and run eslint and tsc only once for the whole code base based on ts project references.

[paul-marechal]

It still won't help, since you need then dependency to @theia/filesystem to make it work at all. We are not going to pull it in core.

To keep with the @theia/preferences issue, filesystem is only used for widgets and views, but what I mean is that since preferences are a core concept (since we need to mention it in some way in @theia/core), it must mean that we should have a basic, maybe programmatic-only, preference infrastructure in core. Simply a registry. Then the @theia/preferences would contribute all the workspace/filesystem/editor related features. Of course we would not hoist the filesystem package into core!

What about other stubs (definitions) like QuickPickService should we pull @theia/monaco in core?

Again, never said anything about hoisting @theia/monaco in core, but I didn't dive deep enough into the quick open case to understand the actual reasons of it being in core.

It would be nice to make it simple. If each package is split to API (i don't mean that we should use typescript interfaces here, I hate boilerplate) and implementation sides, then:

• A contributor can add a dependency to @theia/editor-api, it is not a violation.
• He will need some runtime check whether actually implementation is provided, like injecting some class with @optional annotation and checking whether an instance is defined. We will need to think about it, but think how you check whether some browser API is supported or not.
• If it is there he will use it.

If the API package doesn't provide interfaces, or at the very least symbols to bind, then what does it provide? Could the runtime check be done at binding time, in the container modules?

I think currently, we check out theia extensions as transitive dependencies, so if we define our app like:

"dependencies": [
    "@theia/core": "...",
    "@theia/terminal": "..."
]

It will mount @theia/filesystem and @theia/editor etc into the application. But why would we rather not mount transitive dependencies, but add checks at binding time? The packages would still be installed locally by yarn, so requires will keep working. But only at the inversify mounting time would things actually be checked out or not?

[akosyakov]

Looking at how @theia/terminal is using @theia/editor: it would be enough to move @theia/editor to dev dependencies to import Position type, and rename EDITOR_FONT_DEFAULTS to FONT_DEFAULTS and move to @theia/core. We already have notion of font css class names in core.

I think it would be more constructive if we look at real usages of @theia/terminal and see how we can handle them.

[paul-marechal]

I was only taking the @theia/terminal extension as an example, but something more concrete and on topic: @theia/debug pulling the terminals.

In this case, it would map to what you said, if I make an application as

"dependencies": [
    "@theia/core": "...",
    "@theia/debug": "..."
]

Then we may assume that someone doesn't want the terminals in his application. In this case, the generators shouldn't try to mount the @theia/terminal container modules into the application, and the @theia/debug extension should check for the optional binding in the debug-session.tsx file. We can keep the imports to the @theia/terminal package, the problem is that we don't want to load the terminals container into the app, but code/types from it can be reused. Does this make sense?

[akosyakov]

We can keep the imports to the @theia/terminal package, the problem is that we don't want to load the terminals container into the app, but code/types from it can be reused. Does this make sense?

Why would i want to download @theia/terminal at all if I want to exclude it?

[akosyakov]

For @theia/debug:

• move @theia/terminal to dev dependency
• use @optional:

@inject('TerminalService') @optional()
protected readonly terminalService: TerminalService | undefined;

• based on it tell the debug adapter whether terminal is supported:

supportsRunInTerminalRequest: !!this.terminalServer

No need for any drastic changes in how extensions are installed. @theia/terminal is not installed at all. Statically typed support whether terminal can be used or not. Proper configuration of the debug adapter.

[akosyakov]

I don't think we can remove @theia/terminal from @theia/plugin-ext. It is required to implement mandatory functionality in vscode.d.ts and theia.d.ts. The same for @theia/task. VS Code extensions won't work anymore. What is the reason to do it?

[paul-marechal]

If having something as dev-dependency makes it so generators won't mount containers, then it is exactly what I was trying to get to, thanks for the trick.

For @theia/plugin-ext, the rationale is that @thegecko wants to contribute its own plugin extension namespace, not reusing the theia nor vscode one, but that logic is not standalone at the moment. Correct me if I am wrong.

[thegecko]

@thegecko wants to contribute its own plugin

Correct, we have custom extension points and don't want to expose the terminal in our browser version

[akosyakov]

If having something as dev-dependency makes it so generators won't mount containers, then it is exactly what I was trying to get to, thanks for the trick.

Generators is designed to follow how npm/yarn works. It collects modules transitively from production dependencies. Dev dependencies are not production. We want to stick with this: if someone knows how yarn install dependencies, he should be sure that transitive are installed and loaded. Plus it removes maintenance work to keep package.json up to date whenever some extensions adds another extension dependency.

For @theia/plugin-ext, the rationale is that @thegecko wants to contribute its own plugin extension namespace, not reusing the theia nor vscode one, but that logic is not standalone at the moment. Correct me if I am wrong.

The plugin system is already complex enough because of custom namespaces and so on and we have a big maintenance effort to catch up with VS Code. We actually would like to find ways to make it simpler, see #6353 and #6353 for example.

Technically it is feasible to do what you want by extracting parts agnostic to namespaces in another extension, so you can reuse engine running plugins without features. Conceptually i don't know whether it goes in the direction of simplification or introduce another variant. You are better to discuss with @svenefftinge

Regards for other extensions i would be fine to remove dependencies for @theia/terminal. I also like the approach with dev dependencies for types and optional annotation for checking APIs. It has to be enforced somehow, but it is simpler than adding new packages or moving APIs somewhere else.

[thegecko]

So what's the consensus around moving this forward?

@svenefftinge do you have anything to add?

[akosyakov]

I would be fine extracting something like @theia/plugin-core, which is a miminal subset of @theia/plugin-ext responsible for deployment, loading and activation of plugins. It won't contain any concrete features only contribution points to add such. @thegecko do you have something like that on your mind?

@svenefftinge I don't think it collides with an intention to reuse the extension host process, or maybe even help by enabling alternative implementations.

It will be breaking change though. cc @eclipse-theia/plugin-system

[thegecko]

@akosyakov I'm more keen to implement the terminal plugin as an interface so it's implementation can be explicitly excluded from a build no matter what other packages rely on it.