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

Improve working with featureModules, container configurations #1349

Closed
tortmayr opened this issue Jun 6, 2024 · 0 comments · Fixed by eclipse-glsp/glsp-client#355
Closed
Labels
bug Something isn't working client

Comments

@tortmayr
Copy link
Contributor

tortmayr commented Jun 6, 2024

Certain aspect of the feature module/container configuration API could be improved

Uniqueness check of featureIds. : FeatureModules operate on the assumption that the featureId of a module is unique among the set of feature modules that should be loaded into the container. Currently we don't check that. In case of duplicated ids, inversify will fail when trying to bind the featureId a second time. However, the resulting error is kind of cryptic and non-intuitive for adopters.
We should implement additional checks both at the end of resolveContainerConfiguration and when trying to load a feature module to catch this problem and throw a better Error.

Issue with replacing a preloaded feature module with a custom variant:
In certain usecases adopters want to full replace one of the default modules with a customized version. Currently this can be done by:

// Reusing the featureId of the module you want to replace for your custom module
const customBoundsModule= new FeatureModule(()=>{//do something}, {featureId: boundsModule.featureId});
and then remove the module you want to replace and add the custom variant instead as part of the container configuration:
configureDiagramContainer(...containerConfiguration,{remove: boundsModule, add: customBoundsModule}; 

This works, but messes up the module order. (bounds module gets removed from its original index location and is added at the end of the resolved module array). Which than has impacts on feature module loading. By affectivly moving the bounds module to the end of the array, loading other featureModules that require the bounds module will fail.

We should introduce a dedicated `replace´ option as part of the container configuration.

@tortmayr tortmayr added bug Something isn't working client labels Jun 6, 2024
tortmayr added a commit to eclipse-glsp/glsp-client that referenced this issue Jun 6, 2024
- Add `replace` property to `ModulConfiguration` interface. When resolving feature modules of the replace property, any potential already configured module with the same id will be replaced with the defined module. If there is nothing to replace, the module will be added to the end of the resolved configuration
- Add check for non-unique feature module ids to `resolveContainerConfiguration` function.
- Add option to enable additional debug logging when loading feature modules. This is usefull during development to get insights about why the container modules are not loaded as expected
- Add dedicated `featureIds` to all default modules. If no featureId is provided the number id of the module is reused for creating the featureId. While this works perfectly fine, it makes debugging harder because feature modules are hard to indentify based on their featureid.
With this change its now easier to derive the corresponding feature module from the featureid.

Fixes eclipse-glsp/glsp#1349
tortmayr added a commit to eclipse-glsp/glsp-client that referenced this issue Jun 9, 2024
- Add `replace` property to `ModulConfiguration` interface. When resolving feature modules of the replace property, any potential already configured module with the same id will be replaced with the defined module. If there is nothing to replace, the module will be added to the end of the resolved configuration
- Add check for non-unique feature module ids to `resolveContainerConfiguration` function.
- Add option to enable additional debug logging when loading feature modules. This is usefull during development to get insights about why the container modules are not loaded as expected
- Add dedicated `featureIds` to all default modules. If no featureId is provided the number id of the module is reused for creating the featureId. While this works perfectly fine, it makes debugging harder because feature modules are hard to indentify based on their featureid.
With this change its now easier to derive the corresponding feature module from the featureid.

Fixes eclipse-glsp/glsp#1349
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working client
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant