Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

refactor core messaging and RPC (electron-only so far) #11076

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ const serverModule = require('./server');
const serverAddress = main.start(serverModule());

serverAddress.then(({ port, address }) => {
if (process && process.send) {
if (process.send) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is what I mean by "drive by fix". Yes, it's a good fix, but unrelated to the problem at hand.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair enough on this one.

process.send({ port, address });
}
});
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@
import * as chalk from 'chalk';
import * as fs from 'fs-extra';
import * as path from 'path';
import { sortLocalization } from '.';
import { Localization } from './common';
import { Localization, sortLocalization } from './common';
import { deepl, DeeplLanguage, DeeplParameters, isSupportedLanguage, supportedLanguages } from './deepl-api';

export interface LocalizationOptions {
Expand Down
3 changes: 1 addition & 2 deletions dev-packages/private-re-exports/src/package-re-exports.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@
import cp = require('child_process');
import fs = require('fs');
import path = require('path');
import { parseModule } from '.';
import { PackageJson, ReExportJson } from './utility';
paul-marechal marked this conversation as resolved.
Show resolved Hide resolved
import { PackageJson, parseModule, ReExportJson } from './utility';

export async function readJson<T = unknown>(jsonPath: string): Promise<T> {
return JSON.parse(await fs.promises.readFile(jsonPath, 'utf8')) as T;
Expand Down
6 changes: 3 additions & 3 deletions doc/Migration.md
Original file line number Diff line number Diff line change
Expand Up @@ -42,14 +42,14 @@ This version updates the Monaco code used in Theia to the state of VSCode 1.65.2

#### ASM to ESM

Two kinds of changes may be required to consume Monaco using ESM modules.
Two kinds of changes may be required to consume Monaco using ESM modules.

- If your application uses its own Webpack config rather than that generated by the @theia/dev-packages, you
will need to update that config to remove the `CopyWebpackPlugin` formerly used to place Monaco
code in the build folder and to build a separate entrypoint for the `editor.worker`. See [the changes here](https://github.com/eclipse-theia/theia/pull/10736/files#diff-b4677f3ff57d8b952eeefc10493ed3600d2737f9b5c9b0630b172472acb9c3a2)
- If your application uses its own frontend generator, you should modify the code that generates the `index.html` to load the `script` containing the bundle into the `body` element rather than the head. See [changes here](https://github.com/eclipse-theia/theia/pull/10947/files)
- References to the `window.monaco` object should be replaced with imports from `@theia/monaco-editor-core`. In most cases, simply adding an import `import * as monaco from
'@theia/monaco-editor-core'` will suffice. More complex use cases may require imports from specific parts of Monaco. Please see
- References to the `window.monaco` object should be replaced with imports from `@theia/monaco-editor-core`. In most cases, simply adding an import `import * as monaco from
'@theia/monaco-editor-core'` will suffice. More complex use cases may require imports from specific parts of Monaco. Please see
[the PR](https://github.com/eclipse-theia/theia/pull/10736) for details, and please post any questions or problems there.

Using ESM modules, it is now possible to follow imports to definitions and to the Monaco source code. This should aid in tracking down issues related to changes in Monaco discussed
Expand Down
4 changes: 4 additions & 0 deletions examples/api-samples/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,10 @@
{
"electronMain": "lib/electron-main/update/sample-updater-main-module",
"frontendElectron": "lib/electron-browser/updater/sample-updater-frontend-module"
},
{
"electronMain": "lib/electron-main/ipc/electron-main-ipc-module",
"backendElectron": "lib/electron-node/ipc/electron-backend-ipc-module"
}
],
"keywords": [
Expand Down
17 changes: 6 additions & 11 deletions examples/api-samples/src/common/updater/sample-updater.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,25 +13,20 @@
//
// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
// *****************************************************************************
import { JsonRpcServer } from '@theia/core/lib/common/messaging/proxy-factory';

import { Event, serviceIdentifier, servicePath } from '@theia/core/lib/common';

export enum UpdateStatus {
InProgress = 'in-progress',
Available = 'available',
NotAvailable = 'not-available'
}

export const SampleUpdaterPath = '/services/sample-updater';
export const SampleUpdater = Symbol('SampleUpdater');
export interface SampleUpdater extends JsonRpcServer<SampleUpdaterClient> {
export const SampleUpdaterPath = servicePath<SampleUpdater>('/services/sample-updater');
export const SampleUpdater = serviceIdentifier<SampleUpdater>('SampleUpdater');
export interface SampleUpdater {
onReadyToInstall: Event<void>;
checkForUpdates(): Promise<{ status: UpdateStatus }>;
onRestartToUpdateRequested(): void;
disconnectClient(client: SampleUpdaterClient): void;

setUpdateAvailable(available: boolean): Promise<void>; // Mock
}

export const SampleUpdaterClient = Symbol('SampleUpdaterClient');
export interface SampleUpdaterClient {
notifyReadyToInstall(): void;
}
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,6 @@ import { inject, injectable, postConstruct } from '@theia/core/shared/inversify'
import { isOSX } from '@theia/core/lib/common/os';
import { CommonMenus } from '@theia/core/lib/browser';
import {
Emitter,
Command,
MenuPath,
MessageService,
Expand All @@ -31,7 +30,7 @@ import {
} from '@theia/core/lib/common';
import { ElectronMainMenuFactory } from '@theia/core/lib/electron-browser/menu/electron-main-menu-factory';
import { FrontendApplicationConfigProvider } from '@theia/core/lib/browser/frontend-application-config-provider';
import { SampleUpdater, UpdateStatus, SampleUpdaterClient } from '../../common/updater/sample-updater';
import { SampleUpdater, UpdateStatus } from '../../common/updater/sample-updater';

export namespace SampleUpdaterCommands {

Expand Down Expand Up @@ -68,18 +67,6 @@ export namespace SampleUpdaterMenu {
export const MENU_PATH: MenuPath = [...CommonMenus.FILE_SETTINGS_SUBMENU, '3_settings_submenu_update'];
}

@injectable()
export class SampleUpdaterClientImpl implements SampleUpdaterClient {

protected readonly onReadyToInstallEmitter = new Emitter<void>();
readonly onReadyToInstall = this.onReadyToInstallEmitter.event;

notifyReadyToInstall(): void {
this.onReadyToInstallEmitter.fire();
}

}
Comment on lines -71 to -81
Copy link
Member Author

@paul-marechal paul-marechal May 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a very important change with the new RPC/proxy API: we no longer setup "clients" to get notifications back from the remote: we can use Event fields directly instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is actually an improvement. How is this better than the old way of setting up service objects on both ends?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I fear this might make initialization of service objects more complicated.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see how this makes things more complicated. I keep deleting code to use this new API.


// Dynamic menus aren't yet supported by electron: https://github.com/eclipse-theia/theia/issues/446
@injectable()
export class ElectronMenuUpdater {
Expand Down Expand Up @@ -113,14 +100,11 @@ export class SampleUpdaterFrontendContribution implements CommandContribution, M
@inject(SampleUpdater)
protected readonly updater: SampleUpdater;

@inject(SampleUpdaterClientImpl)
protected readonly updaterClient: SampleUpdaterClientImpl;

protected readyToUpdate = false;

@postConstruct()
protected init(): void {
this.updaterClient.onReadyToInstall(async () => {
this.updater.onReadyToInstall(async () => {
Comment on lines -116 to +107
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's an example of directly using Event fields directly on the proxy. Less boilerplate code than with clients.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I generally don't mind boilerplate code if it's simpler to understand than the alternative. Typing is not the problem in software construction.

Copy link
Member Author

@paul-marechal paul-marechal May 11, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Typing prevents mistakes. This apart, the previous required boilerplate code for the xxxClient API was often misused which I assumed was a symptom of a design problem.

The new API allows servers to expose their events through proxies. For requests going the other way from the main RPC setup you can just create a new dedicated proxy.

Just as an example, the most egregious implementation I've seen using clients must be this one:

this.terminalWatcher.onUpdateTerminalEnvVariablesRequested(() => {
this.storageService.getData<string>(ENVIRONMENT_VARIABLE_COLLECTIONS_KEY).then(data => {
if (data) {
const collectionsJson: SerializableExtensionEnvironmentVariableCollection[] = JSON.parse(data);
collectionsJson.forEach(c => this.shellTerminalServer.setCollection(c.extensionIdentifier, true, c.collection));
}
});
});

Here TerminalWatcher is a client to TerminalServer. And instead of having the remote server call a method to fetch collections from its clients and use the result, it does this roundabout procedure where it sends an event and expects the receiving side to initiate subsequent RPC calls (plural) with this.shellTerminalServer.setCollection(...).

In my latest changes I replaced the idea of the client on the server side by a proxy from the frontend implementing storage methods. The server just does environmentStore.getEnvironmentVariables() or environmentStore.saveEnvironmentVariables(vars), it is transparently handled by the frontend.

this.readyToUpdate = true;
this.menuUpdater.update();
this.handleUpdatesAvailable();
Expand All @@ -138,7 +122,7 @@ export class SampleUpdaterFrontendContribution implements CommandContribution, M
}
case UpdateStatus.NotAvailable: {
const { applicationName } = FrontendApplicationConfigProvider.get();
this.messageService.info(`[Not Available]: Youre all good. Youve got the latest version of ${applicationName}.`, { timeout: 3000 });
this.messageService.info(`[Not Available]: You're all good. You've got the latest version of ${applicationName}.`, { timeout: 3000 });
break;
}
case UpdateStatus.InProgress: {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -14,21 +14,17 @@
// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
// *****************************************************************************

import { bindContribution, CommandContribution, MenuContribution, ProxyProvider } from '@theia/core/lib/common';
import { ElectronMainAndFrontend } from '@theia/core/lib/electron-common';
import { ContainerModule } from '@theia/core/shared/inversify';
import { ElectronIpcConnectionProvider } from '@theia/core/lib/electron-browser/messaging/electron-ipc-connection-provider';
import { CommandContribution, MenuContribution } from '@theia/core/lib/common';
import { SampleUpdater, SampleUpdaterPath, SampleUpdaterClient } from '../../common/updater/sample-updater';
import { SampleUpdaterFrontendContribution, ElectronMenuUpdater, SampleUpdaterClientImpl } from './sample-updater-frontend-contribution';
import { SampleUpdater, SampleUpdaterPath } from '../../common/updater/sample-updater';
import { ElectronMenuUpdater, SampleUpdaterFrontendContribution } from './sample-updater-frontend-contribution';

export default new ContainerModule(bind => {
bind(SampleUpdater)
.toDynamicValue(ctx => ctx.container.getNamed(ProxyProvider, ElectronMainAndFrontend).getProxy(SampleUpdaterPath))
.inSingletonScope();
bind(ElectronMenuUpdater).toSelf().inSingletonScope();
bind(SampleUpdaterClientImpl).toSelf().inSingletonScope();
bind(SampleUpdaterClient).toService(SampleUpdaterClientImpl);
bind(SampleUpdater).toDynamicValue(context => {
const client = context.container.get(SampleUpdaterClientImpl);
return ElectronIpcConnectionProvider.createProxy(context.container, SampleUpdaterPath, client);
}).inSingletonScope();
bind(SampleUpdaterFrontendContribution).toSelf().inSingletonScope();
bind(MenuContribution).toService(SampleUpdaterFrontendContribution);
bind(CommandContribution).toService(SampleUpdaterFrontendContribution);
bindContribution(bind, SampleUpdaterFrontendContribution, [MenuContribution, CommandContribution]);
});
23 changes: 23 additions & 0 deletions examples/api-samples/src/electron-common/ipc/electron-ipc.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
// *****************************************************************************
// Copyright (C) 2022 TypeFox and others.
//
// This program and the accompanying materials are made available under the
// terms of the Eclipse Public License v. 2.0 which is available at
// http://www.eclipse.org/legal/epl-2.0.
//
// This Source Code may also be made available under the following Secondary
// Licenses when the conditions for such availability set forth in the Eclipse
// Public License v. 2.0 are satisfied: GNU General Public License, version 2
// with the GNU Classpath Exception which is available at
// https://www.gnu.org/software/classpath/license.html.
//
// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
// *****************************************************************************

import { serviceIdentifier, servicePath } from '@theia/core/lib/common';

export const ELECTRON_MAIN_AND_BACKEND_IPC_SAMPLE_PATH = servicePath<ElectronMainAndBackendIpcSample>('/services/test-connection');
export const ElectronMainAndBackendIpcSample = serviceIdentifier<ElectronMainAndBackendIpcSample>('TestConnection');
export interface ElectronMainAndBackendIpcSample {
getBrowserWindowTitles(): Promise<string[]>
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// *****************************************************************************
// Copyright (C) 2020 Ericsson and others.
// Copyright (C) 2022 TypeFox and others.
//
// This program and the accompanying materials are made available under the
// terms of the Eclipse Public License v. 2.0 which is available at
Expand All @@ -14,17 +14,14 @@
// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
// *****************************************************************************

import { ConnectionHandler } from '../../common/messaging/handler';
import { BrowserWindow } from '@theia/core/electron-shared/electron';
import { injectable } from '@theia/core/shared/inversify';
import { ElectronMainAndBackendIpcSample } from '../../electron-common/ipc/electron-ipc';

/**
* Name of the channel used with `ipcMain.on/emit`.
*/
export const THEIA_ELECTRON_IPC_CHANNEL_NAME = 'theia-electron-ipc';
@injectable()
export class ElectronMainAndBackendIpcSampleImpl implements ElectronMainAndBackendIpcSample {

/**
* Electron-IPC-specific connection handler.
* Use this if you want to establish communication between the frontend and the electron-main process.
*/
export const ElectronConnectionHandler = Symbol('ElectronConnectionHandler');
export interface ElectronConnectionHandler extends ConnectionHandler {
async getBrowserWindowTitles(): Promise<string[]> {
return BrowserWindow.getAllWindows().map(browserWindow => browserWindow.getTitle());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,32 @@
// *****************************************************************************
// Copyright (C) 2022 Ericsson// *****************************************************************************
// Copyright (C) 2020 TypeFox and others.
//
// This program and the accompanying materials are made available under the
// terms of the Eclipse Public License v. 2.0 which is available at
// http://www.eclipse.org/legal/epl-2.0.
//
// This Source Code may also be made available under the following Secondary
// Licenses when the conditions for such availability set forth in the Eclipse
// Public License v. 2.0 are satisfied: GNU General Public License, version 2
// with the GNU Classpath Exception which is available at
// https://www.gnu.org/software/classpath/license.html.
//
// SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
// *****************************************************************************

import { ServiceContribution } from '@theia/core/lib/common';
import { ElectronMainAndBackend } from '@theia/core/lib/electron-common';
import { ContainerModule } from '@theia/core/shared/inversify';
import { ElectronMainAndBackendIpcSample, ELECTRON_MAIN_AND_BACKEND_IPC_SAMPLE_PATH } from '../../electron-common/ipc/electron-ipc';
import { ElectronMainAndBackendIpcSampleImpl } from './electron-main-ipc-impl';

export default new ContainerModule(bind => {
bind(ElectronMainAndBackendIpcSample).to(ElectronMainAndBackendIpcSampleImpl).inSingletonScope();
bind(ServiceContribution)
.toDynamicValue(ctx => ({
[ELECTRON_MAIN_AND_BACKEND_IPC_SAMPLE_PATH]: () => ctx.container.get(ElectronMainAndBackendIpcSample)
}))
.inSingletonScope()
.whenTargetNamed(ElectronMainAndBackend);
});
Original file line number Diff line number Diff line change
Expand Up @@ -16,15 +16,20 @@

import { injectable } from '@theia/core/shared/inversify';
import { ElectronMainApplication, ElectronMainApplicationContribution } from '@theia/core/lib/electron-main/electron-main-application';
import { SampleUpdater, SampleUpdaterClient, UpdateStatus } from '../../common/updater/sample-updater';
import { SampleUpdater, UpdateStatus } from '../../common/updater/sample-updater';
import { Emitter, Event } from '@theia/core/lib/common';

@injectable()
export class SampleUpdaterImpl implements SampleUpdater, ElectronMainApplicationContribution {

protected clients: Array<SampleUpdaterClient> = [];
protected onReadyToInstallEmitter = new Emitter<void>();
protected inProgressTimer: NodeJS.Timer | undefined;
protected available = false;

get onReadyToInstall(): Event<void> {
return this.onReadyToInstallEmitter.event;
}

async checkForUpdates(): Promise<{ status: UpdateStatus }> {
if (this.inProgressTimer) {
return { status: UpdateStatus.InProgress };
Expand All @@ -48,9 +53,7 @@ export class SampleUpdaterImpl implements SampleUpdater, ElectronMainApplication
this.inProgressTimer = setTimeout(() => {
this.inProgressTimer = undefined;
this.available = true;
for (const client of this.clients) {
client.notifyReadyToInstall();
}
this.onReadyToInstallEmitter.fire();
}, 5000);
}
}
Expand All @@ -62,30 +65,4 @@ export class SampleUpdaterImpl implements SampleUpdater, ElectronMainApplication
onStop(application: ElectronMainApplication): void {
// Invoked when the contribution is stopping. You can clean up things here. You are not allowed call async code from here.
}

setClient(client: SampleUpdaterClient | undefined): void {
if (client) {
this.clients.push(client);
console.info('Registered a new sample updater client.');
} else {
console.warn("Couldn't register undefined client.");
}
}

disconnectClient(client: SampleUpdaterClient): void {
const index = this.clients.indexOf(client);
if (index !== -1) {
this.clients.splice(index, 1);
console.info('Disposed a sample updater client.');
} else {
console.warn("Couldn't dispose client; it was not registered.");
}
}

dispose(): void {
console.info('>>> Disposing sample updater service...');
this.clients.forEach(this.disconnectClient.bind(this));
console.info('>>> Disposed sample updater service.');
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -15,22 +15,27 @@
// *****************************************************************************

import { ContainerModule } from '@theia/core/shared/inversify';
import { JsonRpcConnectionHandler } from '@theia/core/lib/common/messaging/proxy-factory';
import { ElectronMainApplicationContribution } from '@theia/core/lib/electron-main/electron-main-application';
import { ElectronConnectionHandler } from '@theia/core/lib/electron-common/messaging/electron-connection-handler';
import { SampleUpdaterPath, SampleUpdater, SampleUpdaterClient } from '../../common/updater/sample-updater';
import { SampleUpdaterPath, SampleUpdater } from '../../common/updater/sample-updater';
import { SampleUpdaterImpl } from './sample-updater-impl';
import { ServiceContribution } from '@theia/core/lib/common';
import { ElectronMainAndFrontend } from '@theia/core/lib/electron-common';

export const SampleUpdaterElectronMainAndFrontendContainerModule = new ContainerModule(bind => {
bind(ServiceContribution)
.toDynamicValue(ctx => ({
// This will return the same singleton instance from the main container module
// for `SampleUpdater`, but this is by design here.
[SampleUpdaterPath]: () => ctx.container.get(SampleUpdater)
}))
.inSingletonScope()
.whenTargetNamed(ElectronMainAndFrontend);
});

export default new ContainerModule(bind => {
bind(SampleUpdaterImpl).toSelf().inSingletonScope();
bind(SampleUpdater).toService(SampleUpdaterImpl);
bind(SampleUpdater).to(SampleUpdaterImpl).inSingletonScope();
bind(ElectronMainApplicationContribution).toService(SampleUpdater);
bind(ElectronConnectionHandler).toDynamicValue(context =>
new JsonRpcConnectionHandler<SampleUpdaterClient>(SampleUpdaterPath, client => {
const server = context.container.get<SampleUpdater>(SampleUpdater);
server.setClient(client);
client.onDidCloseConnection(() => server.disconnectClient(client));
return server;
})
).inSingletonScope();
bind(ContainerModule)
.toConstantValue(SampleUpdaterElectronMainAndFrontendContainerModule)
.whenTargetNamed(ElectronMainAndFrontend);
});
Loading