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

fix(module-loader) - Fixed ModuleLoader behavior with multiple module catalogs #569

Closed

Conversation

lilla28
Copy link
Contributor

@lilla28 lilla28 commented Mar 25, 2024

Changes:

  • picked up changes from branch and commits related to the ModuleLoader when it tries to register its ModuleCatalog 89ec2d9 && 57ad7da
  • fixed tests when it registers its modulecatalogs (mocking array of catalogs)

@lilla28 lilla28 requested a review from a team as a code owner March 25, 2024 12:44
Copy link

codecov bot commented Mar 25, 2024

Codecov Report

Attention: Patch coverage is 76.59574% with 11 lines in your changes are missing coverage. Please review.

Project coverage is 74.37%. Comparing base (6262df0) to head (cd3318a).
Report is 1 commits behind head on main.

❗ Current head cd3318a differs from pull request most recent head 7e305ff. Consider uploading reports for the commit 7e305ff to get more accurate results

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #569      +/-   ##
==========================================
- Coverage   74.39%   74.37%   -0.03%     
==========================================
  Files         238      241       +3     
  Lines        7351     7391      +40     
  Branches      825      878      +53     
==========================================
+ Hits         5469     5497      +28     
- Misses       1689     1698       +9     
- Partials      193      196       +3     
Files Coverage Δ
...duleLoader.Abstractions/ModuleNotFoundException.cs 100.00% <100.00%> (ø)
...rganStanley.ComposeUI.ModuleLoader/ModuleLoader.cs 88.00% <100.00%> (-4.00%) ⬇️
...mposeUI.ModuleLoader/Runners/NativeModuleRunner.cs 48.48% <0.00%> (ø)
...y.ComposeUI.ModuleLoader/AggregateModuleCatalog.cs 86.66% <86.66%> (ø)
...ModuleLoader.Abstractions/ModuleLoaderException.cs 25.00% <25.00%> (ø)

... and 1 file with indirect coverage changes

@lilla28 lilla28 force-pushed the fix/module-loader-registration branch from 2468e9b to ca9c9f2 Compare March 25, 2024 18:18
kruplm
kruplm previously approved these changes Mar 26, 2024
ztanczos
ztanczos previously approved these changes Mar 27, 2024
fhubi
fhubi previously approved these changes Mar 28, 2024
@lilla28 lilla28 force-pushed the fix/module-loader-registration branch from 88a28f2 to cdbe817 Compare April 2, 2024 09:30
@lilla28 lilla28 dismissed stale reviews from kruplm, ztanczos, and fhubi via d935638 April 3, 2024 18:59
@lilla28 lilla28 force-pushed the fix/module-loader-registration branch from df2d892 to 171236b Compare April 8, 2024 13:16
- When multiple ModuleCatalog is being used, some modules could have the same module id in different ModuleCatalog which could cause issue while starting/stopping the application/module via the ModuleLoader.

## Decision
- If the same module id is being used for multiple modules (even in different ModuleCatalogs), the first occurrence will be used/saved via the ModuleLoader.
Copy link
Contributor

Choose a reason for hiding this comment

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

The same id in the same catalog shouldn't ever exist, although it's fine to describe the behavior.
I'd specify what is the first occurance in case of multiple catalogs though -> e.g. in the order of provider registration sounds like a method that is easy to understand, and provides some agency to the developer. This might have implications on how to implement merging modulecatalogs in case of async fetching.


| Type | ID | Status | Title |
| ------------- | ------------- | ------------- | ---------------------- |
| ADR | adr-014 | Proposed | Multiple ModuleCatalog |
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd suggest discussing and putting the ADR to Accepted state before merging it together with the PR

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will create a new commit with the change of the status once everything is resolved and we discussed.


- When multiple ModuleCatalog is being used, some modules could have the same module id in different ModuleCatalog which could cause issue while starting/stopping the application/module via the ModuleLoader.

## Decision
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an idea: what if we'd fail fast & hard if there are duplicates, i.e.: throw an exception and crash? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah we talked about it as well last week we talked about that it would be a kind of a weird thing to throw here an exception so we decided to go with the first item.

Copy link
Contributor

Choose a reason for hiding this comment

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

To recap, the issue with failing on duplicates from different sources is that it might cause a situation that the developer/user has no way of resolving.
Ideally this is never an issue but if merging multiple module catalogs is a feature we want to support and we wish to keep the standard interface (and not do something like e.g. prefixing ids that would be non-standard), we need a well documented consistent way of resolving these conflicts that the developer/user has a way to control.

@lilla28 lilla28 force-pushed the fix/module-loader-registration branch 4 times, most recently from 4f15a50 to cd3318a Compare April 10, 2024 08:45
ztanczos
ztanczos previously approved these changes Apr 11, 2024
kruplm
kruplm previously approved these changes Apr 11, 2024
@lilla28 lilla28 dismissed stale reviews from kruplm and ztanczos via 7e305ff April 12, 2024 13:51
@lilla28 lilla28 closed this Apr 12, 2024
@lilla28 lilla28 force-pushed the fix/module-loader-registration branch from 7e305ff to 6262df0 Compare April 12, 2024 14:12
@lilla28 lilla28 deleted the fix/module-loader-registration branch May 15, 2024 10:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants