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

Inconsistent usage of ActionDispatcher #1416

Open
tortmayr opened this issue Oct 24, 2024 · 0 comments · May be fixed by eclipse-glsp/glsp-client#394
Open

Inconsistent usage of ActionDispatcher #1416

tortmayr opened this issue Oct 24, 2024 · 0 comments · May be fixed by eclipse-glsp/glsp-client#394
Labels
enhancement New feature or request

Comments

@tortmayr
Copy link
Contributor

tortmayr commented Oct 24, 2024

In GLSP we use an extended version of sprotty's ActionDispatcher that adds some additional API methods.
These API methods are not reflected in the default IActionDispatcher interface. As a consequence we often use the implementation type (GLSPActionDispatcher) directly when injecting. In the end we end up with different variants of injecting the action dispatcher:

// When we only use API methods from the default sprotty dispacher we often use:
@inject(TYPES.IActionDispatcher) protected actionDispatcher: IActionDispatcher

// When we need the extended API we either use
@inject(TYPES.IActionDispatcher protected actionDispatcher: GLSPActionDispatcher
// or 
@inject(GLSPActionDispatcher) protected actionDispatcher: GLSPActionDispatcher

In the end this is quite confusing and we end up with a weird mix of injecting either the implementation class directly, or injection the generic TYPE identifier and then set the type itself to the implementation class. In any case we have the implicit assumption here that the TYPES.IActionDispatcher class is always an instance of GLSPActionDispatcher.

While this assumption is very reasonable we should make it explicit. With @eclipse-glsp/sprotty package we have the possibility to directly add the additional API to the IActionDispatcher interface and can then consistently use the IActionDispatcher type for injecting.
In addition, this should also avoid potential breakages if an adopter binds a custom IActionDispatcher that is not derived from the GLSPActionDispatcher.

@tortmayr tortmayr added the enhancement New feature or request label Oct 24, 2024
tortmayr added a commit to eclipse-glsp/glsp-client that referenced this issue Oct 24, 2024
- Rename `action-override` module of @eclipse-glsp/sprotty to `api-override` which  a better fit.
- Add additional API methods of the `GLSPActionDispatcher` directly to the `IActionDispatcher` interface and update related code for consistent usage of the interface. Export new `IActionDispatcher` interface via api-override
- Also add API override for `IVNodePostProcessor`

Also:
- Adjust directs imports of `matchesKeystroke` and `toArray` from sprotty.  Import from @eclipse/glsp-sprotty instead

Fixes eclipse-glsp/glsp#1416
@tortmayr tortmayr linked a pull request Oct 24, 2024 that will close this issue
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant