-
Notifications
You must be signed in to change notification settings - Fork 269
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
[ACS-6630] Added models and API classes for extensions manager config table #9513
base: dev-swapnil-poc-extensions-manager
Are you sure you want to change the base?
[ACS-6630] Added models and API classes for extensions manager config table #9513
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@popovicsandras @DenysVuika @swapnil-verma-gl Extension should not depend from JS-API please review this PR
@eromano The dependency of extensions library on js-api is not something that is being added in this PR, but rather something that is pre-existing. This can be seen in the following instances -
Having said that, if we do decide that this is not a valid approach, then we would have to figure out another solution to provide the availability of the actionRef, ruleRef, routeRef and ruleParameter classes in js-api as we need to create backend endpoints using these classes |
I guess we need the opposite we need to clear the one you have indicated and remove js-api as dep |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not move interfaces to js-api if they do not belong to REST api calls and ACS backend
* limitations under the License. | ||
*/ | ||
|
||
export interface RuleParameter { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this should not be part of the JS-API library as it has nothing to do with the REST api calls and ACS backend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This interface is used by the RuleRef model, which is in turn being used in the newly created REST api call for saving the extension state on the ACS backend.
To not repeat the same mistake again, would it be possible to define the nx enforce-module-boundaries rule and whitelist only those packages which a package can really rely on? |
The interfaces and classes were moved because they are being planned to be used in the new REST api for saving extension state on the backend. @DenysVuika @eromano If, however, we do not want to move those models over, then I guess some of the options we have are
IMHO, I would prefer the second approach, as it would minimise code impact, and ensure that there are type restrictions in places that matter the most (components, services). However, it would also mean using the keyword |
2d3562f
to
80760de
Compare
52efec8
to
49684ed
Compare
…F. Renamed ExtensionConfig to ExtensionComposition
…ions library to js-api library
…omponents from js-api to core.
Quality Gate passedIssues Measures |
Since the movement of interfaces from extensions to js-api was impacting HxP and was not deemed a good approach, for now, what I've done is revert the movement back to its original state. The JS-API layer (settingsApi.ts) now uses the 'any' keyword for its all its purposes instead, with the layers following that (for e.g., extensions-manager.service.ts, present in adf-core) using the models and interfaces from extensions instead. |
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x")
What is the current behaviour? (You can also link to an open issue here)
APIs and models do not exist for the extensions manager configuration table feature
What is the new behaviour?
Added models and API classes for extensions manager. Moved some models from extensions library to js-api library
Does this PR introduce a breaking change? (check one with "x")
If this PR contains a breaking change, please describe the impact and migration path for existing applications: ...
Certain model classes from the extensions library have been moved over to the js-api library. As such, if there are imports of any such classes (listed below), then those would have to be updated to be imported from @alfresco/js-api
Other information: