Circular dependency in UIExtension with ISelectionListener runnin gin GLSP 2.0

[rsoika]

During my migration to GLSP V2.0 I run into another problem regarding circular dependencies.
The problem is caused by a UIExtension that looks like this:

@injectable()
export class BPMNPropertyPanel extends AbstractUIExtension implements ISelectionListener, IActionHandler, IDiagramStartup {

}

My UIExtension reacts on Actions and creates its own Actions. But the problem seems to be the ISelectionListener. During runtime I got the following Error:

Uncaught (in promise) Error: It looks like there is a circular dependency in one of the 'toDynamicValue' bindings. Please investigate bindings with service identifier 'Symbol(ISelectionListener)'.
    at resolver.js:72:129
    at tryAndThrowErrorIfStackOverflow (exceptions.js:12:1)
    at _resolveFactoryFromBinding (resolver.js:72:43)
    at _getResolvedFromBinding (resolver.js:90:1)
    at resolver.js:105:1
    at _resolveInScope (resolver.js:99:1)
    at _resolveBinding (resolver.js:104:1)
    at resolver.js:66:1
    at resolver.js:58:1
    at Array.map (<anonymous>)

The binding of my UIExtension looks actually like this:

export const BPMNPropertyModule = new FeatureModule((bind, _unbind, isBound, _isBound) => {
    bind(BPMNPropertyPanel).toSelf().inSingletonScope();
    bind(TYPES.IUIExtension).toService(BPMNPropertyPanel);
    bind(TYPES.IDiagramStartup).toService(BPMNPropertyPanel);
    bind(TYPES.ISelectionListener).toService(BPMNPropertyPanel);


    /* WORKAROUND START  */
    bind(ActionDispatcherFactory).toFactory<GLSPActionDispatcher>(
        ctx => () => ctx.container.get<GLSPActionDispatcher>(TYPES.IActionDispatcher)
    );
    /* WORKARROUND END */


    configureActionHandler({ bind, isBound }, EnableToolPaletteAction.KIND, BPMNPropertyPanel);
    configureActionHandler({ bind, isBound }, BPMNPropertyPanelToggleAction.KIND, BPMNPropertyPanel);
    configureActionHandler({ bind, isBound }, BPMNPropertyPanelUpdateAction.KIND, BPMNPropertyPanel);
});

The problem seems to be teh binding for the ISelectionListener. But using the ActionDispatcherFactory in this module seems not to solve the problem.

Note 1): When I change the binding form toService into

    bind(TYPES.ISelectionListener).to(BPMNPropertyPanel);

I got the following error:

serialization.js:61 Uncaught (in promise) Error: Circular dependency found: SelectionService --> Symbol(ISelectionListener) --> Symbol(ISelectionListener)

Note2): I have now two modules declaring a ActionDispatcherFactory with :

export const ActionDispatcherFactory = Symbol("ActionDispatcherFactory");

Can this be a problem?

[tortmayr]

Hi @rsoika,

the good news is that this issue is fixed with eclipse-glsp/glsp-client#305.
We are planning to do 2.1.0 release at the week of December 11th. This non-breaking release contains a couple of bug fixes and minor new features.

With 2.0.0 I can reproduce your problem, but for me the workaround with the ActionDispatcherFactory seems to work.
Here is the code I tested with:

@injectable()
export class BPMNPropertyPanel extends AbstractUIExtension implements ISelectionListener, IActionHandler, IDiagramStartup {
    @inject(ActionDispatcherFactory)
    protected readonly actionDispatcherFactory: ActionDispatcherFactory;

    private _actionDispatcher?: GLSPActionDispatcher;
    get actionDispatcher(): GLSPActionDispatcher {
        if (!this._actionDispatcher) {
            this._actionDispatcher = this.actionDispatcherFactory();
        }
        return this._actionDispatcher;
    }

    override id(): string {
        return 'test';
    }
    override containerClass(): string {
        return 'test';
    }
    protected override initializeContents(containerElement: HTMLElement): void {
        containerElement.innerHTML = 'Hello World';
    }

    handle(action: Action): void | Action | ICommand {
        this.actionDispatcher.dispatch(StatusAction.create('Set model action'));
    }

    selectionChanged(root: Readonly<GModelRoot>, selectedElements: string[], deselectedElements?: string[] | undefined): void {
        console.log('Here is the selection', selectedElements);
    }

    preInitialize?(): MaybePromise<void>;
}

