Skip to content

Commit

Permalink
GLSP-1225: Avoid circular dependency issues in mutli services
Browse files Browse the repository at this point in the history
- Introduce `IServiceProvider` and `IContributionProvider` that allow deferred retrival of services
 or contributions (i.e. multi-bound services) form the container
- Migrate `IGModelListener` API from commandstack to the `EditorContextService`. This serves two purposes: 
  - The `EditorContextService` becomes the central component for managing model related context information
  -  The `Commandstack` is removed from the dependency chain for editor context listeners. This should  
     avoid circular dependency issues related to injecting the action provider. 

Note that base sprotty still uses multi inject for some services (ViewRegistration, ModelElementRegistrations, IVnode post processors etc.)
Currently, these bindings don't seem to cause any dependency issues. If we encounter additional issues in the future we might consider fully replacing all sprotty multiinjections with contribution providers

Also
- Add `inversify` peerDependency to client and glsp-sprotty package. Adding this dependency explicitly allows the TS-LSP to properly resolve inversify for auto-importing and quick fixing of imports.
- Add option for advanced inversify logs to standalone example. If the example is opened with
`?inversifyLog=true` extensive infos about the inversify resolution tree will be logged
- Fix visibilty of projection bars on hidden div

Fixes eclipse-glsp/glsp#1225
  • Loading branch information
tortmayr committed Mar 24, 2024
1 parent 39671a2 commit 51bf80d
Show file tree
Hide file tree
Showing 27 changed files with 580 additions and 221 deletions.
4 changes: 3 additions & 1 deletion examples/workflow-standalone/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,8 @@
},
"dependencies": {
"@eclipse-glsp-examples/workflow-glsp": "2.2.0-next",
"@eclipse-glsp/client": "2.2.0-next"
"@eclipse-glsp/client": "2.2.0-next",
"inversify-logger-middleware": "^3.1.0"
},
"devDependencies": {
"@types/shelljs": "0.8.12",
Expand All @@ -42,6 +43,7 @@
"css-loader": "^6.7.1",
"inversify": "^6.0.1",
"path-browserify": "^1.0.1",
"process": "^0.11.10",
"source-map-loader": "^4.0.0",
"style-loader": "^3.3.1",
"webpack": "^5.74.0",
Expand Down
43 changes: 40 additions & 3 deletions examples/workflow-standalone/src/di.config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,20 @@
********************************************************************************/
import { createWorkflowDiagramContainer } from '@eclipse-glsp-examples/workflow-glsp';
import {
accessibilityModule,
bindOrRebind,
ConsoleLogger,
createDiagramOptionsModule,
IDiagramOptions,
LogLevel,
STANDALONE_MODULE_CONFIG,
TYPES,
accessibilityModule,
bindOrRebind,
createDiagramOptionsModule,
toolPaletteModule
} from '@eclipse-glsp/client';
import { Container } from 'inversify';
import { makeLoggerMiddleware } from 'inversify-logger-middleware';
import '../css/diagram.css';
import { getParameters } from './url-parameters';
export default function createContainer(options: IDiagramOptions): Container {
const container = createWorkflowDiagramContainer(
createDiagramOptionsModule(options),
Expand All @@ -39,5 +41,40 @@ export default function createContainer(options: IDiagramOptions): Container {
bindOrRebind(container, TYPES.ILogger).to(ConsoleLogger).inSingletonScope();
bindOrRebind(container, TYPES.LogLevel).toConstantValue(LogLevel.warn);
container.bind(TYPES.IMarqueeBehavior).toConstantValue({ entireEdge: true, entireElement: true });
configureInversifyLogger(container);
return container;
}

function configureInversifyLogger(container: Container): void {
const parameters = getParameters();
if (!parameters.inversifyLog) {
return;
}
const logOptions = {
request: {
bindings: {
activated: true,
cache: false,
constraint: false,
dynamicValue: false,
factory: false,
implementationType: true,
onActivation: false,
provider: false,
scope: true,
serviceIdentifier: true,
type: false
},
serviceIdentifier: true,
target: {
metadata: true,
name: false,
serviceIdentifier: true
}
},
time: true
};

const logger = makeLoggerMiddleware(logOptions);
container.applyMiddleware(logger);
}
8 changes: 7 additions & 1 deletion examples/workflow-standalone/webpack.config.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/********************************************************************************
* Copyright (c) 2017-2022 TypeFox & others
* Copyright (c) 2019-2024 EclipseSource & 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 @@ -20,6 +20,9 @@ const buildRoot = path.resolve(__dirname, 'lib');
const appRoot = path.resolve(__dirname, 'app');
var CircularDependencyPlugin = require('circular-dependency-plugin');

/**
* @type {import('webpack').Configuration}
*/
module.exports = {
entry: [path.resolve(buildRoot, 'app')],
output: {
Expand Down Expand Up @@ -64,6 +67,9 @@ module.exports = {
exclude: /(node_modules|examples)\/./,
failOnError: false
}),
new webpack.ProvidePlugin({
process: 'process/browser'
}),
new webpack.WatchIgnorePlugin({ paths: [/\.js$/, /\.d\.ts$/] })
]
};
4 changes: 4 additions & 0 deletions packages/client/css/glsp-sprotty.css
Original file line number Diff line number Diff line change
Expand Up @@ -230,6 +230,10 @@
opacity: 1;
}

.sprotty.sprotty-hidden .sprotty-projection-bar {
visibility: hidden;
}

.sprotty-projection-bar.vertical.bordered-projection-bar {
position: absolute;
top: 0;
Expand Down
3 changes: 3 additions & 0 deletions packages/client/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,9 @@
"@types/file-saver": "^2.0.5",
"@types/lodash": "4.14.191"
},
"peerDependencies": {
"inversify": "^6.0.1"
},
"publishConfig": {
"access": "public"
}
Expand Down
7 changes: 4 additions & 3 deletions packages/client/src/base/action-dispatcher.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,19 +13,20 @@
*
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
********************************************************************************/
import { ActionHandlerRegistry, IActionHandler, RequestAction, ResponseAction, TYPES } from '@eclipse-glsp/sprotty';
import { expect } from 'chai';
import { Container } from 'inversify';
import { ActionHandlerRegistry, IActionHandler, RequestAction, ResponseAction, TYPES } from '@eclipse-glsp/sprotty';
import { IDiagramOptions } from '..';

import { GLSPActionDispatcher } from './action-dispatcher';
import { defaultModule } from './default.module';
import { IDiagramOptions } from './model';

const container = new Container();
container.load(defaultModule);
container.bind(TYPES.IDiagramOptions).toConstantValue(<IDiagramOptions>(<unknown>{
clientId: 'client1',
diagramType: 'diagramType',
glspClientProvider: async () => ({} as any)
glspClientProvider: async () => ({}) as any
}));
const registry = container.get(ActionHandlerRegistry);
const actionDispatcher = container.get(GLSPActionDispatcher);
Expand Down
22 changes: 19 additions & 3 deletions packages/client/src/base/action-dispatcher.ts
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@
*
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
********************************************************************************/
import { Action, ActionDispatcher, RequestAction, ResponseAction } from '@eclipse-glsp/sprotty';
import { Action, ActionDispatcher, EMPTY_ROOT, RequestAction, ResponseAction, SetModelAction } from '@eclipse-glsp/sprotty';
import { inject, injectable } from 'inversify';
import { GLSPActionHandlerRegistry } from './action-handler-registry';
import { OptionalAction } from './model/glsp-model-source';
import { ModelInitializationConstraint } from './model/model-initialization-constraint';

Expand All @@ -27,10 +28,25 @@ export class GLSPActionDispatcher extends ActionDispatcher {
protected initializationConstraint: ModelInitializationConstraint;

override initialize(): Promise<void> {
return super.initialize().then(() => this.startModelInitialization());
if (!this.initialized) {
this.initialized = this.doInitialize();
}
return this.initialized;
}

protected async doInitialize(): Promise<void> {
const registry = await this.actionHandlerRegistryProvider();
this.actionHandlerRegistry = registry;
if (registry instanceof GLSPActionHandlerRegistry) {
registry.initialize();
}
this.handleAction(SetModelAction.create(EMPTY_ROOT)).catch(() => {
/* Logged in handleAction method */
});
this.startModelInitialization();
}

startModelInitialization(): void {
protected startModelInitialization(): void {
if (!this.initializedConstraint) {
this.logger.log(this, 'Starting model initialization mode');
this.initializationConstraint.onInitialized(() => this.logger.log(this, 'Model initialization completed'));
Expand Down
69 changes: 35 additions & 34 deletions packages/client/src/base/action-handler-registry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,56 +14,57 @@
* SPDX-License-Identifier: EPL-2.0 OR GPL-2.0 WITH Classpath-exception-2.0
********************************************************************************/

import { ActionHandlerRegistration, ActionHandlerRegistry, IActionHandler, IActionHandlerInitializer, TYPES } from '@eclipse-glsp/sprotty';
import { inject, injectable, named } from 'inversify';
import { IContributionProvider } from '../utils/contribution-provider';
import {
ActionHandlerRegistration,
ActionHandlerRegistry,
BindingContext,
IActionHandlerInitializer,
TYPES,
bindOrRebind
} from '@eclipse-glsp/sprotty';
import { decorate, inject, injectable, unmanaged } from 'inversify';
import { IContributionProvider } from './contribution-provider';

@injectable()
export class GLSPActionHandlerRegistry extends ActionHandlerRegistry {
@inject(TYPES.IContributionProvider)
@named(TYPES.ActionHandlerRegistration)
protected readonly registrations: IContributionProvider<ActionHandlerRegistration>;

@inject(TYPES.IContributionProvider)
@named(TYPES.IActionHandlerInitializer)
protected readonly initializers: IContributionProvider<IActionHandlerInitializer>;
protected contributionProvider: IContributionProvider;

protected initialized = false;

constructor() {
super([], []);
}

protected init(): void {
if (!this.initialized) {
this.initialized = true;
this.registrations.getContributions().forEach(registration => this.register(registration.actionKind, registration.factory()));
this.initializers.getContributions().forEach(initializer => this.initializeActionHandler(initializer));
}
}

override register(key: string, instance: IActionHandler): void {
this.init();
super.register(key, instance);
}

override get(key: string): IActionHandler[] {
this.init();
return super.get(key);
}

override initializeActionHandler(initializer: IActionHandlerInitializer): void {
this.init();
super.initializeActionHandler(initializer);
}

/**
* Retrieve a set of all action kinds for which (at least) one
* handler is registered
* @returns the set of handled action kinds
*/
getHandledActionKinds(): string[] {
this.init();
return Array.from(this.elements.keys());
}

initialize(): void {
if (this.initialized) {
return;
}

this.contributionProvider
.getAll<ActionHandlerRegistration>(TYPES.ActionHandlerRegistration)
.forEach(registration => this.register(registration.actionKind, registration.factory()));
this.contributionProvider
.getAll<IActionHandlerInitializer>(TYPES.IActionHandlerInitializer)
.forEach(initializer => this.initializeActionHandler(initializer));
}
}

let baseClassDecorated = false;
export function bindActionHandlerRegistry(context: Omit<BindingContext, 'unbind'>): void {
context.bind(GLSPActionHandlerRegistry).toSelf().inSingletonScope();
bindOrRebind(context, ActionHandlerRegistry).toService(GLSPActionHandlerRegistry);
if (!baseClassDecorated) {
decorate(unmanaged(), ActionHandlerRegistry, 0);
decorate(unmanaged(), ActionHandlerRegistry, 1);
baseClassDecorated = true;
}
}
42 changes: 15 additions & 27 deletions packages/client/src/base/command-stack.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,50 +17,38 @@ import {
CommandStack,
Disposable,
DisposableCollection,
Emitter,
Event,
GModelRoot,
ICommand,
SetModelCommand,
TYPES,
UpdateModelCommand
} from '@eclipse-glsp/sprotty';
import { injectable, multiInject, optional, preDestroy } from 'inversify';

/**
* A hook to listen for model root changes. Will be called after a server update
* has been processed
*/
export interface IGModelRootListener {
modelRootChanged(root: Readonly<GModelRoot>): void;
}

/**
* @deprecated Use {@link IGModelRootListener} instead
*/
export type ISModelRootListener = IGModelRootListener;
import { inject, injectable, preDestroy } from 'inversify';
import { EditorContextService } from './editor-context-service';
import { IServiceProvider } from './service-provider';

@injectable()
export class GLSPCommandStack extends CommandStack implements Disposable {
@multiInject(TYPES.IGModelRootListener)
@optional()
protected modelRootListeners: IGModelRootListener[] = [];
protected toDispose = new DisposableCollection();
@inject(TYPES.IServiceProvider)
protected serviceProvider: IServiceProvider;

protected override initialize(): void {
super.initialize();
this.toDispose.push(this.onModelRootChangedEmitter);
this.modelRootListeners.forEach(listener => this.onModelRootChanged(root => listener.modelRootChanged(root)));
}
protected toDispose = new DisposableCollection();

@preDestroy()
dispose(): void {
this.toDispose.dispose();
}

protected onModelRootChangedEmitter = new Emitter<Readonly<GModelRoot>>();
get editorContext(): EditorContextService {
return this.serviceProvider.get(EditorContextService);
}

/**
* @deprecated Use the `EditorContext.onModelRootChanged` event instead
*/
get onModelRootChanged(): Event<Readonly<GModelRoot>> {
return this.onModelRootChangedEmitter.event;
return this.editorContext.onModelRootChanged;
}

override undo(): Promise<GModelRoot> {
Expand Down Expand Up @@ -88,6 +76,6 @@ export class GLSPCommandStack extends CommandStack implements Disposable {
}

protected notifyListeners(root: Readonly<GModelRoot>): void {
this.onModelRootChangedEmitter.fire(root);
this.editorContext.notifyModelRootChanged(root, this);
}
}
Loading

0 comments on commit 51bf80d

Please sign in to comment.