// bindings

    bind(BPMNPropertyPanel).toSelf().inSingletonScope();
    bind(ActionDispatcherFactory).toFactory<GLSPActionDispatcher>(
        ctx => () => ctx.container.get<GLSPActionDispatcher>(TYPES.IActionDispatcher)
    );
    bind(TYPES.IUIExtension).toService(BPMNPropertyPanel);
    bind(TYPES.IDiagramStartup).toService(BPMNPropertyPanel);
    bind(TYPES.ISelectionListener).toService(BPMNPropertyPanel);
    configureActionHandler({ bind, isBound }, SetModelAction.KIND, BPMNPropertyPanel);

Note2): I have now two modules declaring a ActionDispatcherFactory with :
export const ActionDispatcherFactory = Symbol("ActionDispatcherFactory");
Can this be a problem?

No that should not be a problem. Symbols in JS are unique, the string in the constructor more or less just serves as a description.
So both your ActionDispatcherFactory declarations should not conflict with each other.

Side note:

configureActionHandler({ bind, isBound }, EnableToolPaletteAction.KIND, BPMNPropertyPanel);

This should no longer be necessary. Looking at the current implementation it seems like the EnableToolspaletteAction was just used a trigger to initialize the property panel on startup. You can now do that with IDiagramStartup.postRequestModel hook.

[rsoika]

Hi @tortmayr , I now switched to "@eclipse-glsp/client": "2.1.0-next.303" and I can see some improvements.
But still I have the problem with a circular dependency in my UIExtension regarding the selecitionListener.

@injectable()
export class BPMNPropertyPanel extends AbstractUIExtension implements ISelectionListener, IActionHandler, IDiagramStartup {
    ....
    @inject(TYPES.IActionDispatcher)
    protected readonly actionDispatcher: GLSPActionDispatcher;
    ....
    selectionChanged(root: Readonly<GModelRoot>, selectedElements: string[]): void {
     ...
     }
}    
......

export const BPMNPropertyModule = new FeatureModule((bind, unbind, isBound, rebind) => {
    bind(BPMNPropertyPanel).toSelf().inSingletonScope();
    bind(TYPES.IUIExtension).toService(BPMNPropertyPanel);
    bind(TYPES.IDiagramStartup).toService(BPMNPropertyPanel);

    const context = { bind, unbind, isBound, rebind };
    bindAsService(context, TYPES.ISelectionListener, BPMNPropertyPanel);
    .....
}

Is this construction valid in your eyes?
Do you expect that the fix you mentioned is already part of 2.1.0-next.303 ?

[rsoika]

I did a little bit more investigation and now I found out that the problem is caused by injecting a EditorContextService:

 @inject(EditorContextService)
 protected readonly editorContext: EditorContextService;

maybe this is idiotic, I use the editorContext in the method postModelInitialization()

    postModelInitialization(): MaybePromise<void> {
        this.show(this.editorContext.modelRoot);
    }

It is possible that this method was from some example code in the early beginning of my project. I guess this should not be necessary. But anyway: maybe this is a improtant information for you that injecting the editorContext can cause a circular dependency. The error message looks like this:

2024-01-07T17:43:39.025Z root WARN Couldn't restore widget for bpmn-diagram-diagram-manager. Error: Error: It looks like there is a circular dependency in one of the 'toDynamicValue' bindings. Please investigate bindings with service identifier 'Symbol(ISelectionListener)'.

[rsoika]

ok, next I found out, that calling bindAsService(context, TYPES.ISelectionListener, BPMNPropertyPanel); is a very bad thing in my code.
I found that you sometimes use a bindAsService method call like here,

and sometimes (like in this discussion) you use:

bind(TYPES.ISelectionListener).toService(BPMNPropertyPanel);

And my assumption always was, that this are two different ways to do the same thing. Now I figured out that it is not!

As you mention in the source code to this method, it is just a convenience function. But a very dangerous one. I did not recognize that this method calls also

bind(targetService).toSelf().inSingletonScope();

And this was the reason why sometimes I run in an "Ambiguous Error"

I guess the method bindAsService can't be removed anymore. But maybe you know a way to check if the service is already bound before you bind the service.

I think I am not the only one calling this method quickly by accident. However, the consequences of this method are dramatic.

[rsoika]

So, finally I found a way to get around the Circular dependency in UIExtension .
I am very sure that the problem is caused by the EditorContextService in combination with implementing a ISelectionListener

In my previous code I used the EditorContextService to get the model root - like:

@injectable()
export class BPMNPropertyPanel extends AbstractUIExtension implements IDiagramStartup, ISelectionListener, IActionHandler {
 ...
    @inject(EditorContextService)
    protected editorContext: EditorContextService;
..
   postModelInitialization(): MaybePromise<void> {
        this.show(this.editorContext.modelRoot);
    }

and this causes a Circular dependency.

My solution is now to still use the method onBeforeShow to get the model root - like this

@injectable()
export class BPMNPropertyPanel extends AbstractUIExtension implements IDiagramStartup, ISelectionListener, IActionHandler {
....
 modelRoot: Readonly<GModelRoot>;
 ...
  protected override onBeforeShow(_containerElement: HTMLElement, root: Readonly<GModelRoot>): void {
        this.modelRoot = root;
    }
..
   postModelInitialization(): MaybePromise<void> {
        this.show(this.modelRoot);
    }

But the question to me is: why is not IDiagramStartup providing the modelRoot ? Maybe a method like

 setModelRoot(root: Readonly<GModelRoot>): void

[rsoika]

HI, @tortmayr, I need to come back to this discussion. Now that I am on GLSP 2.1 I have the problem that I do no longer receive the GModelRoot during initialization phase. Neither in method postModelInitialization nor in method onBeforeShow. The parameter is always undefined.

I expected that the workaround with injecting the ActionDispatcherFactory is no longer needed and so I removed this code. The extension works fine regarding SelectionChange events. But I have no access to the model root during startup.

The question is for what IDiagramStartup and postModelInitialization are good for? Is this a bug that the model root is not transferred ?

You can see my implementation here and the bindings here.

Additional note: injecting the EditorContextService as described in the documentation example leads still to a circular dependency

[tortmayr]

Hi @rsoika,

the IDiagramStartup offers methods to hook into the diagram startup cycle and execute additional code.
Typically this used to dispatch additional custom actions on diagram startup.

As you already noticed currently the interface does not have any notion of the GModelRoot:

/**
 * Services that implement startup hooks which are invoked during the {@link DiagramLoader.load} process.
 * Typically used to dispatch additional initial actions and/or activate UI extensions on startup.
 * Execution order is derived by the `rank` property of the service. If not present, the {@link Ranked.DEFAULT_RANK} will be assumed.
 */
export interface IDiagramStartup extends Partial<Ranked> {
    /**
     * Hook for services that should be executed before the underlying GLSP client is configured and the server is initialized.
     */
    preInitialize?(): MaybePromise<void>;
    /**
     * Hook for services that should be executed before the initial model loading request (i.e. `RequestModelAction`) but
     * after the underlying GLSP client has been configured and the server is initialized.
     */
    preRequestModel?(): MaybePromise<void>;
    /**
     * Hook for services that should be executed after the initial model loading request (i.e. `RequestModelAction`).
     * Note that this hook is invoked directly after the `RequestModelAction` has been dispatched. It does not necessarily wait
     * until the client-server update roundtrip is completed. If you need to wait until the diagram is fully initialized use the
     * {@link postModelInitialization} hook.
     */
    postRequestModel?(): MaybePromise<void>;
    /* Hook for services that should be executed after the diagram model is fully initialized
     * (i.e. `ModelInitializationConstraint` is completed).
     */
    postModelInitialization?(): MaybePromise<void>;
}

so the issue with receiving the GModelRoot during initialization is probably related to your custom code an not a direct bug of the IDiagramStartup.

However, i like the idea of also passing the model root when appropriate. We could add the root as additional argument of the postModelInitialization hook (before that hook the model is not yet available). I will look into that.

You can see my implementation here and the bindings here.

Looking at your implementation, I think the issue is related this.show(this.modelRoot). Looking at the implementation of show in the AbstractUIExtension you can see that show internally calls onBeforeShow:

show(root: Readonly<SModelRootImpl>, ...contextElementIds: string[]): void {
        this.activeElement = document.activeElement;
        if (!this.containerElement) {
            if (!this.initialize()) return;
        }
        this.onBeforeShow(this.containerElement, root, ...contextElementIds);
        this.setContainerVisible(true);
    }   

So you are calling this.show(...) in your postModelInitialization hook. At this point the root is undefined.
Then onBeforeShow will be invoked and the passed root if still undefined.
u. I guess you currently only receive the actual root if a selection change occurred because then your selectionChanged method will trigger which sets the correct root.

To be hosted I'm actually wondering why this code did work you before using.2.1.x

Additional note: injecting the EditorContextService as described in the documentation example leads still to a circular dependency

Probably related to #1225. We are currently working on a generic way to decouple listener registration from the inversify construction phase, which should hopefully completely mitigate circular dependency injection issues when using listeners.

[rsoika]

Yes, but I am wondering why the method onBeforeShow from the AbstractUIExtension is providing a parameter root if that is always undefined (is this not a bug?). From your documentation example you call show in the postModelInitialization method (which will trigger onBeforeShow).

    postModelInitialization(): MaybePromise<void> {
        this.show(this.editorContext.modelRoot);
    }

In this way I use the event Listener IDiagramStartup to call a internal method with information from another api service. Hmm.... So passing the model root within the postModelInitialization will be much more clear.

And yes you are right, after the first selectionChanged event was fired, all is fine. So the problem is not super critical.